concurrency: non-locking reads of RC txns ignore exclusive locks#106316
Conversation
|
This change is |
|
@nvanbenschoten feel free to hold off on the review until we've landed the other PRs; I just wanted to get this out before addressing comments on those ones. |
a613893 to
3a174e2
Compare
|
@nvanbenschoten this should be good for a look now! |
nvb
left a comment
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @arulajmani)
pkg/kv/kvserver/concurrency/testdata/lock_table/read_committed line 7 at r1 (raw file):
---- new-txn txn=txn2 ts=10,1 epoch=0 iso=read-committed
Want to call this file isolation_level and make this txn snapshot iso?
pkg/kv/kvserver/concurrency/testdata/lock_table/read_committed line 15 at r1 (raw file):
# ------------------------------------------------------------------------------ # Test that non-locking reads from RC transactions do not block on exclusive # exclusive locks held by other RC/Serializable transactions.
"exclusive exclusive"
pkg/kv/kvserver/concurrency/testdata/lock_table/read_committed line 88 at r1 (raw file):
num=1 lock: "a" holder: txn: 00000000-0000-0000-0000-000000000002 epoch: 0, ts: 10.000000000,1, info: repl
Should we include the isolation level in this output? Maybe at least for non-SSI lock holders?
pkg/kv/kvserver/concurrency/lock_table_test.go line 731 at r1 (raw file):
} func ScanIsoLevel(t *testing.T, d *datadriven.TestData) isolation.Level {
nit: don't split ScanLockStrength from scanLockStrength.
Non-locking reads issued by transactions that can tolerate write-skew do not block on exclusive locks after this patch. Resolves cockroachdb#94729 Release note: None
Now that we've got multiple isolation levels in the system, let's print the lock holder's isolation level when formatting it. The line count here is entirely test churn. Epic: none Release note: None
3a174e2 to
f3d2a83
Compare
arulajmani
left a comment
There was a problem hiding this comment.
TFTR!
bors r=nvanbenschoten
Reviewable status:
pkg/kv/kvserver/concurrency/lock_table_test.go line 731 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: don't split
ScanLockStrengthfromscanLockStrength.
Did you mean getStrength? If so, done.
pkg/kv/kvserver/concurrency/testdata/lock_table/read_committed line 7 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Want to call this file
isolation_leveland make this txnsnapshotiso?
Good call. I changed the file name and extended these tests to cover snapshot transactions as well. It was a bit more involved than changing the isolation level of this transaction, but the test coverage is much better now.
pkg/kv/kvserver/concurrency/testdata/lock_table/read_committed line 88 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Should we include the isolation level in this output? Maybe at least for non-SSI lock holders?
That's a good call. I ended up including isolation level information for all transactions, not just SI and RC; there's quite some test churn, but IMO it's trivial, so no point special casing. It's the second commit that I tacked on this PR.
|
Build succeeded: |
Non-locking reads issued by transactions that can tolerate write-skew
do not block on exclusive locks after this patch.
Resolves #94729
Release note: None