sql: implement sequence caching and cached sequence serial normalization

[jayshrivastava]

sql: implement sequence caching

Previously, incrementing sequences at a high throughput
would result in many distributed writes to the KV layer
due to MVCC. This has caused garbage collection problems
in the past. This would occur in situations such as
bulk importing data while using the sequence number as an
id for each new row being added.

This change allows clients to cache sequence numbers in their local
session data. When the cache is empty, the sequence will be
incremented once by the cache size * increment amount, which are
both sequence options. Then, all the intermediate values will be
cached locally on a node to be given out whenever the sequence is
incremented.

To accommodate schema changes, cached sequences values will be
invalidated when new descriptor versions are seen by the cache.
This invalidation can occur when old versions are seen as well
to accommodate schema change rollbacks.

Release note (sql change): Using the CACHE sequence option no longer
results in an "unimplemented" error. The CACHE option is now fully
implemented and will allow nodes to cache sequence numbers. A cache
size of 1 means that there is no cache, and cache sizes of less than 1
are not valid.

sql: create benchmark for concurrent sequence increments

Previously, there was no benchmark that tested the performance
of concurrent increments to sequences. There was also no
benchmark which compared sequence performance based on
different cache sizes. This change updates a benchmark to measure
performance based on the above criteria.

Release note: None

sql: add serial normalization setting for cached sequences

Closes: #51259

Release note (sql change): The serial_normalization session
variable can now be set to the value sql_sequence_cached.
Cached sequences will allow nodes to cache 256 sequence numbers
locally. The underlying sequence will only be incremened once (by
256 increments) when the cache is empty. Using sql_sequence_cached
will result in better performance than sql_sequence because the
former will perform fewer distributed calls to increment sequences.
However, cached seqences may contribute to large gaps between
sequence numbers if cached values are lost due to errors or
node outages.

sql: make cached sequences bounds-related semantics match pg semantics

Previously, cached sequences did not obey pg semantics
with respect to exceeding bounds. For example, if there
were a sequence with a cache size of 5 and max value of 2,
calling nextval(...) would immediately cause an error due
to exceeded bounds. According to postgres, nextval(...)
should return 1, then 2, then finally return an error due
to the max value of 2 being reached. This change alters
sequence caching behavior to match the above semantics.

Additionally, this change now makes it possible for sequences
to exceed the bounds set by MAXVALUE and MINVALUE. This is
because calling nextval(...) should be as fast as possible,
and the fastest way to do this is to let nextval(...) always
succeed on incrementing a sequence. Despite this, a user
will never see any values that exceed a sequence's bounds.
To make a sequence incrementable again after exceeding its
bounds, there are two options:

1. The user changes the sequence's value by calling setval(...).
2. The user performs a schema change to alter the sequences MinValue
   or MaxValue. If the value of a sequence exceeds its bounds,
   it must be restored to the original MinValue or MaxValue in
   the same transaction as the schema change.
   This change adds code to handle case 2 above.

Release note: None

Benchmark

Using make bench PKG='./pkg/sql' BENCHES='BenchmarkSequenceIncrement' TESTFLAGS="--count=5 --benchmem" |& tee c{size} .txt:

Caching 256 values

name                     old time/op    new time/op    delta
SequenceIncrement/P-1-8     313µs ± 1%     107µs ± 1%  -65.85%  (p=0.008 n=5+5)
SequenceIncrement/P-2-8     327µs ± 2%      82µs ± 2%  -75.04%  (p=0.008 n=5+5)
SequenceIncrement/P-4-8     347µs ± 1%      70µs ± 2%  -79.89%  (p=0.008 n=5+5)
SequenceIncrement/P-8-8     389µs ± 2%      65µs ± 3%  -83.40%  (p=0.008 n=5+5)

name                     old alloc/op   new alloc/op   delta
SequenceIncrement/P-1-8     130kB ± 1%      57kB ± 1%  -55.76%  (p=0.008 n=5+5)
SequenceIncrement/P-2-8     131kB ± 0%      49kB ± 1%  -62.56%  (p=0.008 n=5+5)
SequenceIncrement/P-4-8     132kB ± 0%      46kB ± 0%  -65.26%  (p=0.008 n=5+5)
SequenceIncrement/P-8-8     134kB ± 0%      44kB ± 1%  -67.09%  (p=0.008 n=5+5)

name                     old allocs/op  new allocs/op  delta
SequenceIncrement/P-1-8       807 ± 0%       406 ± 0%  -49.75%  (p=0.000 n=5+4)
SequenceIncrement/P-2-8       812 ± 1%       363 ± 0%  -55.32%  (p=0.008 n=5+5)
SequenceIncrement/P-4-8       826 ± 1%       346 ± 0%  -58.10%  (p=0.008 n=5+5)
SequenceIncrement/P-8-8       863 ± 1%       341 ± 0%  -60.44%  (p=0.008 n=5+5)

For the other cache sizes I tested, the performance improvement is virtually the same as above.
c1.txt
c32.txt
c64.txt
c128.txt
c256.txt
c512.txt
c1024.txt

[cockroach-teamcity]

This change is 

[ajwerner]

Reviewed 2 of 8 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @jayshrivastava)

---

pkg/sql/exec_util.go, line 745 at r1 (raw file):

	VersionUpgradeHook func(ctx context.Context, to roachpb.Version) error

	SequenceCache *SequenceCache

give this a comment to indicate that it is node-level cache of sequence values.

---

pkg/sql/sequence.go, line 43 at r1 (raw file):

// are stored using sequence descriptor IDs as keys. The cache should only
// be accessed using the provided API.
type SequenceCache struct {

let's iterate on this a bit. I think it's worthwhile to write some microbenchmarks to help guide the optimizations here.

I think it's a bad idea to hold the mutex during the entire operation for a given cache value. Instead let's utilize a singleflight.Group for initializing and updating the cache value.

Another thing we will likely eventually want to do is to switch to a syncutil.IntMap but let's leave that off for the first optimization path.

---

pkg/sql/sequence.go, line 64 at r1 (raw file):

		currentIndex int
		// Stores values to be given out.
		values *[]int64

rather than using a slice, I think we can get away with constant memory usage. What if we had a curVal, increment, maxVal or something like that.

---

pkg/sql/sequence.go, line 186 at r1 (raw file):

		}

		if seqOpts.CacheSize == 1 {

all previously existing sequences will have a value of 0. How do we deal with that? Can you write a test that injects such a descriptor and ensure that it behaves properly?

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• When CI has completed, please ensure no errors have appeared.

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

[ajwerner]

Seems like you need a rebase. Sorry I took so darn long to get back to this. It's very close.

Reviewed 1 of 12 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @jayshrivastava)

---

pkg/sql/exec_util.go, line 783 at r5 (raw file):

	// VersionUpgradeHook is called after validating a `SET CLUSTER SETTING
	// version` but before executing it. It structuredcan carry out arbitrary migrations

detritus?

---

pkg/sql/exec_util.go, line 2250 at r5 (raw file):

}

// initSequenceCache

Missing a comment.

---

pkg/sql/sequence.go, line 105 at r5 (raw file):

		val = int64(rowid)
	} else {
		fetchNextValues := func() (int64, int64, int64, error) {

Consider pulling everything in this else into a helper.

---

pkg/sql/sequence.go, line 224 at r5 (raw file):

Quoted 4 lines of code…

	// If there are cached values, changing the value of the sequence should invalidate the cache.
	// For simplicity, the cache invalidates implicitly when it sees new descriptor versions. Thus,
	// a schema change is triggered here to make sure the cache gets invalidated.
	mutableDescriptor, err := resolver.ResolveMutableExistingTableObject(ctx, p, seqName, true, tree.ResolveRequireSequenceDesc)

I don't think we need to do this. Simpler not to. Sorry I sent you astray here.

ALTER SEQUENCE will not immediately affect nextval results in backends, other than the current one, that have preallocated (cached) sequence values. They will use up all cached values prior to noticing the changed sequence generation parameters. The current backend will be affected immediately.

https://www.postgresql.org/docs/12/sql-altersequence.html

[jayshrivastava]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @jayshrivastava)

---

pkg/sql/exec_util.go, line 783 at r5 (raw file):

Previously, ajwerner wrote…

detritus?

Yes.

---

pkg/sql/exec_util.go, line 2250 at r5 (raw file):

Previously, ajwerner wrote…

Missing a comment.

Done.

---

pkg/sql/sequence.go, line 224 at r5 (raw file):

Previously, ajwerner wrote…

	// If there are cached values, changing the value of the sequence should invalidate the cache.
	// For simplicity, the cache invalidates implicitly when it sees new descriptor versions. Thus,
	// a schema change is triggered here to make sure the cache gets invalidated.
	mutableDescriptor, err := resolver.ResolveMutableExistingTableObject(ctx, p, seqName, true, tree.ResolveRequireSequenceDesc)

I don't think we need to do this. Simpler not to. Sorry I sent you astray here.

ALTER SEQUENCE will not immediately affect nextval results in backends, other than the current one, that have preallocated (cached) sequence values. They will use up all cached values prior to noticing the changed sequence generation parameters. The current backend will be affected immediately.

https://www.postgresql.org/docs/12/sql-altersequence.html

Yup it's simpler not to.

[ajwerner]

My one remaining thing on this is do we need a cluster version to gate this in the mixed version state. This week is the last week before stability period. I don't have much doubt that this is going to make it across the line. Thanks for all the revisions!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @jayshrivastava)

---

pkg/sql/sequence.go, line 171 at r19 (raw file):

Quoted 4 lines of code…

			if endValue < seqOpts.MinValue {
				if endValue-seqOpts.Increment*cacheSize <= seqOpts.MinValue {
					return 0, 0, 0, boundsExceededError(descriptor)

What is this case dealing with?

---

pkg/sql/sequence.go, line 176 at r19 (raw file):

					return 0, 0, 0, boundsExceededError(descriptor)
				}
				return endValue - seqOpts.Increment*(cacheSize-1), seqOpts.Increment, int64(math.Abs(float64(seqOpts.MinValue-(endValue-seqOpts.Increment*cacheSize)))) / seqOpts.Increment * -1, nil

It feels like there's some repetition that can be dealt with here.

[jayshrivastava]

Having a cluster in a mixed version state is fine.

Caching occurs in a session, so the feature is isolated from other nodes. The new code handles all cache sizes, including 0. The old code will ignore a sequence's cache size when incrementing a sequence. In the case of creating/altering a sequence's cache size, the old code will just throw an error saying that caching is not supported.

One tricky thing is that a session running the new code may increment a sequence beyond its MAXVALUE several times. A user talking to another node may try to increase the MAXVALUE so that the sequence can be incremented again. If the other node is running the new version, any value greater than the old max value will make the sequence incrementable again. This is because the new alter sequence code will restore the value of the sequence to the old MAXVALUE before executing the schema change. If the node has an old version, this will not happen. To make the sequence incrementable again, a user will have to set a max value greater than the current value of the sequence or they will have to use setval() to set the value of the sequence to the old MAXVALUE first. Overall, I think this is okay since sequences are non-transactional and we are allowed to skip numbers.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @arulajmani)

---

pkg/sql/sequence.go, line 171 at r19 (raw file):

Previously, ajwerner wrote…

			if endValue < seqOpts.MinValue {
				if endValue-seqOpts.Increment*cacheSize <= seqOpts.MinValue {
					return 0, 0, 0, boundsExceededError(descriptor)

What is this case dealing with?

Just repetition of the above case when seqOpts.Increment < 0. I just updated the code to not have this repetition.

[ajwerner]

Get rid of the float math.Abs stuff and this is ready for bors!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @jayshrivastava)

---

pkg/sql/sequence.go, line 148 at r24 (raw file):

	cacheSize := seqOpts.EffectiveCacheSize()

	fetchNextValues := func() (currentValue int64, incrementAmount int64, sizeOfCache int64, err error) {

nit, you can group the return values: `(currentValue, incrementAmount, sizeOfCache int64, err error)

---

pkg/sql/sequence.go, line 164 at r24 (raw file):

		if endValue > seqOpts.MaxValue || endValue < seqOpts.MinValue {
			// If the sequence exceeded its bounds prior to the increment, then return an error.
			if seqOpts.Increment > 0 && endValue-seqOpts.Increment*cacheSize >= seqOpts.MaxValue || seqOpts.Increment < 0 && endValue-seqOpts.Increment*cacheSize <= seqOpts.MinValue {

nit: wrap this conditional.

---

pkg/sql/sequence.go, line 172 at r24 (raw file):

				limit = seqOpts.MinValue
			}
			return endValue - seqOpts.Increment*(cacheSize-1), seqOpts.Increment, int64(math.Abs(float64(limit-(endValue-seqOpts.Increment*cacheSize))) / math.Abs(float64(seqOpts.Increment))), nil

Also, please assign these variables for readability.

this math.Abs stuff seems like it'll get screwed up as these limit values get to be larger than can be represented in a float64. Just write an `

abs := func(i int64) int64 {
    if i < 0 { 
        return -i
    } 
    return i
}

[ajwerner]

thanks again for keeping after this. This is going to be a great addition!

bors r+

Reviewed 3 of 13 files at r20, 4 of 5 files at r22, 1 of 3 files at r23, 1 of 1 files at r25.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @arulajmani)

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[mosquito2333]

Why should out-of-range endValue be stored in the cache?

[sandr8]

Cached sequences will allow nodes to cache 256 sequence numbers locally

This is not the behavior I have observed. At least on v21.2.10, the sequence seems to be cached per-session (as compared to per-node). This means that -unless sessions are very long lived- a lot of identifiers will be wasted and not a lot of improvement in terms of contention will be achieved.

[rafiss]

You're right - this cache is implemented per-session. The sequence docs are a little more clear, but I agree that this release note is misleading, and the docs could have more clarity.

One thing to note is that this was implemented to be compatible with sequence caching in PostgreSQL, which is described here in the "Notes" section: https://www.postgresql.org/docs/current/sql-createsequence.html

And I also agree with you that the sequence cache size should be chosen based on how frequently the sequence is used along with how long the session lasts.

[ale-dd]

@rafiss I agree. But I would love the addition of a keyword to enable the per-node caching, which would be very advantageous.

Some syntax like [ [ PER NODE ] CACHE # ]...

[rafiss]

I've filed #89338 to track that request.