storage: release replicated locks during intent resolution #110480
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1, 1 of 1 files at r2, 5 of 5 files at r3, 2 of 2 files at r4, 7 of 7 files at r5, 1 of 1 files at r6, 3 of 3 files at r7, 5 of 5 files at r8, 9 of 9 files at r9, 4 of 4 files at r10, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)
pkg/storage/mvcc.go line 5389 at r10 (raw file):
writer Writer, ms *enginepb.MVCCStats, intent roachpb.LockUpdate,
nit: consider renaming this to something other than intent in this patch itself. Maybe the "resolution", from a similar TODO you had in the previous PR?
pkg/storage/testdata/mvcc_histories/replicated_locks line 559 at r10 (raw file):
lock (Replicated): "k3"/Exclusive -> txn={id=00000001 key=/Min iso=Serializable pri=0.00000000 epo=1 ts=10.000000000,0 min=0,0 seq=1} ts=0,0 del=false klen=0 vlen=0 mergeTs=<nil> txnDidNotUpdateMeta=false # Pending resolution of the lock in the new epoch without a savepoint rollback
Should we add a test for resolution from a prior epoch being a no-op as well?
There was a problem hiding this comment.
change to mvcc.go looks good to me. I didn't look at the tests since @arulajmani is also reviewing.
Reviewed 1 of 4 files at r10, all commit messages.
Reviewable status:
…poch Spotted while working on cockroachdb#110480. Epic: None Release note: None
This commit adds the results of resolve_intent and resolve_intent_range to the TestMVCCHistories output. This includes the number of keys resolved and resume span information, if any. Epic: None Release note: None
Fixes cockroachdb#109648. Informs cockroachdb#100193. This commit adds support for releasing replicated locks during point and ranged intent resolution, if any are found. Replicated locks are released if the resolving transaction is finalized or if resolving transaction is pending and the lock has been rolled back through an epoch bump or savepoint rollback. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/releaseAllReplLocks
branch
from
1a3652f to
a3b9f67
Compare
There was a problem hiding this comment.
TFTRs!
I updated the PR to address the TODO about lock pagination across multiple locks on the same locked key, ensuring that all locks on a key are resolved before returning a resume span. To ensure this was tested sufficiently, I added a prep commit which adds intent resolution results (num keys, num bytes, resume span) to TestMVCCHistories output.
This PR has two LGTMs, but I'll hold off on merging until @arulajmani is able to take one last look at these changes.
Reviewable status:
pkg/storage/mvcc.go line 5389 at r10 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: consider renaming this to something other than intent in this patch itself. Maybe the "resolution", from a similar TODO you had in the previous PR?
Done. I ended up going with "update", because that's the naming we use in cmd_resolve_intent.go and cmd_resolve_intent_range.go.
pkg/storage/testdata/mvcc_histories/replicated_locks line 559 at r10 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Should we add a test for resolution from a prior epoch being a no-op as well?
Done. This also revealed a bug where resolution from a prior epoch with ignored sequence numbers was able to release a lock from a newer epoch. Fixed and added a test for this as well.
There was a problem hiding this comment.
Reviewed 52 of 52 files at r11, 30 of 30 files at r12, all commit messages.
Reviewable status:
pkg/storage/testdata/mvcc_histories/replicated_locks line 559 at r10 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Done. This also revealed a bug where resolution from a prior epoch with ignored sequence numbers was able to release a lock from a newer epoch. Fixed and added a test for this as well.
Nice! I think this is the same one we saw together way back in April, right?
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/storage/testdata/mvcc_histories/replicated_locks line 559 at r10 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Nice! I think this is the same one we saw together way back in April, right?
Yep, I think so! It's an easy mistake to make.
|
Build failed: |
bors r+ |
|
Build succeeded: |
Fixes #109648.
Informs #100193.
This commit adds support for releasing replicated locks during point and ranged intent resolution, if any are found. Replicated locks are released if the resolving transaction is finalized or if resolving transaction is pending and the lock has been rolled back through an epoch bump or savepoint rollback.
Release note: None