From c959d9d65760f529ede7115673a6216eb95c5635 Mon Sep 17 00:00:00 2001 From: Arul Ajmani Date: Thu, 6 Apr 2023 12:18:15 -0400 Subject: [PATCH] lock: track isolation level information in lock modes 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 #94729 Release note: None --- .../kvserver/concurrency/isolation/levels.go | 4 + pkg/kv/kvserver/concurrency/lock/BUILD.bazel | 7 +- pkg/kv/kvserver/concurrency/lock/locking.go | 94 ++- .../kvserver/concurrency/lock/locking.proto | 38 +- .../kvserver/concurrency/lock/locking_test.go | 613 ++++++++++-------- 5 files changed, 434 insertions(+), 322 deletions(-) diff --git a/pkg/kv/kvserver/concurrency/isolation/levels.go b/pkg/kv/kvserver/concurrency/isolation/levels.go index 44372a7d6d1b..fcf303a34f79 100644 --- a/pkg/kv/kvserver/concurrency/isolation/levels.go +++ b/pkg/kv/kvserver/concurrency/isolation/levels.go @@ -46,3 +46,7 @@ func (l Level) PerStatementReadSnapshot() bool { // SafeValue implements the redact.SafeValue interface. func (Level) SafeValue() {} + +// Levels returns a list of all isolation levels, ordered from strongest to +// weakest. +func Levels() []Level { return []Level{Serializable, Snapshot, ReadCommitted} } diff --git a/pkg/kv/kvserver/concurrency/lock/BUILD.bazel b/pkg/kv/kvserver/concurrency/lock/BUILD.bazel index c7a94db39629..a5c4c3168404 100644 --- a/pkg/kv/kvserver/concurrency/lock/BUILD.bazel +++ b/pkg/kv/kvserver/concurrency/lock/BUILD.bazel @@ -13,6 +13,7 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock", visibility = ["//visibility:public"], deps = [ + "//pkg/kv/kvserver/concurrency/isolation", "//pkg/settings", "//pkg/util/hlc", "@com_github_cockroachdb_errors//:errors", @@ -29,6 +30,7 @@ proto_library( strip_import_prefix = "/pkg", visibility = ["//visibility:public"], deps = [ + "//pkg/kv/kvserver/concurrency/isolation:isolation_proto", "//pkg/storage/enginepb:enginepb_proto", "//pkg/util/hlc:hlc_proto", "@com_github_gogo_protobuf//gogoproto:gogo_proto", @@ -43,6 +45,7 @@ go_proto_library( proto = ":lock_proto", visibility = ["//visibility:public"], deps = [ + "//pkg/kv/kvserver/concurrency/isolation", "//pkg/storage/enginepb", "//pkg/util/hlc", "@com_github_gogo_protobuf//gogoproto", @@ -58,11 +61,13 @@ go_test( args = ["-test.timeout=295s"], deps = [ ":lock", + "//pkg/kv/kvserver/concurrency/isolation", "//pkg/roachpb", "//pkg/settings/cluster", "//pkg/storage/enginepb", - "//pkg/testutils", "//pkg/util/hlc", + "//pkg/util/leaktest", + "//pkg/util/log", "//pkg/util/uuid", "@com_github_cockroachdb_redact//:redact", "@com_github_stretchr_testify//require", diff --git a/pkg/kv/kvserver/concurrency/lock/locking.go b/pkg/kv/kvserver/concurrency/lock/locking.go index 4cdbfb389856..111393ebad96 100644 --- a/pkg/kv/kvserver/concurrency/lock/locking.go +++ b/pkg/kv/kvserver/concurrency/lock/locking.go @@ -15,6 +15,7 @@ package lock import ( "fmt" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/errors" @@ -22,15 +23,26 @@ import ( // ExclusiveLocksBlockNonLockingReads dictates locking interactions between // non-locking reads and exclusive locks. Configuring this setting to true makes -// it such that non-locking reads block on exclusive locks if their read -// timestamp is at or above the timestamp at which the lock is held; however, -// if this setting is set to false, non-locking reads do not block on exclusive -// locks, regardless of their relative timestamp. +// it such that non-locking reads from serializable transactions block on +// exclusive locks held by serializable transactions if the read's timestamp is +// at or above the timestamp at which the lock is held. If set to false, +// non-locking reads do not block on exclusive locks, regardless of isolation +// level or timestamp. // -// The tradeoff here is between increased concurrency (non-locking reads become -// non-blocking in the face of Exclusive locks) at the expense of forcing -// Exclusive lock holders to perform a read refresh, which in turn may force -// them to restart if the refresh fails. +// Note that the setting only applies if both the reader and lock holder are +// running with serializable isolation level. If either of them is running with +// weaker isolation levels, the setting has no effect. To understand why, +// consider the tradeoff this setting presents -- the tradeoff here is increased +// concurrency (non-locking reads become non-blocking in the face of Exclusive +// locks) at the expense of forcing Exclusive lock holders to perform a read +// refresh (to prevent write skew), which in turn may force them to restart if +// the refresh fails. +// +// If the lock holder is running at a weaker isolation level (snapshot, +// read committed), then it is able to tolerate write skew. Thus, there is no +// tradeoff -- it is always a good idea to allow any non-locking read to +// proceed. On the other hand, non-locking reads running at weaker isolation +// levels should never block on exclusive locks. var ExclusiveLocksBlockNonLockingReads = settings.RegisterBoolSetting( settings.SystemOnly, "kv.lock.exclusive_locks_block_non_locking_reads.enabled", @@ -49,33 +61,49 @@ func init() { } } -// Conflicts returns whether the supplied lock mode conflicts with the receiver. -// The conflict rules are as described in the compatibility matrix in -// locking.pb.go. -func (m *Mode) Conflicts(sv *settings.Values, o *Mode) bool { - if m.Empty() || o.Empty() { +// 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, m2 Mode, sv *settings.Values) bool { + if m1.Empty() || m2.Empty() { panic("cannot check conflict for uninitialized locks") } - switch m.Strength { + if m1.Strength > m2.Strength { + // Conflict rules are symmetric, so reduce the number of cases we need to + // handle. + m1, m2 = m2, m1 + } + switch m1.Strength { case None: - return false - case Shared: - return o.Strength == Exclusive || o.Strength == Intent - case Update: - return o.Strength == Update || o.Strength == Exclusive || o.Strength == Intent - case Exclusive: - if ExclusiveLocksBlockNonLockingReads.Get(sv) { - // Only non-locking reads below the timestamp at which the lock is held - // are allowed. - return !(o.Strength == None && o.Timestamp.Less(m.Timestamp)) + switch m2.Strength { + case None, Shared, Update: + return false + case Exclusive: + // Non-locking reads only conflict with Exclusive locks if the following + // conditions hold: + // 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 or above the timestamp of the + // lock. + // 3. AND the ExclusiveLocksBlockNonLockingReads cluster setting is + // configured to do as much. + return !m1.IsoLevel.ToleratesWriteSkew() && + !m2.IsoLevel.ToleratesWriteSkew() && m2.Timestamp.LessEq(m1.Timestamp) && + ExclusiveLocksBlockNonLockingReads.Get(sv) + case Intent: + // Non-locking read (m1) conflicts if the intent is at a lower or equal + // timestamp. + return m2.Timestamp.LessEq(m1.Timestamp) + default: + panic(errors.AssertionFailedf("unknown strength: %s", m2.Strength)) } - return o.Strength != None // non-locking read. - case Intent: - // Only non-locking read, below held the timestamp at which the lock is held - // are allowed. - return !(o.Strength == None && o.Timestamp.Less(m.Timestamp)) + case Shared: + // m2.Strength >= Shared due to the normalization above. + return m2.Strength == Exclusive || m2.Strength == Intent + case Update, Exclusive, Intent: + // m2.Strength >= Update due to the normalization above. + return true default: - panic(errors.AssertionFailedf("unknown strength: %s", m.Strength)) + panic(errors.AssertionFailedf("unknown strength: %s", m1.Strength)) } } @@ -85,10 +113,11 @@ func (m *Mode) Empty() bool { } // MakeModeNone constructs a Mode with strength None. -func MakeModeNone(ts hlc.Timestamp) Mode { +func MakeModeNone(ts hlc.Timestamp, isoLevel isolation.Level) Mode { return Mode{ Strength: None, Timestamp: ts, + IsoLevel: isoLevel, } } @@ -107,10 +136,11 @@ func MakeModeUpdate() Mode { } // MakeModeExclusive constructs a Mode with strength Exclusive. -func MakeModeExclusive(ts hlc.Timestamp) Mode { +func MakeModeExclusive(ts hlc.Timestamp, isoLevel isolation.Level) Mode { return Mode{ Strength: Exclusive, Timestamp: ts, + IsoLevel: isoLevel, } } diff --git a/pkg/kv/kvserver/concurrency/lock/locking.proto b/pkg/kv/kvserver/concurrency/lock/locking.proto index d17368a6bc63..3201f9585631 100644 --- a/pkg/kv/kvserver/concurrency/lock/locking.proto +++ b/pkg/kv/kvserver/concurrency/lock/locking.proto @@ -12,6 +12,7 @@ syntax = "proto3"; package cockroach.kv.kvserver.concurrency.lock; option go_package = "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock"; +import "kv/kvserver/concurrency/isolation/levels.proto"; import "gogoproto/gogo.proto"; import "util/hlc/timestamp.proto"; @@ -130,10 +131,11 @@ enum Strength { // // The protection a lock offers is primarily determined by the Strength it is // held with. However, lock Modes also contain auxiliary information (eg. the -// timestamp at which the lock was acquired), which may be used in conjunction -// with a lock's Strength to resolve conflicts between a lock holder and a -// concurrent lock acquirer. This allows us to codify conflict rules as a pure -// function of 2 lock Modes. +// timestamp at which the lock was acquired, the isolation level associated with +// a non-locking read), which may be used in conjunction with a lock's Strength +// to resolve conflicts between a lock holder and a concurrent overlapping +// request. This allows us to codify conflict rules as a pure function of 2 lock +// Modes. // // Users of the key-value layer are expected to indicate locking intentions // using lock Strengths; lock Mode is an internal concept for the concurrency @@ -167,21 +169,29 @@ enum Strength { // lock's timestamp. If the read's timestamp is below the Intent lock's // timestamp then the two are compatible. // -// [*] historically, CockroachDB did not make a strength distinction between -// Exclusive locks and Intents. As such, reads under optimistic concurrency -// control would conflict with Exclusive locks if the read's timestamp was at or -// above the Exclusive lock's timestamp. Now that there is a distinction between -// Exclusive locks and Intents, non-locking reads do not block on Exclusive -// locks, regardless of their timestamp. However, there is a desire to let users -// opt into the old behavior using the ExclusiveLocksBlockNonLockingReads -// cluster setting. +// [*] until the re-introduction of weaker isolation levels, all transactions in +// CockroachDB used serializable isolation. Historically, CockroachDB did not +// make a strength distinction between Exclusive locks and Intents. As such, +// reads under concurrency control would conflict with Exclusive locks if the +// read's timestamp was at or above the Exclusive lock's timestamp. Now that +// there is such a distinction, non-locking reads from serializable transactions +// do not block on Exclusive locks, regardless of their timestamp. However, +// there is a desire to let users opt into the old behavior using the +// ExclusiveLocksBlockNonLockingReads cluster setting. Note that this only +// applies when both the non-locking read and Exclusive lock belong to +// serializable transactions, as that's the only "old" behavior to speak of +// here. message Mode { // Strength in which the lock is held. Strength strength = 1; - // Timestamp at which the lock was/is being acquired. This field must be set - // for None, Exclusive, and Intent locking strengths. + // Timestamp at which the lock was/is being acquired. This field must (and + // only) be set for None, Exclusive, and Intent locking strengths. util.hlc.Timestamp timestamp = 2 [(gogoproto.nullable)=false]; + + // IsoLevel is the isolation level of the associated transaction. This field + // is only set for None and Exclusive locking strength. + cockroach.kv.kvserver.concurrency.isolation.Level iso_level = 3; } // Durability represents the different durability properties of a lock acquired diff --git a/pkg/kv/kvserver/concurrency/lock/locking_test.go b/pkg/kv/kvserver/concurrency/lock/locking_test.go index 330e4794a5d3..a9c6fe708b30 100644 --- a/pkg/kv/kvserver/concurrency/lock/locking_test.go +++ b/pkg/kv/kvserver/concurrency/lock/locking_test.go @@ -12,332 +12,395 @@ package lock_test import ( "context" + "fmt" "testing" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock" "github.com/cockroachdb/cockroach/pkg/settings/cluster" - "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/util/hlc" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/stretchr/testify/require" ) -// TestCheckLockConflicts ensures the lock conflict semantics, as -// illustrated in the compatibility matrix in locking.pb.go, are upheld. -func TestCheckLockConflicts(t *testing.T) { - makeTS := func(nanos int64) hlc.Timestamp { - return hlc.Timestamp{ - WallTime: nanos, +func makeTS(nanos int64) hlc.Timestamp { + return hlc.Timestamp{ + WallTime: nanos, + } +} + +func testCheckLockConflicts( + t *testing.T, desc string, m1, m2 lock.Mode, st *cluster.Settings, exp bool, +) { + t.Run(desc, func(t *testing.T) { + require.Equal(t, exp, lock.Conflicts(m1, m2, &st.SV)) + // Test for symmetry -- things should work the other way around as well. + require.Equal(t, exp, lock.Conflicts(m2, m1, &st.SV)) + }) +} + +// TestCheckLockConflicts_NoneWithNone tests the Conflicts function for all +// combinations where both modes belong to non-locking reads. We never expect +// this to happen in practice, as non-locking reads don't acquire locks in the +// lock table; however, this test ensures interactions are sane, and two +// non-locking reads never conflict. +func TestCheckLockConflicts_NoneWithNone(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + tsBelow := makeTS(1) + tsMid := makeTS(2) + tsAbove := makeTS(3) + + st := cluster.MakeTestingClusterSettings() + for _, ts := range []hlc.Timestamp{tsBelow, tsMid, tsAbove} { + for _, iso1 := range isolation.Levels() { + for _, iso2 := range isolation.Levels() { + if iso2.WeakerThan(iso1) { + continue // we only care about unique permutations + } + r1 := lock.MakeModeNone(tsMid, iso1) + r2 := lock.MakeModeNone(ts, iso2) + testCheckLockConflicts( + t, + fmt.Sprintf("exclusive lock(%s)-exclusive lock(%s)", iso1, iso2), + r1, + r2, + st, + false, // never conflicts + ) + } } } +} + +// TestCheckLockConflicts_NoneWithSharedUpdateIntent tests the Conflicts +// function for all combinations of None lock modes with either Shared, or +// Update, or Intent lock modes. Interactions with None lock mode (although +// nonsensical) are tested above and interactions with Exclusive locks are +// tested separately. +func TestCheckLockConflicts_NoneWithSharedUpdateIntent(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) tsBelow := makeTS(1) - tsLock := makeTS(2) + ts := makeTS(2) tsAbove := makeTS(3) testCases := []struct { - desc string - heldLockMode lock.Mode - toCheckLockMode lock.Mode - // Expectation for when non-locking reads do not block on Exclusive locks. - exp bool - // Expectation when non-locking reads do block on Exclusive locks. This - // distinction is only meaningful when testing for None <-> Exclusive lock - // mode interactions. - expExclusiveLocksBlockNonLockingReads bool + desc string + mode lock.Mode + exp bool }{ - // A. Held lock mode is "Shared". - // A1. Shared lock mode is doesn't conflict with non-locking reads. - { - desc: "non-locking read, held shared lock", - heldLockMode: lock.MakeModeShared(), - toCheckLockMode: lock.MakeModeNone(tsLock), - exp: false, - expExclusiveLocksBlockNonLockingReads: false, - }, - // A2. Shared lock mode doesn't conflict with itself. - { - desc: "shared lock acquisition, held shared lock", - heldLockMode: lock.MakeModeShared(), - toCheckLockMode: lock.MakeModeShared(), - exp: false, - expExclusiveLocksBlockNonLockingReads: false, - }, - // A3. Shared lock mode doesn't conflict with an Update lock mode. - { - desc: "update lock acquisition, held shared lock", - heldLockMode: lock.MakeModeShared(), - toCheckLockMode: lock.MakeModeUpdate(), - exp: false, - expExclusiveLocksBlockNonLockingReads: false, - }, - // A4. Shared lock mode conflicts with Exclusive lock mode. - { - desc: "exclusive lock acquisition, held shared lock", - heldLockMode: lock.MakeModeShared(), - toCheckLockMode: lock.MakeModeExclusive(tsLock), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, - }, - // A5. Shared lock mode conflicts with Intent lock mode. + // A. With Shared locks. { - desc: "update lock acquisition, held shared lock", - heldLockMode: lock.MakeModeShared(), - toCheckLockMode: lock.MakeModeIntent(tsLock), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "shared lock", + mode: lock.MakeModeShared(), + exp: false, }, - // B. Held lock mode is "Update". - // B1. Update lock mode doesn't conflict with non-locking reads. + // B. With Update locks. { - desc: "non-locking read, held update lock", - heldLockMode: lock.MakeModeUpdate(), - toCheckLockMode: lock.MakeModeNone(tsLock), - exp: false, - expExclusiveLocksBlockNonLockingReads: false, + desc: "update lock", + mode: lock.MakeModeUpdate(), + exp: false, }, - // B2. Update lock mode doesn't conflict with Shared lock mode. + // C. With Intents. + // C1. Below the read timestamp. { - desc: "shared lock acquisition, held update lock", - heldLockMode: lock.MakeModeUpdate(), - toCheckLockMode: lock.MakeModeShared(), - exp: false, - expExclusiveLocksBlockNonLockingReads: false, + desc: "intent below ts", + mode: lock.MakeModeIntent(tsBelow), + exp: true, }, - // B3. Update lock mode conflicts with a concurrent attempt to acquire an - // Update lock. + // C2. At the read timestamp. { - desc: "update lock acquisition, held update lock", - heldLockMode: lock.MakeModeUpdate(), - toCheckLockMode: lock.MakeModeUpdate(), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "intent at ts", + mode: lock.MakeModeIntent(ts), + exp: true, }, - // B4. Update lock mode conflicts with Exclusive lock mode. + // C3. Above the read timestamp. { - desc: "exclusive lock acquisition, held update lock", - heldLockMode: lock.MakeModeUpdate(), - toCheckLockMode: lock.MakeModeExclusive(tsLock), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, - }, - // B5. Update lock mode conflicts with Intent lock mode. - { - desc: "update lock acquisition, held shared lock", - heldLockMode: lock.MakeModeUpdate(), - toCheckLockMode: lock.MakeModeIntent(tsLock), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "intent above ts", + mode: lock.MakeModeIntent(tsAbove), + exp: false, }, + } + st := cluster.MakeTestingClusterSettings() + for _, tc := range testCases { + for _, iso := range isolation.Levels() { + nonLockingRead := lock.MakeModeNone(ts, iso) + testCheckLockConflicts( + t, + fmt.Sprintf("non-locking read(%s)-%s", iso, tc.desc), + nonLockingRead, + tc.mode, + st, + tc.exp, + ) + } + } +} - // C. Held lock mode is "Exclusive". - // C1. Non-locking reads below the timestamp of Exclusive locks should never - // conflict. - { - desc: "non-locking read at lower timestamp, held exclusive lock", - heldLockMode: lock.MakeModeExclusive(tsLock), - toCheckLockMode: lock.MakeModeNone(tsBelow), - exp: false, - expExclusiveLocksBlockNonLockingReads: false, - }, - // C2. Non-locking reads, at the same timestamp at which an Exclusive lock - // is held, conflict depending on the value of the - // ExclusiveLocksBlockNonLockingReads cluster setting. - { - desc: "non-locking read at lock timestamp, held exclusive lock", - heldLockMode: lock.MakeModeExclusive(tsLock), - toCheckLockMode: lock.MakeModeNone(tsLock), - exp: false, - expExclusiveLocksBlockNonLockingReads: true, - }, - // C3. Ditto for non-locking reads above the timestamp of the Exclusive - // lock. - { - desc: "non-locking read at lock timestamp, held exclusive lock", - heldLockMode: lock.MakeModeExclusive(tsLock), - toCheckLockMode: lock.MakeModeNone(tsAbove), - exp: false, - expExclusiveLocksBlockNonLockingReads: true, - }, - // C4. Exclusive lock mode conflicts with Shared lock mode. - { - desc: "shared lock acquisition, held exclusive lock", - heldLockMode: lock.MakeModeExclusive(tsLock), - toCheckLockMode: lock.MakeModeShared(), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, - }, - // C5. Exclusive lock mode conflicts with Update lock mode. - { - desc: "update lock acquisition, held exclusive lock", - heldLockMode: lock.MakeModeExclusive(tsLock), - toCheckLockMode: lock.MakeModeUpdate(), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, - }, - // C6. Exclusive lock mode conflicts with Exclusive locks at lower - // timestamps. - { - desc: "exclusive lock acquisition, held exclusive lock", - heldLockMode: lock.MakeModeExclusive(tsLock), - toCheckLockMode: lock.MakeModeExclusive(tsBelow), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, - }, - // C7. Ditto for the "at timestamp" case. - { - desc: "exclusive lock acquisition, held exclusive lock", - heldLockMode: lock.MakeModeExclusive(tsLock), - toCheckLockMode: lock.MakeModeExclusive(tsLock), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, - }, - // C8. As well as the "above timestamp" case. - { - desc: "exclusive lock acquisition, held exclusive lock", - heldLockMode: lock.MakeModeExclusive(tsLock), - toCheckLockMode: lock.MakeModeExclusive(tsAbove), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, - }, - // C9. Exclusive lock mode conflicts with Intent lock mode at lower - // timestamps. - { - desc: "intent lock mode acquisition, held exclusive lock", - heldLockMode: lock.MakeModeExclusive(tsLock), - toCheckLockMode: lock.MakeModeIntent(tsBelow), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, - }, - // C10. Ditto for the "at timestamp" case. - { - desc: "intent lock mode acquisition, held exclusive lock", - heldLockMode: lock.MakeModeExclusive(tsLock), - toCheckLockMode: lock.MakeModeIntent(tsLock), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, - }, - // C11. As well as the "above timestamp" case. - { - desc: "intent lock mode acquisition, held exclusive lock", - heldLockMode: lock.MakeModeExclusive(tsLock), - toCheckLockMode: lock.MakeModeIntent(tsAbove), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, - }, +// TestCheckLockConflicts_NoneWithExclusive tests interactions between +// non-locking reads and Exclusive locks. It does so both with and without the +// ExclusiveLocksBlockNonLockingReads cluster setting overridden. +func TestCheckLockConflicts_NoneWithExclusive(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) - // D. Held lock mode is "Intent". - // D1. Non-locking reads below the timestamp of the Intent do not conflict. - { - desc: "non-locking read at lower timestamp, held intent", - heldLockMode: lock.MakeModeIntent(tsLock), - toCheckLockMode: lock.MakeModeNone(tsBelow), - exp: false, - expExclusiveLocksBlockNonLockingReads: false, - }, - // D2. However, non-locking reads at the timestamp of the Intent conflict. + tsBelow := makeTS(1) + tsLock := makeTS(2) + tsAbove := makeTS(3) + + ctx := context.Background() + for _, isoLock := range isolation.Levels() { + for _, isoRead := range isolation.Levels() { + for i, readTS := range []hlc.Timestamp{tsBelow, tsLock, tsAbove} { + exclusiveLock := lock.MakeModeExclusive(tsLock, isoLock) + nonLockingRead := lock.MakeModeNone(readTS, isoRead) + + expConflicts := isoLock == isolation.Serializable && + isoRead == isolation.Serializable && (readTS == tsLock || readTS == tsAbove) + + st := cluster.MakeTestingClusterSettings() + // Test with the ExclusiveLocksBlockNonLockingReads cluster setting + // enabled. + lock.ExclusiveLocksBlockNonLockingReads.Override(ctx, &st.SV, true) + testCheckLockConflicts( + t, + fmt.Sprintf("#%d non-locking read(%s) exclusive(%s)", i, isoRead, isoLock), + nonLockingRead, + exclusiveLock, + st, + expConflicts, + ) + + // Now, test with the ExclusiveLocksBlockNonLockingReads cluster setting + // disabled, and ensure the two lock modes do not conflict. + lock.ExclusiveLocksBlockNonLockingReads.Override(ctx, &st.SV, false) + testCheckLockConflicts( + t, + fmt.Sprintf("#%d non-locking read(%s) exclusive(%s)", i, isoRead, isoLock), + nonLockingRead, + exclusiveLock, + st, + false, // should not conflict + ) + } + } + } +} + +// TestCheckLockConflicts_Shared tests the Conflicts function where one of the +// lock modes is Shared. It tests these with other Shared, Update, and Intent +// lock modes. Tests for non-locking reads are already covered above and tests +// with Exclusive are covered below. +func TestCheckLockConflicts_Shared(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ts := makeTS(2) + + testCases := []struct { + desc string + mode lock.Mode + exp bool + }{ + // A. With Shared locks. { - desc: "non-locking read at lock timestamp, held intent", - heldLockMode: lock.MakeModeIntent(tsLock), - toCheckLockMode: lock.MakeModeNone(tsLock), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "shared lock", + mode: lock.MakeModeShared(), + exp: false, }, - // D3. Ditto for non-locking reads above the Intent's timestamp. + // B. With Update locks. { - desc: "non-locking read at lock timestamp, held intent", - heldLockMode: lock.MakeModeIntent(tsLock), - toCheckLockMode: lock.MakeModeNone(tsAbove), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "update lock", + mode: lock.MakeModeUpdate(), + exp: false, }, - // D4. Intent lock mode conflicts with Shared lock mode. + // C. With Intents. { - desc: "shared lock acquisition, held intent", - heldLockMode: lock.MakeModeIntent(tsLock), - toCheckLockMode: lock.MakeModeShared(), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "intent", + mode: lock.MakeModeIntent(ts), + exp: true, }, - // D5. Intent lock mode conflicts with Update lock mode. + } + st := cluster.MakeTestingClusterSettings() + for _, tc := range testCases { + sharedLock := lock.MakeModeShared() + testCheckLockConflicts( + t, + fmt.Sprintf("shared lock-%s", tc.desc), + sharedLock, + tc.mode, + st, + tc.exp, + ) + } +} + +// TestCheckLockConflicts_Update tests the Conflicts function where one of the +// lock modes is Update. It tests these with other Update and Intent +// lock modes. Tests for non-locking reads and Shared locks are already covered +// above, and tests with Exclusive locks are covered below. +func TestCheckLockConflicts_Update(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ts := makeTS(2) + + testCases := []struct { + desc string + mode lock.Mode + exp bool + }{ + // A. With Update locks. { - desc: "update lock acquisition, held intent", - heldLockMode: lock.MakeModeIntent(tsLock), - toCheckLockMode: lock.MakeModeUpdate(), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "update lock", + mode: lock.MakeModeUpdate(), + exp: true, }, - // D6. Intent lock mode conflicts with Exclusive locks at lower - // timestamps. + // B. With Intents. { - desc: "exclusive lock acquisition, held intent", - heldLockMode: lock.MakeModeIntent(tsLock), - toCheckLockMode: lock.MakeModeExclusive(tsBelow), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "intent", + mode: lock.MakeModeIntent(ts), + exp: true, }, - // D7. Ditto for the "at timestamp" case. + } + st := cluster.MakeTestingClusterSettings() + for _, tc := range testCases { + updateLock := lock.MakeModeUpdate() + testCheckLockConflicts( + t, + fmt.Sprintf("update lock-%s", tc.desc), + updateLock, + tc.mode, + st, + tc.exp, + ) + } +} + +// TestCheckLockConflicts_ExclusiveWithSharedUpdateIntent tests the Conflicts +// function for all combinations where one of the locks is Exclusive and the +// other is either Shared or Update or Intent. Tests with non-locking reads are +// already covered below and Exclusive-Exclusive interactions are covered below. +func TestCheckLockConflicts_ExclusiveWithSharedUpdateIntent(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + tsBelow := makeTS(1) + ts := makeTS(2) + tsAbove := makeTS(3) + + testCases := []struct { + desc string + mode lock.Mode + exp bool + }{ + // A. With Shared locks. { - desc: "exclusive lock acquisition, held intent", - heldLockMode: lock.MakeModeIntent(tsLock), - toCheckLockMode: lock.MakeModeExclusive(tsLock), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "shared lock", + mode: lock.MakeModeShared(), + exp: true, }, - // D8. As well as the "above timestamp" case. + // B. With Update locks. { - desc: "exclusive lock acquisition, held intent", - heldLockMode: lock.MakeModeIntent(tsLock), - toCheckLockMode: lock.MakeModeExclusive(tsAbove), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "update lock", + mode: lock.MakeModeUpdate(), + exp: true, }, - // D9. Intent lock mode conflicts with another Intent lock mode at a - // lower timestamp. + // C. With Intents. + // C1. Below the lock timestamp. { - desc: "intent lock mode acquisition, held intent", - heldLockMode: lock.MakeModeIntent(tsLock), - toCheckLockMode: lock.MakeModeIntent(tsBelow), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "intent below ts", + mode: lock.MakeModeIntent(tsBelow), + exp: true, }, - // D10. Ditto for the "at timestamp" case. + // C2. At the lock timestamp. { - desc: "intent lock mode acquisition, held intent", - heldLockMode: lock.MakeModeIntent(tsLock), - toCheckLockMode: lock.MakeModeIntent(tsLock), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "intent at ts", + mode: lock.MakeModeIntent(ts), + exp: true, }, - // D11. As well as the "above timestamp" case. + // C3. Above the lock timestamp. { - desc: "intent lock mode acquisition, held intent", - heldLockMode: lock.MakeModeIntent(tsLock), - toCheckLockMode: lock.MakeModeIntent(tsAbove), - exp: true, - expExclusiveLocksBlockNonLockingReads: true, + desc: "intent above ts", + mode: lock.MakeModeIntent(tsAbove), + exp: true, }, } + st := cluster.MakeTestingClusterSettings() + for _, tc := range testCases { + for _, iso := range isolation.Levels() { + exclusiveLock := lock.MakeModeExclusive(ts, iso) + testCheckLockConflicts( + t, + fmt.Sprintf("exclusive lock(%s)-%s", iso, tc.desc), + exclusiveLock, + tc.mode, + st, + tc.exp, + ) + } + } +} + +// TestCheckLockConflicts_ExclusiveWithExclusive tests the Conflicts function +// for all combinations where both the locks have Exclusive lock strength. +// Exclusive locks always conflict with other Exclusive locks. +func TestCheckLockConflicts_ExclusiveWithExclusive(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + tsBelow := makeTS(1) + tsLock := makeTS(2) + tsAbove := makeTS(3) - ctx := context.Background() st := cluster.MakeTestingClusterSettings() + for _, ts := range []hlc.Timestamp{tsBelow, tsLock, tsAbove} { + for _, iso1 := range isolation.Levels() { + for _, iso2 := range isolation.Levels() { + if iso2.WeakerThan(iso1) { + continue // we only care about unique permutations + } + exclusiveLock := lock.MakeModeExclusive(tsLock, iso1) + exclusiveLock2 := lock.MakeModeExclusive(ts, iso2) + testCheckLockConflicts( + t, + fmt.Sprintf("exclusive lock(%s)-exclusive lock(%s)", iso1, iso2), + exclusiveLock, + exclusiveLock2, + st, + true, // always conflicts + ) + } + } + } +} - for _, tc := range testCases { - testutils.RunTrueAndFalse(t, - "exclusive-locks-block-non-locking-reads-override", - func(t *testing.T, exclusiveLocksBlockNonLockingReadsOverride bool) { - t.Run(tc.desc, func(t *testing.T) { - lock.ExclusiveLocksBlockNonLockingReads.Override( - ctx, &st.SV, exclusiveLocksBlockNonLockingReadsOverride, - ) - exp := tc.exp - if exclusiveLocksBlockNonLockingReadsOverride { - exp = tc.expExclusiveLocksBlockNonLockingReads - } - - require.Equal( - t, exp, tc.heldLockMode.Conflicts(&st.SV, &tc.toCheckLockMode), - ) - }) - }) +// TestCheckLockConflicts_IntentWithIntent tests the Conflicts for all +// combinations where both the locks have Intent lock strength. Intents always +// conflict with other Intents. +func TestCheckLockConflicts_IntentWithIntent(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + tsBelow := makeTS(1) + tsLock := makeTS(2) + tsAbove := makeTS(3) + + st := cluster.MakeTestingClusterSettings() + for i, ts := range []hlc.Timestamp{tsBelow, tsLock, tsAbove} { + intent1 := lock.MakeModeIntent(tsLock) + intent2 := lock.MakeModeIntent(ts) + testCheckLockConflicts( + t, + fmt.Sprintf("%d-intent-intent", i), + intent1, + intent2, + st, + true, // always conflicts + ) } }