concurrency: don't consider lock acquisitions at higher epochs idempotent

Stateful transaction retries that result from an IntentMissingError
happen at the same WriteTimestamp as the previous attempt.

Thus, sequences like the one included in the test:

 - Req0: Put@epo=0 (not included below since it has no lock table side effects)
 - Req1: Get(Exclusive, Unreplicated)@epo=0
 - Req2: Get(Shared, Replicated)@epo=0
 - Req3: Put@epo=0
 - IntentMissingError on some unrelated key.
 - Req4: Put@epo=1 (retry of Req0)

Would not result in the unreplicated lock being cleared from the lock
table, even though it would be cleared on most other types of retries.

Fixes #148635
Release note: None

Author: stevendanna

pkg/kv/kvserver/concurrency/lock_table.go

@@ -1587,16 +1587,24 @@ func (tl *txnLock) isIdempotentLockAcquisition(acq *roachpb.LockAcquisition) boo
 			tl.unreplicatedInfo.ts.Equal(acq.Txn.WriteTimestamp) && tl.unreplicatedInfo.ignoredSequenceNumbersEqual(acq)
 	case lock.Replicated:
 		// Lock is being re-acquired with the same strength.
-		return tl.replicatedInfo.held(acq.Strength) &&
-			// NB: Lock re-acquisitions at different timestamps are not considered
-			// idempotent. Strictly speaking, we could tighten this condition in a
-			// few ways:
-			// 1. We could consider lock re-acquisition at a lower timestamp idempotent,
-			// as a lock's timestamp at a given durability can never regress.
-			// 2. We could only check the timestamp if the lock is being acquired with
-			// strength lock.Intent, as we disregard the timestamp for all other lock
-			// strengths when dealing with replicated locks.
-			tl.replicatedInfo.ts.Equal(acq.Txn.WriteTimestamp)
+		isIdempotent := tl.replicatedInfo.held(acq.Strength)
+		// NB: Lock re-acquisitions at different timestamps are not considered
+		// idempotent. Strictly speaking, we could tighten this condition in a
+		// few ways:
+		//
+		// 1. We could consider lock re-acquisition at a lower timestamp idempotent,
+		// as a lock's timestamp at a given durability can never regress.
+		//
+		// 2. We could only check the timestamp if the lock is being acquired with
+		// strength lock.Intent, as we disregard the timestamp for all other lock
+		// strengths when dealing with replicated locks.
+		isIdempotent = isIdempotent && tl.replicatedInfo.ts.Equal(acq.Txn.WriteTimestamp)
+		// Lock acquisitions at a new epoch are not considered idempotent. In most
+		// cases of an epoch bump, the WriteTimestamp also changes. But in the case
+		// of an IntentMissingError, the WriteTimestamp isn't forwarded, but the
+		// on-disk intent has still changed.
+		isIdempotent = isIdempotent && tl.txn.Epoch == acq.Txn.Epoch
+		return isIdempotent
 	default:
 		panic(fmt.Sprintf("unknown lock durability: %s", acq.Durability))
 	}
@@ -3690,7 +3698,7 @@ func (kl *keyLocks) tryFreeLockOnReplicatedAcquire(
 	// Bail if there's an epoch number mismatch, as we can't compare sequence
 	// numbers across epochs. Note that we're not making any effort to forget
 	// unreplicated locks if the replicated lock acquisition corresponds to a
-	// newer epoch -- we defer to acquireLock to handle epcoh bumps instead.
+	// newer epoch -- we defer to acquireLock to handle epoch bumps instead.
 	if tl.txn.Epoch != acq.Txn.Epoch {
 		return false /* freed */, false /* mustGC */
 	}

pkg/kv/kvserver/concurrency/lock_table_test.go

@@ -450,6 +450,22 @@ func TestLockTableBasic(t *testing.T) {
 				}
 				return lt.String()
 
+			case "update-txn-not-observed":
+				var txnName string
+				d.ScanArgs(t, "txn", &txnName)
+				txnMeta, ok := txnsByName[txnName]
+				if !ok {
+					d.Fatalf(t, "unknown txn %s", txnName)
+				}
+				ts := scanTimestamp(t, d)
+				var epoch int
+				d.ScanArgs(t, "epoch", &epoch)
+				txnMeta = &enginepb.TxnMeta{ID: txnMeta.ID, Sequence: txnMeta.Sequence}
+				txnMeta.Epoch = enginepb.TxnEpoch(epoch)
+				txnMeta.WriteTimestamp = ts
+				txnsByName[txnName] = txnMeta
+				return ""
+
 			case "add-discovered":
 				var reqName string
 				d.ScanArgs(t, "r", &reqName)

pkg/kv/kvserver/concurrency/testdata/lock_table/acquire_idepmpotency_retry

@@ -0,0 +1,118 @@
+# -------------------------------------------------------------
+# This test is a regression test for an issue in which a
+# series of acquires resulted in the lock table holding an unreplicated
+# lock belonging to an older epoch.
+#
+# The actual test failure involved a longer and more complicated
+# sequence of acquisition, here we've reduce it the following
+# operations on a single key:
+#
+# Req0: Put@epo=0 (not included below since it has no lock table side effects)
+# Req1: Get(Exclusive, Unreplicated)@epo=0
+# Req2: Get(Shared, Replicated)@epo=0
+# Req3: Put@epo=0
+#
+# Then we bump the transaction Epoch to 1 but leave the WriteTimestamp
+# the same -- simulating an IntentMissingError on some unrelated
+# key.
+#
+# Finally, we send what would be the first request in the retrying
+# transaction:
+#
+# Req4: Put@epo=1
+#
+# Previously, this sequence would leave the unreplicated lock from
+# epoch=0 in our lock table. In most cases this does not cause
+# problems, but if we want to flush the lock table to disk, we would
+# fail with an error.
+# -------------------------------------------------------------
+
+new-lock-table maxlocks=10000
+----
+
+new-txn txn=txn1 ts=10,1 epoch=0 seq=1
+----
+
+new-request r=req1 txn=txn1 ts=10,1 spans=intent@a
+----
+
+scan r=req1
+----
+start-waiting: false
+
+acquire r=req1 k=a durability=u strength=exclusive
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: unrepl [(str: Exclusive seq: 1)]
+
+dequeue r=req1
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: unrepl [(str: Exclusive seq: 1)]
+
+
+new-request r=req2 txn=txn1 ts=10,1 spans=shared@a
+----
+
+scan r=req2
+----
+start-waiting: false
+
+acquire r=req2 k=a durability=r strength=shared
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: repl [Shared], unrepl [(str: Exclusive seq: 1)]
+
+dequeue r=req2
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: repl [Shared], unrepl [(str: Exclusive seq: 1)]
+
+
+new-request r=req3 txn=txn1 ts=10,1 spans=intent@a
+----
+
+scan r=req3
+----
+start-waiting: false
+
+acquire r=req3 k=a durability=r strength=intent
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: repl [Intent, Shared], unrepl [(str: Exclusive seq: 1)]
+   queued locking requests:
+    active: false req: 3 promoting: true, strength: Intent, txn: 00000000-0000-0000-0000-000000000001
+
+dequeue r=req3
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,1, info: repl [Intent, Shared], unrepl [(str: Exclusive seq: 1)]
+
+# Update Epoch but not the WriteTimestamp like an IntentMissingError.
+update-txn-not-observed txn=txn1 ts=10,1 epoch=1 seq=1
+----
+
+new-request r=req4 txn=txn1 ts=10,1 spans=intent@a
+----
+
+scan r=req4
+----
+start-waiting: false
+
+acquire r=req4 k=a durability=r strength=intent
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 1, iso: Serializable, ts: 10.000000000,1, info: repl [Intent, Shared]
+
+dequeue r=req4
+----
+num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 1, iso: Serializable, ts: 10.000000000,1, info: repl [Intent, Shared]