kv: give savepoints distinct start and end sequence numbers#121458
kv: give savepoints distinct start and end sequence numbers#121458craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
This is failing in CI for the same reason that #111424 was failing in CI — the EDIT: I addressed this by only incrementing the write seq number on savepoint creation and rollback if the transaction has acquired locks. |
be5afa1 to
d109830
Compare
I will try to get this in this week. |
miraradeva
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status:
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?
d109830 to
00b7963
Compare
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, cockroachdb#121088 runs into trouble with the following sequence of operations: ```sql 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
00b7963 to
d6f902e
Compare
nvb
left a comment
There was a problem hiding this comment.
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:
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
IgnoredSeqNumRangeeven 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.
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:
Epic: None
Release note: None