sql: add a way for sequence caching to be per-node

[jasminejsun]

Previously, the sequence CACHE implementation caches IDs per-session. This can be wasteful since a single increment will allocate extra space within the sequences for each session. Depending on the use cases, a node-level cache would be more effective and minimize the number of wasted sequence values. In order to transition from session-level to node-level, we will create a shared cache for sequence entry information. A new cache entry object will embed the existing SequenceCacheEntry struct that session-level sequence caching currently utilizes.

Epic: CRDB-20209
Fixes: #89338

Release note (sql change): Added option for node-level sequence caching. All the sessions on the node can share the same cache, which can be concurrently accessed. The serial_normalization session variable can now be set to the value sql_sequence_cached_node. If this value is set, the cluster setting sql.defaults.serial_sequences_cache_size can be used to control the number of values to cache in a node with a default of 256. The PER NODE CACHE sequence option (syntax is is [ [ PER NODE ] CACHE # ]) is now fully implemented and will allow nodes to cache sequence numbers. A cache size of 1 means there is no cache, and cache sizes of less than 1 are not valid.

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser.

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

[cockroach-teamcity]

This change is 

[rafiss]

great work! just had a few comments

the first commit looks good to me. it might be worth pulling that into a separate PR so we can get it merged sooner, but that's up to you.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jasminejsun and @rytaft)

---

-- commits line 9 at r2:
super nit: to make it easier for the docs team, we can just put all of the release note text into one of the commits. (each release note generated a DOC issue)

---

pkg/sql/planner.go line 611 at r2 (raw file):

}

// GetSequenceCacheNode returns the node-level sequence cache.

super nit: comment has the wrong name for the function

i think GetSequenceCacheNode is the correct name to use, since this function is not initializing anything

---

pkg/sql/sequence.go line 220 at r2 (raw file):

		}
	} else {
		if seqOpts.CacheSize == 1 && seqOpts.NodeCacheSize > 1 {

nit: i didn't follow this if-else condition that well. what does it mean for seqOpts.CacheSize to be 1 and seqOpts.NodeCacheSize to be greater than 1? maybe adding some comments on the if and else could help

---

pkg/sql/catalog/descpb/structured.go line 303 at r2 (raw file):

// sizes of 1.
func (opts *TableDescriptor_SequenceOpts) EffectiveCacheSize() int64 {
	if opts.CacheSize == 0 || opts.NodeCacheSize == 0 {

nit: "it will return 1 if the both fields are 0" so should this be && instead of ||?

---

pkg/sql/schemachanger/scexec/scmutationexec/sequence.go line 85 at r2 (raw file):

		tree.SeqOptStart:     {SetFunc: setIntValue(&sc.SequenceOpts.Start)},
		tree.SeqOptCache:     {SetFunc: setIntValue(&sc.SequenceOpts.CacheSize)},
		tree.SeqOptCacheNode: {SetFunc: setIntValue(&sc.SequenceOpts.NodeCacheSize)},

for both the new and old schema changer, i think we might want a check that you can only use per-node sequence caching is the cluster version is >= 24.1. the reason is that we use the cluster version to make sure all nodes have upgraded to the latest version, and finalized the upgrade. it could be problematic for someone to use per-node sequence caching if some nodes in the cluster are on 24.1, but others are still on 23.2 (which doesn't have any code that knows how to handle the per-node cache)

here's an example of a version gate in the new schema changer:

cockroach/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go

Lines 296 to 299 in 085e759

	if !b.EvalCtx().Settings.Version.IsActive(b, clusterversion.V23_2) {
	panic(scerrors.NotImplementedErrorf(n, "mixing ADD COLUMN, DROP COLUMN, "+
	"and ALTER PRIMARY KEY not supported before V23.2"))
	}

and here's an example in the old one:

cockroach/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go

Lines 276 to 281 in d5d7449

	// Until the appropriate version gate is hit, we still do not allow
	// dropping unique without index constraints.
	if !b.ClusterSettings().Version.IsActive(b, clusterversion.V23_1) {
	panic(scerrors.NotImplementedErrorf(nil, "dropping without"+
	"index constraints is not allowed."))
	}

cc @fqazi would you agree with adding version gates on this?

---

pkg/sql/sessiondatapb/sequence_cache_node.go line 24 at r2 (raw file):

// SequenceCacheNode stores sequence values at a per-node level
type SequenceCacheNode struct {
	cache map[uint32]*SequenceCacheNodeEntry

nit: could there be a comment describing what the key of this map is? (and also maybe explain why we can't use descpb.ID. there's a similar comment in sequence_cache.go, but it would be good to explain here too

cockroach/pkg/sql/sessiondatapb/sequence_cache.go

Lines 13 to 22 in 249b87e

	// SequenceCache stores sequence values that have already been created in KV
	// and are available to be given out as sequence numbers. Values for sequences
	// are keyed by the descpb.ID of each sequence. These IDs are represented as
	// uint32 to prevent an import cycle with the descpb package. The cache should
	// only be accessed using the provided API.
	//
	// The cache ensures that values are invalidated when new descriptor versions are seen. Note that
	// new descriptor versions may not monotonically increase. For example, the sequence schema
	// may be altered in a txn, so the cache sees a new version V and invalidates/repopulates itself. Then,
	// the txn may get rolled back, so the cache will see version V-1 and invalidate/repopulate itself again.

---

pkg/sql/sequence_test.go line 754 at r2 (raw file):

func TestCachedNodeSequence(t *testing.T) {
	defer leaktest.AfterTest(t)()

nit: let's also add defer log.Scope(t).Close(t). this will make the test logs go to a file.

---

pkg/sql/sequence_test.go line 866 at r2 (raw file):

				//   s1: 4,6,8,10
				// db:
				//  s1: 10

the comments here are really helpful, thanks!

---

pkg/sql/sequence_test.go line 1003 at r2 (raw file):

					var startChannel chan struct{}
					startChannel = make(chan struct{})
					cg.Go(func() error {

nit: it's preferred to use GoCtx. the ctxgroup.go file has further explanation

[jasminejsun]

pkg/sql/catalog/descpb/structured.go line 303 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: "it will return 1 if the both fields are 0" so should this be && instead of ||?

oops! i updated the comment. i dont think the cache size can be set to 0 (uninitialized) when using the PER NODE CACHE sequence option, unlike the CACHE option based on this comment: "Prior to #51259, sequence caching was unimplemented and cache sizes were left uninitialized (ie. to have a value of 0).". which is why i used || instead of && (its just a precautionary). the user can't set the value to 0 due to validation.

[fqazi]

Reviewed 12 of 13 files at r1, 13 of 13 files at r3, 2 of 14 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jasminejsun and @rafiss)

---

pkg/sql/sequence.go line 151 at r4 (raw file):

) (int64, error) {
	seqOpts := descriptor.GetSequenceOpts()
	if seqOpts.NodeCacheSize > 1 && !p.execCfg.Settings.Version.IsActive(ctx, clusterversion.V24_1) {

Lets put the gates during sequence creation / modification, we shouldn't be able to enter this state.

---

pkg/sql/schemachanger/scexec/scmutationexec/sequence.go line 85 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

for both the new and old schema changer, i think we might want a check that you can only use per-node sequence caching is the cluster version is >= 24.1. the reason is that we use the cluster version to make sure all nodes have upgraded to the latest version, and finalized the upgrade. it could be problematic for someone to use per-node sequence caching if some nodes in the cluster are on 24.1, but others are still on 23.2 (which doesn't have any code that knows how to handle the per-node cache)

here's an example of a version gate in the new schema changer:

cockroach/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go

Lines 296 to 299 in 085e759

	if !b.EvalCtx().Settings.Version.IsActive(b, clusterversion.V23_2) {
	panic(scerrors.NotImplementedErrorf(n, "mixing ADD COLUMN, DROP COLUMN, "+
	"and ALTER PRIMARY KEY not supported before V23.2"))
	}

and here's an example in the old one:

cockroach/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go

Lines 276 to 281 in d5d7449

	// Until the appropriate version gate is hit, we still do not allow
	// dropping unique without index constraints.
	if !b.ClusterSettings().Version.IsActive(b, clusterversion.V23_1) {
	panic(scerrors.NotImplementedErrorf(nil, "dropping without"+
	"index constraints is not allowed."))
	}

cc @fqazi would you agree with adding version gates on this?

Yeah we need version gates for both the new / old schema changer for this feature. We should also add a mixed version logic test to confirm they work correctly.

[annrpom]

nice job!! just added some drive-by comments

[annrpom]

thank you! lgtm

[rafiss]

this is looking close!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @jasminejsun)

---

-- commits line 9 at r8:
nit: commit messages need to have newlines entered manually, otherwise most text editors will show it on one gigantic line that's hard to read

---

pkg/sql/alter_sequence.go line 157 at r8 (raw file):

			}
		} else if option.Name == tree.SeqOptCacheNode {
			if !params.p.execCfg.Settings.Version.IsActive(params.ctx, clusterversion.V24_1) {

i may not follow this: don't we need to look at option.IntVal if the version is active?

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_sequence.go line 76 at r8 (raw file):

		if opt.Name == tree.SeqOptCacheNode && !b.EvalCtx().Settings.Version.IsActive(b, clusterversion.V24_1) {
			panic(scerrors.NotImplementedErrorf(n, "node-level sequence caching unsupported"+
				"before V24.1"))

does this code need to use the value of opt.IntVal if the v24.1 version is active?

---

pkg/sql/logictest/testdata/logic_test/mixed_version_sequence_per_node_cache line 1 at r8 (raw file):

# LogicTest: cockroach-go-testserver-23.1

nit: let's use cockroach-go-testserver-23.2 for this

---

pkg/sql/logictest/testdata/logic_test/serial line 400 at r2 (raw file):

# Verify that the sequence gets incremented to the default cache
# size of 256 for node-level sequence caching

just curious, why did this test get removed in the latest patch?

[jasminejsun]

pkg/sql/logictest/testdata/logic_test/serial line 400 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

just curious, why did this test get removed in the latest patch?

moved it to pkg/sql/logictest/testdata/logic_test/mixed_version_sequence_per_node_cache!

[jasminejsun]

pkg/sql/alter_sequence.go line 157 at r8 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i may not follow this: don't we need to look at option.IntVal if the version is active?

I noticed that the NodeCacheSize value was getting assigned on line 87. Example: here I altered the sequence NodeCacheSize from 10->20:

[jasminejsun]

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_sequence.go line 76 at r8 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

does this code need to use the value of opt.IntVal if the v24.1 version is active?

same as above comment! call to AssignSequenceOptions on line 90!

[fqazi]

Just some minor comments from me, but this is looking pretty solid now.

Reviewed 1 of 14 files at r4, 1 of 14 files at r5, 12 of 27 files at r6, 1 of 1 files at r7, 9 of 12 files at r9, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jasminejsun and @rafiss)

---

pkg/sql/sessiondatapb/sequence_cache_node.go line 28 at r9 (raw file):

// uint32 to prevent an import cycle with the descpb package.
type SequenceCacheNode struct {
	cache map[uint32]*SequenceCacheNodeEntry

Replace with catid.DescID

---

pkg/sql/sequence_test.go line 1046 at r9 (raw file):

				}

				time.Sleep(500 * time.Millisecond)

Why do you need a sleep before the commit?

[rafiss]

lgtm! great work

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom, @fqazi, and @jasminejsun)

[fqazi]

Reviewed 1 of 14 files at r4, 1 of 14 files at r5, 1 of 12 files at r9, 13 of 13 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @jasminejsun)

[jasminejsun]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[rickystewart]

Looks like this PR introduced a race build failure in pkg/sql/sessiondatapb/sequence_cache_node.go

pkg/sql/sessiondatapb/sequence_cache_node.go:47:8: sc.mu.RWMutex undefined (type syncutil.RWMutex has no field or method RWMutex)
pkg/sql/sessiondatapb/sequence_cache_node.go:49:8: sc.mu.RWMutex undefined (type syncutil.RWMutex has no field or method RWMutex)
pkg/sql/sessiondatapb/sequence_cache_node.go:52:9: sc.mu.RWMutex undefined (type syncutil.RWMutex has no field or method RWMutex)
pkg/sql/sessiondatapb/sequence_cache_node.go:53:15: sc.mu.RWMutex undefined (type syncutil.RWMutex has no field or method RWMutex)