concurrency: use lock modes to detect conflicts during optimistic eval

This patch switches over conflict resolution performed by optimistic
evaluation to use lock modes instead of ad-hoc logic. As a result of
this, optimistic evaluation is able to handle shared locks. We add a
test to show this.

Closes cockroachdb#108142

Release note: None

Author: arulajmani

pkg/kv/kvserver/concurrency/lock_table.go

@@ -667,7 +667,7 @@ func (g *lockTableGuardImpl) CheckOptimisticNoConflicts(
 		ltRange := &lockState{key: startKey, endKey: span.EndKey}
 		for iter.FirstOverlap(ltRange); iter.Valid(); iter.NextOverlap(ltRange) {
 			l := iter.Cur()
-			if !l.isNonConflictingLock(g, g.curStrength()) {
+			if !l.isNonConflictingLock(g) {
 				return false
 			}
 		}
@@ -2359,7 +2359,7 @@ func (l *lockState) claimBeforeProceeding(g *lockTableGuardImpl) {
 	panic("lock table bug: did not find enqueued request")
 }
 
-func (l *lockState) isNonConflictingLock(g *lockTableGuardImpl, str lock.Strength) bool {
+func (l *lockState) isNonConflictingLock(g *lockTableGuardImpl) bool {
 	l.mu.Lock()
 	defer l.mu.Unlock()
 
@@ -2368,13 +2368,12 @@ func (l *lockState) isNonConflictingLock(g *lockTableGuardImpl, str lock.Strengt
 		return true
 	}
 	// Lock is not empty.
-	lockHolderTxn, lockHolderTS := l.getLockHolder()
-	if lockHolderTxn == nil {
-		// Transactions that have claimed the lock, but have not acquired it yet,
-		// are considered non-conflicting.
-		//
-		// Optimistic evaluation may call into this function with or without holding
-		// latches. It's worth considering both these cases separately:
+	if !l.isHeld() {
+		// If the lock is neither empty nor held it must be the case that another
+		// transaction has claimed the lock. Locks that have been claimed, but have
+		// not been acquired yet, are considered non-conflicting. Optimistic
+		// evaluation may call into this function with or without holding latches.
+		// It's worth considering both these cases separately:
 		//
 		// 1. If Optimistic evaluation is holding latches, then there cannot be a
 		// conflicting request that has claimed (but not acquired) the lock that is
@@ -2397,19 +2396,29 @@ func (l *lockState) isNonConflictingLock(g *lockTableGuardImpl, str lock.Strengt
 		// claimed the lock will know what happened and what to do about it.
 		return true
 	}
+	lockHolderTxn, _ := l.getLockHolder()
 	if g.isSameTxn(lockHolderTxn) {
-		// Already locked by this txn.
+		// NB: Unlike the pessimistic (normal) evaluation code path, we do not need
+		// to check the lock's strength if it is already held by this transaction --
+		// it's non-conflicting. There's two cases to consider:
+		//
+		// 1. If the lock is held with the same/higher lock strength on this key
+		// then this optimistic evaluation attempt already has all the protection it
+		// needs.
+		//
+		// 2. If the lock is held with a weaker lock strength other transactions may
+		// be able to acquire a lock on this key that conflicts with this optimistic
+		// evaluation attempt. This is okay, as we'll detect such cases -- however,
+		// the weaker lock in itself is not conflicting with the optimistic
+		// evaluation attempt.
 		return true
 	}
+
 	// NB: We do not look at the txnStatusCache in this optimistic evaluation
 	// path. A conflict with a finalized txn will be noticed when retrying
 	// pessimistically.
 
-	if str == lock.None && g.ts.Less(lockHolderTS) {
-		return true
-	}
-	// Conflicts.
-	return false
+	return !lock.Conflicts(l.getLockMode(), g.curLockMode(), &g.lt.settings.SV) // non-conflicting
 }
 
 // Acquires this lock. Any requests that are waiting in the lock's wait queues

pkg/kv/kvserver/concurrency/testdata/lock_table/optimistic

@@ -132,3 +132,125 @@ num=2
   holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: unrepl [(str: Exclusive seq: 0)]
  lock: "g"
   holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: unrepl [(str: Exclusive seq: 0)]
+
+
+# ------------------------------------------------------------------------------
+# Test that optimistic evaluation succeeds if the lock is held by our own
+# transaction, regardless of lock strengths.
+# ------------------------------------------------------------------------------
+
+clear
+----
+num=0
+
+new-request r=req5 txn=txn1 ts=10,1 spans=shared@a
+----
+
+scan r=req5
+----
+start-waiting: false
+
+should-wait r=req5
+----
+false
+
+acquire r=req5 k=a durability=u strength=shared
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: unrepl [(str: Shared seq: 0)]
+
+# Ensure a optimistic evaluation attempt from the same transaction that covers
+# key "a" succeeds -- both with lower and higher lock strengths than the
+# strength of the lock already held (shared).
+
+new-request r=req6 txn=txn1 ts=10,1 spans=exclusive@a,c
+----
+
+scan-opt r=req6
+----
+start-waiting: false
+
+should-wait r=req6
+----
+false
+
+check-opt-no-conflicts r=req6 spans=exclusive@a,c
+----
+no-conflicts: true
+
+dequeue r=req6
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: unrepl [(str: Shared seq: 0)]
+
+new-request r=req7 txn=txn1 ts=10,1 spans=none@a,c
+----
+
+scan-opt r=req7
+----
+start-waiting: false
+
+should-wait r=req7
+----
+false
+
+check-opt-no-conflicts r=req7 spans=none@a,c
+----
+no-conflicts: true
+
+dequeue r=req7
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: unrepl [(str: Shared seq: 0)]
+
+# ------------------------------------------------------------------------------
+# Test that optimistic evaluation works with SHARED locking strength -- if a
+# shared lock is held, another transaction should be able to perform optimistic
+# evaluation with shared locking strength and not conflict; optimistic evaluation
+# should conflict if run with exclusive lock strength.
+# ------------------------------------------------------------------------------
+
+new-request r=req8 txn=txn2 ts=10,1 spans=none@a,c
+----
+
+scan-opt r=req8
+----
+start-waiting: false
+
+should-wait r=req8
+----
+false
+
+check-opt-no-conflicts r=req8 spans=shared@a,c
+----
+no-conflicts: true
+
+dequeue r=req8
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: unrepl [(str: Shared seq: 0)]
+
+new-request r=req9 txn=txn2 ts=10,1 spans=exclusive@a,c
+----
+
+scan-opt r=req9
+----
+start-waiting: false
+
+should-wait r=req9
+----
+false
+
+check-opt-no-conflicts r=req9 spans=exclusive@a,c
+----
+no-conflicts: false
+
+dequeue r=req9
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: unrepl [(str: Shared seq: 0)]