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 + ) } }