concurrency: misc cleanup related to multiple transactions holding locks on a single key #109177
Conversation
|
This change is |
arulajmani
force-pushed
the
sl-multiple-locks-follow-up
branch
from
e080535 to
a8bee16
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 8 of 8 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)
-- commits line 43 at r4:
Did you mean to include this in the commit message?
pkg/kv/kvserver/concurrency/concurrency_control.go line 90 at r2 (raw file):
// | | [ lockTable ] | | | // | | [ key1 ] -------------+-----------------+ | ^ | // | | [ key2 ] / keyLocks: | lockWaitQueue: |----<---<---<----+
Looks like the spacing on these diagrams got messed up.
pkg/kv/kvserver/concurrency/lock_table.go line 1931 at r1 (raw file):
// part of the specified transaction. // REQUIRES: l.mu is locked. func (l *lockState) releaseWritersFromTxn(txn *enginepb.TxnMeta) {
I'm not sure about these changes to pass enginepb.TxnMeta by value in more places. The type is still quite large (~120 bytes) and will only grow larger over time, so I think we'll still want to prefer passing it by reference through functions, even if we store it by value in the data-structure.
My suggestion would be to revert the change to all of the function/method signatures but keep the change to the txnLock struct.
pkg/kv/kvserver/concurrency/lock_table.go line 264 at r4 (raw file):
// lockAddMaxLocksCheckInterval, locks will be cleared. // // [1] Simply put, the number of keyLocks objects in the request's lock table
Why specifically reference "the request's lock table snapshot" instead of just "the lockTable btree"?
pkg/kv/kvserver/concurrency/lock_table.go line 3668 at r4 (raw file):
} // checkMaxNumKeysLockedAndTryClear checks if the request is tracking more lock
Should we remove "Num" to mirror the field name directly?
Previously, we would store a reference to the txn meta in the txnLock struct. This isn't required, as this field must now always be set. This patch switches over to storing the txnMeta struct directly. Doing so means we don't hold on to the entire `roachpb.LockAcquisition` or `roachpb.LockUpdate` struct in a few places. Epic: none Release note: None
arulajmani
force-pushed
the
sl-multiple-locks-follow-up
branch
from
a8bee16 to
8be1ea0
Compare
There was a problem hiding this comment.
TFTR, RFAL!
Reviewable status:
pkg/kv/kvserver/concurrency/concurrency_control.go line 90 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Looks like the spacing on these diagrams got messed up.
Fixed.
pkg/kv/kvserver/concurrency/lock_table.go line 1931 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I'm not sure about these changes to pass
enginepb.TxnMetaby value in more places. The type is still quite large (~120 bytes) and will only grow larger over time, so I think we'll still want to prefer passing it by reference through functions, even if we store it by value in the data-structure.My suggestion would be to revert the change to all of the function/method signatures but keep the change to the
txnLockstruct.
That makes sense; took your suggestion and reverted all changes to function/method signatures.
pkg/kv/kvserver/concurrency/lock_table.go line 264 at r4 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Why specifically reference "the request's lock table snapshot" instead of just "the lockTable btree"?
My bad, I thought I was working with a lockTableGuardImpl instead of a lockTableImpl; fixed.
pkg/kv/kvserver/concurrency/lock_table.go line 3668 at r4 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Should we remove "Num" to mirror the field name directly?
Fixed.
There was a problem hiding this comment.
Reviewed 9 of 9 files at r5, 8 of 8 files at r6, 1 of 1 files at r7, 3 of 3 files at r8, all commit messages.
Reviewable status:
pkg/kv/kvserver/concurrency/concurrency_control.go line 90 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Fixed.
Looks like there's also the same issue in the diagram below:
// lockTable holds a collection of locks
The new name is more apt for the current structure. Epic: none Release note: None
More descriptive name. While here, also burn down a TODO. Epic: none Release note: None
Ditto for minKeys. These new names better reflect what these limits were being used for. Epic: none Release note: None
arulajmani
force-pushed
the
sl-multiple-locks-follow-up
branch
from
8be1ea0 to
8b66dac
Compare
There was a problem hiding this comment.
Fixed the formatting.
bors r=nvanbenschoten
Reviewable status:
|
Build succeeded: |
Cleanup following-on from #109049. See individual commits for details.