lock: track isolation level information in lock modes #100827
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)
pkg/kv/kvserver/concurrency/lock/locking.go line 72 at r1 (raw file):
return o.Strength == Update || o.Strength == Exclusive || o.Strength == Intent case Exclusive: if ExclusiveLocksBlockNonLockingReadsFromSerializableTransactions.Get(sv) &&
nit: move the cluster setting's atomic load to last in this short-circuiting conditional.
pkg/kv/kvserver/concurrency/lock/locking.go line 73 at r1 (raw file):
case Exclusive: if ExclusiveLocksBlockNonLockingReadsFromSerializableTransactions.Get(sv) && o.IsoLevel == isolation.Serializable {
Don't we also want to pass isoLevel to MakeModeExclusive? And then shouldn't the logic above be:
if m.IsoLevel == isolation.Serializable && o.IsoLevel == isolation.Serializable && ExclusiveLocksBlockNonLockingReadsFromSerializableTransactions.Get(sv) {
...
}pkg/kv/kvserver/concurrency/lock/locking.go line 73 at r1 (raw file):
case Exclusive: if ExclusiveLocksBlockNonLockingReadsFromSerializableTransactions.Get(sv) && o.IsoLevel == isolation.Serializable {
I wonder if this should be written as !o.IsoLevel.ToleratesWriteSkew() to better clarify the intention of this logic.
pkg/kv/kvserver/concurrency/lock/locking.proto line 182 at r1 (raw file):
// ExclusiveLocksBlockNonLockingReadsFromSerializableTransactions cluster // setting. Note that this only applies to non-locking reads from serializable // transactions, as that's the only "old" behavior to speak of here.
And it only applies to exclusive locks held by serializable transactions, right?
arulajmani
force-pushed
the
mode-iso-level
branch
from
2d36fd8 to
afb27cc
Compare
There was a problem hiding this comment.
RFAL. The combinatorial explosion of tests now that we're pushing isolation level into exclusive lock modes isn't very pretty, but I'm telling myself the coverage is worth the ugliness.
Reviewable status:
pkg/kv/kvserver/concurrency/lock/locking.go line 73 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Don't we also want to pass
isoLeveltoMakeModeExclusive? And then shouldn't the logic above be:if m.IsoLevel == isolation.Serializable && o.IsoLevel == isolation.Serializable && ExclusiveLocksBlockNonLockingReadsFromSerializableTransactions.Get(sv) { ... }
Yeah -- before we discussed this the other week, I had a unaddressed TODO capturing our discussion in this patch. Addressed that TODO, and made the changes we talked about here.
pkg/kv/kvserver/concurrency/lock/locking.go line 73 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I wonder if this should be written as
!o.IsoLevel.ToleratesWriteSkew()to better clarify the intention of this logic.
I reworked this conditional a bit, and I found !m.IsoLevel.ToleratesWriteSkew() && o.IsoLevel != isolation.Serializable clarified our intention here better -- I can switch the o to use TolerateWriteSkew() as well, if you think that's more intuitive.
pkg/kv/kvserver/concurrency/lock/locking.proto line 182 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
And it only applies to exclusive locks held by serializable transactions, right?
Yeah, changed this now that we've addressed the TODO below.
arulajmani
force-pushed
the
mode-iso-level
branch
2 times, most recently
from
9f06b11 to
9f29898
Compare
There was a problem hiding this comment.
@nvanbenschoten following up on our conversation from before, I changed the Conflicts function to take two lock modes instead of being defined on a receiver. I also implemented your suggestion for reducing the number of cases we need to consider when implementing it.
I also revamped the testing here -- the old structure didn't work too well now that we're pushing isolation levels into some lock modes, so I basically rewrote the tests here from scratch. We still have full coverage, but with 300 less lines. You might find it easier to review the test file afresh, instead of scrutinizing the diff.
Reviewable status:
There was a problem hiding this comment.
This is very nice cleanup. Thanks for doing it.
Reviewed 2 of 4 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status:
-- commits line 6 at r3:
s/lcoks/locks/
pkg/kv/kvserver/concurrency/lock/locking.go line 66 at r3 (raw file):
// Conflicts returns whether the supplied lock modes conflict with each other. // Conflict rules are as described in the compatibility matrix in locking.pb.go. func Conflicts(m1 *Mode, m2 *Mode, sv *settings.Values) bool {
Do we need these to be pointers? The size of the Mode struct is small, and the use of pointers risks heap allocations.
pkg/kv/kvserver/concurrency/lock/locking.go line 66 at r3 (raw file):
// Conflicts returns whether the supplied lock modes conflict with each other. // Conflict rules are as described in the compatibility matrix in locking.pb.go. func Conflicts(m1 *Mode, m2 *Mode, sv *settings.Values) bool {
nit: m1, m2 *Mode
pkg/kv/kvserver/concurrency/lock/locking.go line 85 at r3 (raw file):
// 1. both the non-locking read and the Exclusive lock belong to // transactions that cannot tolerate write skew. // 2. AND the non-locking read is reading at a timestamp above the lock.
s/above/equal to or above/
pkg/kv/kvserver/concurrency/lock/locking.go line 90 at r3 (raw file):
if !m1.IsoLevel.ToleratesWriteSkew() && !m2.IsoLevel.ToleratesWriteSkew() && ExclusiveLocksBlockNonLockingReads.Get(sv) { return !m1.Timestamp.Less(m2.Timestamp)
nit here and below: I personally find m2.Timestamp.LessEq(m1.Timestamp) easier to read than !m1.Timestamp.Less(m2.Timestamp) because it avoids the negation. Up to you.
pkg/kv/kvserver/concurrency/lock/locking.go line 94 at r3 (raw file):
return false case Intent: return !m1.Timestamp.Less(m2.Timestamp) // non-locking read above the intent's timestamp
s/above/equal to or above/
pkg/kv/kvserver/concurrency/lock/locking.go line 98 at r3 (raw file):
panic(errors.AssertionFailedf("unknown strength: %s", m2.Strength)) } case Shared:
Consider adding comments to these cases that reflect the normalization you performed above. Something like
case Shared:
// m2.Strength >= Shared, due to normalization above
...
case Update, Exclusive, Intent:
// m2.Strength >= Update, due to normalization above
pkg/kv/kvserver/concurrency/lock/locking_test.go line 37 at r3 (raw file):
) { t.Run(desc, func(t *testing.T) { require.Equal(t, lock.Conflicts(m1, m2, &st.SV), exp)
I believe the first argument to require.Equal is the expected value.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 45 at r3 (raw file):
Interactions with None lock mode is nonsensical
Should we still test it to make sure it works?
pkg/kv/kvserver/concurrency/lock/locking_test.go line 94 at r3 (raw file):
st := cluster.MakeTestingClusterSettings() for _, tc := range testCases { for _, iso := range []isolation.Level{
In #101955, I'm adding an isolation.Levels() utility. We could use that here and elsewhere to iterate over isolation levels.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 102 at r3 (raw file):
testCheckLockConflicts( t, fmt.Sprintf("non-locking read(%s)-%s", iso.String(), tc.desc),
nit: you don't need .String() here.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 113 at r3 (raw file):
// TestCheckLockConflicts_NoneWithExclusive tests interactions between Exclusive // locks and non-locking reads. It does so both with and without the
nit: "between non-locking reads and Exclusive locks"
pkg/kv/kvserver/concurrency/lock/locking_test.go line 124 at r3 (raw file):
ctx := context.Background() for _, isoLock := range []isolation.Level{
Same comment about isolation.Levels() here.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 131 at r3 (raw file):
} { if isoRead > isoLock { continue // we're only interested in unique permutations
Why is this? Are there redundant cases?
pkg/kv/kvserver/concurrency/lock/locking_test.go line 141 at r3 (raw file):
isoRead == isolation.Serializable && (readTS == tsLock || readTS == tsAbove) st := cluster.MakeTestingClusterSettings()
Let's Override here as well so we can change the default value of the cluster setting without breaking the test.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 167 at r3 (raw file):
} // TestCheckLockConflicts_ExclusiveWithSharedUpdateIntent tests the Conflicts
It would be easier to be convinced that these tests are exhaustive if they progressed in order of locking strength. So for instance:
- None, None
- None, Shared
- None, Update
- None, Exclusive
- None, Intent
- Shared, Shared
- Shared, Update
- Shared, Exclusive
- Shared, Intent
- Update, Update
- Update, Exclusive
- Update, Intent
- Exclusive, Exclusive
- Exclusive, Intent
- Intent, Intent
I know you want to deviate a bit from that order to test the cluster setting, but maybe try to stay a little closer to this progression.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 235 at r3 (raw file):
} // TestCheckLockConflicts_ExclusiveWithExclusive tests the Conflicts function
Why is this separate from the previous test? Because we don't want to write out a separate test case for each isolation level?
This adds tracking for isolation level information of the associated transaction in lock modes. This is only done for non-locking reads and exclusive locks. We then use this to modify the conflict resolution rules between non-locking reads and exclusive locks. The new conflict resolution rules are as follows: - If either the non-locking read or the exclusive lock belongs to a a transaction that can tolerate write-skew (read committed or snapshot), the non-locking read does not block on the exclusive lock. - If both the non-locking read and exclusive lock belong to serializable transactions and the non-locking read is doing so at a timestamp below the exclusive lock, it does not block. - Otherwise, the blocking behavior is dictated by the `ExclusiveLocksBlockNonLockingReads` cluster setting. This patch also improves how testing for the Conflicts function is organized. This was motivated by the increased surface area that needed testing now that we're pushing isolation levels into some lock modes. Note that lock modes are yet to be hooked up to the concurrency control package, and as such, the patch itself doesn't resolve the linked issue. Informs cockroachdb#94729 Release note: None
arulajmani
force-pushed
the
mode-iso-level
branch
from
9f29898 to
c959d9d
Compare
There was a problem hiding this comment.
RFAL
Reviewable status:
pkg/kv/kvserver/concurrency/lock/locking.go line 66 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Do we need these to be pointers? The size of the
Modestruct is small, and the use of pointers risks heap allocations.
Done.
pkg/kv/kvserver/concurrency/lock/locking.go line 90 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit here and below: I personally find
m2.Timestamp.LessEq(m1.Timestamp)easier to read than!m1.Timestamp.Less(m2.Timestamp)because it avoids the negation. Up to you.
Done; I also simplified this expression a bit.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 37 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I believe the first argument to
require.Equalis the expected value.
Done.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 45 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Interactions with None lock mode is nonsensical
Should we still test it to make sure it works?
Added a test.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 94 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
In #101955, I'm adding an
isolation.Levels()utility. We could use that here and elsewhere to iterate over isolation levels.
I had the same thought when I saw that PR. It's just 3 lines, so I'm inclined to just copy those over to this patch as well.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 102 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: you don't need
.String()here.
Fixed, here and elsewhere.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 131 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Why is this? Are there redundant cases?
There aren't -- my bad.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 141 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Let's Override here as well so we can change the default value of the cluster setting without breaking the test.
Good call; done.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 167 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
It would be easier to be convinced that these tests are exhaustive if they progressed in order of locking strength. So for instance:
- None, None - None, Shared - None, Update - None, Exclusive - None, Intent - Shared, Shared - Shared, Update - Shared, Exclusive - Shared, Intent - Update, Update - Update, Exclusive - Update, Intent - Exclusive, Exclusive - Exclusive, Intent - Intent, IntentI know you want to deviate a bit from that order to test the cluster setting, but maybe try to stay a little closer to this progression.
I've re-ordered some of the tests here to follow this order more closely.
There's still a slight deviation from your suggestion, in that I still have Exclusive <-> Shared and Exclusive <-> Update interactions in TestCheckLockConflicts_ExclusiveWithSharedUpdateIntent. Doing so avoids the need to repeat Exclusive locks for all isolation levels in TestCheckLockConflicts_Shared and TestCheckLockConflicts_Update. Given there are only 3 isolation levels, if you feel strongly, I can further switch things around -- let me know.
pkg/kv/kvserver/concurrency/lock/locking_test.go line 235 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Why is this separate from the previous test? Because we don't want to write out a separate test case for each isolation level?
Yeah, in this case it would have been 9 separate test cases -- 3 isolation levels x 3 possible timestamp values.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status:
|
TFTR! bors r=nvanbenschoten |
|
Build failed (retrying...): |
|
Build succeeded: |
This adds tracking for isolation level information of the associated transaction in lock modes. This is only done for non-locking reads. We then use this to modify conflict resolution rules between non-locking reads and exclusive locks -- non-locking read requests that belong to transactions running at weaker isolation levels (snapshot isolation, read committed) do not conflict with exclusive locks, regardless of their timestamp.
Behavior for non-locking read requests belonging to serializable transactions remains unchanged. It is dictated by the (now renamed) ExclusiveLocksBlockNonLockingRaedsFromSerializableTransactions cluster setting. By default, they block on exclusive locks if their timestamp is at or greater than that of the lock.
Note that lock modes are yet to be hooked up to the concurrency control package, and as such, the patch itself doesn't resolve the linked issue.
Informs #94729
Release note: None