sql: add option to enable/disable txn id cache #76523
Conversation
|
This change is |
Azhng
force-pushed
the
ctn-cluster-setting-gate
branch
from
e5a9b96
to
8db642f
Compare
There was a problem hiding this comment.
Funny timing, I was reading the initial settings RFC earlier today, and I saw this section, which IIUC would recommend instead using the existing sql.contention.txn_id_cache.max_size setting with a value of 0 to disable the txn id cache.
Perhaps that philosophy has been superseded by something else? But when I look through the generated settings.html, I'm mostly seeing either .enabled or .<value>, but not both. WDYT?
Reviewable status:
complete! 0 of 0 LGTMs obtained
Azhng
force-pushed
the
ctn-cluster-setting-gate
branch
2 times, most recently
from
f82e1ff
to
f6cae3b
Compare
There was a problem hiding this comment.
Good point. Changed this to use the special zero value.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/sql/contention/txnidcache/cluster_settings.go, line 20 at r5 (raw file):
`sql.contention.txn_id_cache.max_size`, "the maximum byte size TxnID cache will use. Set it to zero to disable "+ "txn id cache",
I'm reading the other settings, git grep 'to 0' docs/generated/settings/settings-for-tenants.txt, and maybe something like "the maximum byte size TxnID cache will use (set to 0 to disable)" or "the maximum byte size TxnID cache will use; if set to 0, the TxnID cache is disabled" would fit the existing style, WDYT?
Azhng
force-pushed
the
ctn-cluster-setting-gate
branch
from
f6cae3b
to
8aab5b5
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/contention/txnidcache/cluster_settings.go, line 20 at r5 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
I'm reading the other settings,
git grep 'to 0' docs/generated/settings/settings-for-tenants.txt, and maybe something like "the maximum byte size TxnID cache will use (set to 0 to disable)" or "the maximum byte size TxnID cache will use; if set to 0, the TxnID cache is disabled" would fit the existing style, WDYT?
Done.
There was a problem hiding this comment.
Thanks!
Reviewable status:
There was a problem hiding this comment.
Nit: you need to update your PR title/description to match your new implementation
Reviewed 2 of 2 files at r1, 5 of 5 files at r2, 6 of 6 files at r3.
Reviewable status:
Azhng
force-pushed
the
ctn-cluster-setting-gate
branch
from
8aab5b5
to
9649660
Compare
Azhng
changed the title
|
Done. |
Azhng
force-pushed
the
ctn-cluster-setting-gate
branch
from
9649660
to
13b80bb
Compare
Release note: None
Resolves cockroachdb#76329 Release note (sql change): when `sql.contention.txn_id_cache.max_size` is set to 0, it would effectively turn off transaction ID cache.
Azhng
force-pushed
the
ctn-cluster-setting-gate
branch
from
13b80bb
to
cb35150
Compare
|
TFTR! bors r+ |
|
Build succeeded: |
Reviewer note: only last commit is relevant
Resolves #76329
Release note (sql change): when
sql.contention.txn_id_cache.max_sizeis set to 0, it would effectively turn off transaction ID cache.