sql: default to node-level cache for sequences #148290

bghal · 2025-06-13T16:28:20Z

The session-level cache was implemented first and shared node-level cache was added in #118546.
The better behaving node-level cache is not the default option.
Changes the default for CACHE option to set node-level cache. A new option to allows the session-level cache to be set.

Epic: CRDB-42587
Fixes: #131531

Release note (sql change): Changed the basic sequence cache option to set cache at the node-level. The PER SESSION NODE sequence option (syntax is is [ PER SESSION CACHE # ]) provides the previous behavior to set session-level cache.

cockroach-teamcity · 2025-06-13T16:28:34Z

This change is

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/catalog/descpb/structured.go line 301 at r1 (raw file):

// EffectiveCacheSize evaluates the cache size fields of a sequence option.
// A cache size of 1 indicates that the cache is disabled.
// The session cache size takes precedence: If set and enabled, it will be the used.

super nit: "it will be the one used."

(feel free to amend this and update the PR, and merge whenever ready)

rafiss

there are a few things that need updating as a side effect of this:

run ./dev gen docs to update the auto-generated syntax docs.
run ./dev test pkg/sql/opt/optbuilder -f=TestBuilder --rewrite to update an optimizer test that was affected by this.
run ./dev test pkg/sql/opt/testutils/testcat -f=TestCatalog --rewrite to update another optimizer test

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bghal)

pkg/sql/catalog/descpb/structured.go line 301 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

super nit: "it will be the one used."

(feel free to amend this and update the PR, and merge whenever ready)

my bad -- i meant to add this comment on your first PR.

pkg/sql/show_create_clauses.go line 622 at r2 (raw file):

		f.Printf(" PER SESSION CACHE %d", opts.SessionCacheSize)
	} else if opts.NodeCacheSize > 1 {
		f.Printf(" CACHE %d", opts.NodeCacheSize)

nit: let's actually keep the old formatting logic of showing PER NODE CACHE in this case. when we have the choice, we should normalize the statement to the less ambiguous form.

pkg/sql/pg_catalog.go line 4196 at r2 (raw file):

					tree.NewDInt(tree.DInt(opts.Increment)),        // increment_by
					tree.DBoolFalse,                                // cycle
					tree.NewDInt(tree.DInt(opts.SessionCacheSize)), // cache_size

nit: i think we should change this to be EffectiveCacheSize(). (this was an oversight from before your PR, but may as well fix it now.)

spilchen

Reviewed 12 of 17 files at r2, 15 of 18 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bghal and @rafiss)

pkg/sql/logictest/testdata/logic_test/datetime line 2102 at r3 (raw file):

----
make_timestamptz
-2013-07-15 08:15:23.5 -0500 -0500

Was this change intentional? It doesn't seem related to the sequence change. And I saw that it's causing diffs in the CI run.

pkg/sql/logictest/testdata/logic_test/alter_sequence line 42 at r3 (raw file):

SELECT nextval('foo');
----
17

Isn't the sequence behaviour going to stay the same? We had set the session cache before (implicilty), but now we are doing it explicitly. I'm wondering why the sequence values needed to change. I saw this is picked up as a CI diff too.

blathers-crl · 2025-06-17T15:51:16Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

bghal

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @spilchen)

pkg/sql/logictest/testdata/logic_test/alter_sequence line 42 at r3 (raw file):

Previously, spilchen wrote…

Isn't the sequence behaviour going to stay the same? We had set the session cache before (implicilty), but now we are doing it explicitly. I'm wondering why the sequence values needed to change. I saw this is picked up as a CI diff too.

Yeah turns out I needed to run the rewrite again after making it explicit. The ./dev test pkg/sql/logictest/tests/local --rewrite takes >20 minutes so a bit challenging to be attentive.

pkg/sql/schemachanger/testdata/end_to_end/add_column_generated/add_column_generated.side_effects line 497 at r4 (raw file):

upsert descriptor #107
  ...
         ownerTableId: 106

Might need to pair on this I don't really understand this side-effect.

spilchen

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bghal and @rafiss)

pkg/sql/logictest/testdata/logic_test/alter_sequence line 42 at r3 (raw file):

Previously, bghal wrote…

Yeah turns out I needed to run the rewrite again after making it explicit. The ./dev test pkg/sql/logictest/tests/local --rewrite takes >20 minutes so a bit challenging to be attentive.

I understand. If you know what logic test to rewrite, you can do so individually rather than running the entire suite. Something like this should work:

dev testlogic base --ignore-cache --files=alter_sequence --config=local --rewrite

pkg/sql/schemachanger/testdata/end_to_end/add_column_generated/add_column_generated.side_effects line 497 at r4 (raw file):

Previously, bghal wrote…

Might need to pair on this I don't really understand this side-effect.

I think this is just showing context for the diff. The only actual change in the descriptor is the version field, which behaves the same way it did before your change. The line you're seeing is just a preceding unchanged line. I'm guessing the pretty-print order shifted when you renamed cacheSize to sessionCacheSize.

The session-level cache was implemented first and shared node-level cache was added in cockroachdb#118546. The better behaving node-level cache is not the default option. Changes the default for `CACHE` option to set node-level cache. A new option to allows the session-level cache to be set. Epic: CRDB-42587 Fixes: cockroachdb#131531 Release note (sql change): Changed the basic sequence cache option to set cache at the node-level. The PER SESSION NODE sequence option (syntax is is [ PER SESSION CACHE # ]) provides the previous behavior to set session-level cache.

bghal

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @spilchen)

pkg/sql/catalog/descpb/structured.go line 301 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

my bad -- i meant to add this comment on your first PR.

Done.

pkg/sql/logictest/testdata/logic_test/alter_sequence line 42 at r3 (raw file):

Previously, spilchen wrote…

I understand. If you know what logic test to rewrite, you can do so individually rather than running the entire suite. Something like this should work:
dev testlogic base --ignore-cache --files=alter_sequence --config=local --rewrite

Done.

pkg/sql/logictest/testdata/logic_test/datetime line 2102 at r3 (raw file):

Previously, spilchen wrote…

Was this change intentional? It doesn't seem related to the sequence change. And I saw that it's causing diffs in the CI run.

Done.

a discussion (no related file):
Unclear on the failed teamcity test? I ran ./dev test //pkg/sql:sql_test -f=TestShowCreateSequence locally.

spilchen

Reviewed 2 of 18 files at r3, 15 of 16 files at r4, 9 of 9 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @bghal, @michae2, and @rafiss)

a discussion (no related file):

Previously, bghal wrote…

Unclear on the failed teamcity test? I ran ./dev test //pkg/sql:sql_test -f=TestShowCreateSequence locally.

I think it’s safe to ignore (the test doesn’t need to be clean to merge to master). It looks like a timeout and it's running under stress with race enabled. If you want to try it locally, you can use:

dev test //pkg/sql:sql_test -f=TestShowCreateSequence --race --stress

ooo got other review

bghal · 2025-06-20T13:10:40Z

bors r+

craig · 2025-06-20T14:33:18Z

Build succeeded:

bghal requested a review from a team as a code owner June 13, 2025 16:28

bghal requested a review from a team June 13, 2025 16:28

bghal marked this pull request as draft June 13, 2025 16:28

bghal force-pushed the default-node-cache branch 2 times, most recently from a17b235 to 98b007a Compare June 13, 2025 17:03

rafiss approved these changes Jun 13, 2025

View reviewed changes

rafiss previously requested changes Jun 13, 2025

View reviewed changes

bghal force-pushed the default-node-cache branch 4 times, most recently from 501c865 to 707cd79 Compare June 16, 2025 17:57

bghal requested a review from rafiss June 16, 2025 18:48

bghal force-pushed the default-node-cache branch from 707cd79 to d3df1c7 Compare June 16, 2025 20:43

spilchen reviewed Jun 17, 2025

View reviewed changes

bghal force-pushed the default-node-cache branch from d3df1c7 to e7cf1de Compare June 17, 2025 15:51

bghal force-pushed the default-node-cache branch 6 times, most recently from ebe1ca2 to c640827 Compare June 18, 2025 14:44

bghal commented Jun 18, 2025

View reviewed changes

spilchen reviewed Jun 18, 2025

View reviewed changes

bghal force-pushed the default-node-cache branch from c640827 to f41dde4 Compare June 18, 2025 19:44

bghal commented Jun 18, 2025

View reviewed changes

bghal marked this pull request as ready for review June 18, 2025 20:56

bghal requested a review from a team as a code owner June 18, 2025 20:56

bghal requested review from michae2 and removed request for a team June 18, 2025 20:56

spilchen approved these changes Jun 19, 2025

View reviewed changes

craig bot merged commit 60cd2b1 into cockroachdb:master Jun 20, 2025
21 of 22 checks passed

celeste-cockroachdb bot added the target-release-25.3.0 label Jun 20, 2025

celeste-cockroachdb bot added v25.3.0-prerelease and removed target-release-25.3.0 labels Jul 2, 2025

rmloveland mentioned this pull request Oct 28, 2025

Update docs on {node,sequence}-level caching cockroachdb/docs#20829

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

sql: default to node-level cache for sequences #148290

sql: default to node-level cache for sequences #148290

Uh oh!

bghal commented Jun 13, 2025

Uh oh!

cockroach-teamcity commented Jun 13, 2025

Uh oh!

rafiss left a comment

Uh oh!

rafiss left a comment

Uh oh!

spilchen left a comment

Uh oh!

blathers-crl bot commented Jun 17, 2025

Uh oh!

bghal left a comment

Uh oh!

spilchen left a comment

Uh oh!

bghal left a comment

Uh oh!

spilchen left a comment

Uh oh!

bghal commented Jun 20, 2025

Uh oh!

craig bot commented Jun 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sql: default to node-level cache for sequences #148290

sql: default to node-level cache for sequences #148290

Uh oh!

Conversation

bghal commented Jun 13, 2025

Uh oh!

cockroach-teamcity commented Jun 13, 2025

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

spilchen left a comment

Choose a reason for hiding this comment

Uh oh!

blathers-crl bot commented Jun 17, 2025

Uh oh!

bghal left a comment

Choose a reason for hiding this comment

Uh oh!

spilchen left a comment

Choose a reason for hiding this comment

Uh oh!

bghal left a comment

Choose a reason for hiding this comment

Uh oh!

spilchen left a comment

Choose a reason for hiding this comment

Uh oh!

bghal commented Jun 20, 2025

Uh oh!

craig bot commented Jun 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants