kv: give savepoints distinct start and end sequence numbers

[nvanbenschoten]

This commit increments a transaction's write sequence number on savepoint creation and rollback. This ensures that savepoints have distinct start and end sequence numbers, which is necessary distinguish between all operations (writes and locking reads) that happened before the savepoint creation, those that happened within the savepoint, and those that happened after the savepoint rollback.

This avoids the problem described in the removed TODO. That hypothesized problem is real. Without this change, #121088 runs into trouble with the following sequence of operations:

create table kv (k int primary key, v int);
insert into kv values (1, 2);

begin isolation level read committed;
insert into kv values (2, 2);
savepoint s1;
insert into kv values (3, 2);
rollback to s1;
select * from kv where k = 1 for update;
commit;

ERROR: internal error: QueryIntent request for lock at sequence number 2 but sequence number is ignored [{2 2}]

Epic: None
Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

This is failing in CI for the same reason that #111424 was failing in CI — the enforce_home_region_follower_reads_enabled feature (ab)uses savepoints to run multiple attempts in different regions with different AS OF SYSTEM TIME timestamps. Now that we advance the write sequence number of savepoint rollback, transactions are not allowed to call SetFixedTimestamp after a savepoint rollback. This feature is going to be removed in #113765.

EDIT: I addressed this by only incrementing the write seq number on savepoint creation and rollback if the transaction has acquired locks.

[michae2]

This feature is going to be removed in #113765.

I will try to get this in this week.

[miraradeva]

I'd stress kvnemesis to make sure the validation still works, and that the various MVCC handling of ignored seq nums is ok.

General question (that I already asked @arulajmani some time ago): previously, we weren't discarding acquired locks upon savepoint rollback. That's still the case after the pipelining changes, right? So if we rolled back a lock promotion, that's not really a lock demotion because the exclusive lock is kept even after the savepoint rollback.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani and @nvanbenschoten)

---

pkg/kv/kvclient/kvcoord/txn_coord_sender_savepoints.go line 91 at r1 (raw file):

	// us to distinguish between all operations (writes and locking reads) that
	// happened before the savepoint and those that happened after.
	if tc.interceptorAlloc.txnPipeliner.hasAcquiredLocks() {

Does this mean that the savepoint will be assigned a seq num only if the transaction acquired locks before the savepoint was created? It makes sense but it would be nice if it's consistently assigned a seq num regardless of the state of the transaction. If we do that, then, upon rollback, we'd have to add the savepoint seq num to IgnoredSeqNumRange even if there are no actual writes/locks to ignore. Maybe that's not worth doing just for the sake of consistency.

[arulajmani]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/kv/kvclient/kvcoord/txn_coord_sender_savepoints.go line 91 at r1 (raw file):

	// us to distinguish between all operations (writes and locking reads) that
	// happened before the savepoint and those that happened after.
	if tc.interceptorAlloc.txnPipeliner.hasAcquiredLocks() {

nit: should we add a TODO to remove this conditional once #113765 is addressed for the benefit of future authors who come across this code?

[nvanbenschoten]

previously, we weren't discarding acquired locks upon savepoint rollback. That's still the case after the pipelining changes, right? So if we rolled back a lock promotion, that's not really a lock demotion because the exclusive lock is kept even after the savepoint rollback.

Yes, that's still the case after pipelining changes. We don't eagerly roll back locks on savepoint rollback. However, we do rollback locks after savepoint rollback if another locking request is sent to the same key. Eagerly rolling back locks on savepoint rollback is tracked in #94731.

I'd stress kvnemesis to make sure the validation still works

Done. I don't see any kvnemesis validation failures, but I did see a timeout which I believe is a dup of #121426.

TFTRs!

bors r=miraradeva,arulajmani

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

---

pkg/kv/kvclient/kvcoord/txn_coord_sender_savepoints.go line 91 at r1 (raw file):

Previously, miraradeva (Mira Radeva) wrote…

Does this mean that the savepoint will be assigned a seq num only if the transaction acquired locks before the savepoint was created? It makes sense but it would be nice if it's consistently assigned a seq num regardless of the state of the transaction. If we do that, then, upon rollback, we'd have to add the savepoint seq num to IgnoredSeqNumRange even if there are no actual writes/locks to ignore. Maybe that's not worth doing just for the sake of consistency.

I agree that this is somewhat inconsistent. I had originally written this to increment the write sequence unconditionally, but this is what ran into trouble with #113765. Making this conditional based on whether the transaction has ever acquired a lock (even before the savepoint) is enough to avoid issues with #113765 and serves as a bit of an optimization without requiring us to keep track of extra state.

Longer term, once #113765 is addressed, I'd like to remove this conditional and push the stepWriteSeqLocked back into the txnSeqNumAllocator.createSavepointLocked and txnSeqNumAllocator.rollbackToSavepointLocked savepoints. I added a pair of TODOs.

---

pkg/kv/kvclient/kvcoord/txn_coord_sender_savepoints.go line 91 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: should we add a TODO to remove this conditional once #113765 is addressed for the benefit of future authors who come across this code?

Yep! That's exactly what I'd like to do. I added a pair of TODOs.

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)
• linux_amd64_fips_build
• macos_amd64_build
• examples_orms
• linux_amd64_build
• check_generated_code
• macos_arm64_build
• windows_build
• docker_image_amd64
• linux_arm64_build