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 lcoks. 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

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",

pkg/kv/kvserver/concurrency/lock/locking.go

@@ -15,22 +15,34 @@ 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"
 )
 
 // 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,46 @@ 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 *Mode, 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 a timestamp above the lock.
+			// 3. AND the ExclusiveLocksBlockNonLockingReads cluster setting is
+			// configured to do as much.
+			if !m1.IsoLevel.ToleratesWriteSkew() &&
+				!m2.IsoLevel.ToleratesWriteSkew() && ExclusiveLocksBlockNonLockingReads.Get(sv) {
+				return !m1.Timestamp.Less(m2.Timestamp)
+			}
+			return false
+		case Intent:
+			return !m1.Timestamp.Less(m2.Timestamp) // non-locking read above the intent's 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:
+		return m2.Strength == Exclusive || m2.Strength == Intent
+	case Update, Exclusive, Intent:
+		return true
 	default:
-		panic(errors.AssertionFailedf("unknown strength: %s", m.Strength))
+		panic(errors.AssertionFailedf("unknown strength: %s", m1.Strength))
 	}
 }
 
@@ -85,10 +110,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 +133,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,
 	}
 }
 

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