Merge pull request #47139 from nvanbenschoten/backport20.1-47101

release-20.1: kv/concurrency: permit lock timestamp regression across durabilities

pkg/kv/kvserver/concurrency/concurrency_manager_test.go

@@ -60,7 +60,7 @@ import (
 // handle-write-intent-error  req=<req-name> txn=<txn-name> key=<key>
 // handle-txn-push-error      req=<req-name> txn=<txn-name> key=<key>  TODO(nvanbenschoten): implement this
 //
-// on-lock-acquired  req=<req-name> key=<key> [seq=<seq>]
+// on-lock-acquired  req=<req-name> key=<key> [seq=<seq>] [dur=r|u]
 // on-lock-updated   req=<req-name> txn=<txn-name> key=<key> status=[committed|aborted|pending] [ts=<int>[,<int>]]
 // on-txn-updated    txn=<txn-name> status=[committed|aborted|pending] [ts=<int>[,<int>]]
 //
@@ -276,6 +276,11 @@ func TestConcurrencyManagerBasic(t *testing.T) {
 				}
 				seqNum := enginepb.TxnSeq(seq)
 
+				dur := lock.Unreplicated
+				if d.HasArg("dur") {
+					dur = scanLockDurability(t, d)
+				}
+
 				// Confirm that the request has a corresponding write request.
 				found := false
 				for _, ru := range guard.Req.Requests {
@@ -298,7 +303,7 @@ func TestConcurrencyManagerBasic(t *testing.T) {
 				mon.runSync("acquire lock", func(ctx context.Context) {
 					log.Eventf(ctx, "txn %s @ %s", txn.ID.Short(), key)
 					span := roachpb.Span{Key: roachpb.Key(key)}
-					up := roachpb.MakeLockUpdateWithDur(txnAcquire, span, lock.Unreplicated)
+					up := roachpb.MakeLockUpdateWithDur(txnAcquire, span, dur)
 					m.OnLockAcquired(ctx, &up)
 				})
 				return c.waitAndCollect(t, mon)

pkg/kv/kvserver/concurrency/datadriven_util_test.go

@@ -15,6 +15,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -58,6 +59,20 @@ func scanTimestampWithName(t *testing.T, d *datadriven.TestData, name string) hl
 	return ts
 }
 
+func scanLockDurability(t *testing.T, d *datadriven.TestData) lock.Durability {
+	var durS string
+	d.ScanArgs(t, "dur", &durS)
+	switch durS {
+	case "r":
+		return lock.Replicated
+	case "u":
+		return lock.Unreplicated
+	default:
+		d.Fatalf(t, "unknown lock durability: %s", durS)
+		return 0
+	}
+}
+
 func scanSingleRequest(
 	t *testing.T, d *datadriven.TestData, line string, txns map[string]*roachpb.Transaction,
 ) roachpb.Request {

pkg/kv/kvserver/concurrency/lock_table.go

@@ -1179,16 +1179,35 @@ func (l *lockState) acquireLock(
 		//  - txn A's coordinator does not immediately learn of the push
 		//  - txn A re-acquires lock at sequence 2, ts 15
 		//
-		// A lock's timestamp cannot be allowed to regress, so by forwarding
-		// its timestamp during the second acquisition instead if assigning
-		// to it blindly, it remains at 20.
+		// A lock's timestamp at a given durability level is not allowed to
+		// regress, so by forwarding its timestamp during the second acquisition
+		// instead if assigning to it blindly, it remains at 20.
+		//
+		// However, a lock's timestamp as reported by getLockerInfo can regress
+		// if it is acquired at a lower timestamp and a different durability
+		// than it was previously held with. This is necessary to support
+		// because the hard constraint which we must uphold here that the
+		// lockHolderInfo for a replicated lock cannot diverge from the
+		// replicated state machine in such a way that its timestamp in the
+		// lockTable exceeds that in the replicated keyspace. If this invariant
+		// were to be violated, we'd risk infinite lock-discovery loops for
+		// requests that conflict with the lock as is written in the replicated
+		// state machine but not as is reflected in the lockTable.
+		//
+		// Lock timestamp regressions are safe from the perspective of other
+		// transactions because the request which re-acquired the lock at the
+		// lower timestamp must have been holding a write latch at or below the
+		// new lock's timestamp. This means that no conflicting requests could
+		// be evaluating concurrently. Instead, all will need to re-scan the
+		// lockTable once they acquire latches and will notice the reduced
+		// timestamp at that point, which may cause them to conflict with the
+		// lock even if they had not conflicted before. In a sense, it is no
+		// different than the first time a lock is added to the lockTable.
 		l.holder.holder[durability].ts.Forward(ts)
 		l.holder.holder[durability].seqs = append(seqs, txn.Sequence)
 
 		_, afterTs, _ := l.getLockerInfo()
-		if afterTs.Less(beforeTs) {
-			panic("lockTable bug - lock timestamp regression")
-		} else if beforeTs.Less(afterTs) {
+		if beforeTs.Less(afterTs) {
 			l.increasedLockTs(afterTs)
 		}
 		return nil

pkg/kv/kvserver/concurrency/testdata/concurrency_manager/update

@@ -5,7 +5,7 @@
 # still at the original timestamp. This is permitted but the
 # lock's timestamp should not regress.
 #
-# Setup: txn1 acquire lock k
+# Setup: txn1 acquires lock k
 #        txn2 reads k and waits
 #        txn2 pushes txn1
 #
@@ -124,7 +124,7 @@ reset namespace
 # the original timestamp. This is permitted but the lock's
 # timestamp should not regress.
 #
-# Setup: txn1 acquire lock k
+# Setup: txn1 acquires lock k
 #        txn2 reads k and waits
 #
 # Test:  txn2 pushes txn1's timestamp forward
@@ -238,3 +238,127 @@ local: num=0
 
 reset namespace
 ----
+
+# -------------------------------------------------------------
+# A transaction acquires an unreplicated lock. The lock is
+# pushed to a higher timestamp by a second transaction. The
+# transaction then returns to upgrade the unreplicated lock to a
+# replicated intent at a new sequence number but still at the
+# original timestamp. This is permitted and the lock's timestamp
+# regresses back down to the intent's timestamp. In practice, if
+# the pusher wanted to prevent its push from being reverted, it
+# should have also bumped the timestamp cache to ensure that the
+# intent couldn't be laid down at the original timestamp.
+#
+# Setup: txn1 acquires unreplicated lock k
+#        txn2 reads k and waits
+#        txn2 pushes txn1
+#
+# Test:  txn2 succeeds in pushing txn1's ts forward
+#        txn2 proceeds
+#        txn1 re-acquires replicated lock k at lower ts
+# -------------------------------------------------------------
+
+new-txn name=txn1 ts=10,1 epoch=0
+----
+
+new-txn name=txn2 ts=12,1 epoch=0
+----
+
+new-request name=req1 txn=txn1 ts=10,1
+  put key=k value=v
+----
+
+new-request name=req2 txn=txn2 ts=12,1
+  get key=k
+----
+
+sequence req=req1
+----
+[1] sequence req1: sequencing request
+[1] sequence req1: acquiring latches
+[1] sequence req1: scanning lock table for conflicting locks
+[1] sequence req1: sequencing complete, returned guard
+
+on-lock-acquired req=req1 key=k dur=u
+----
+[-] acquire lock: txn 00000001 @ k
+
+finish req=req1
+----
+[-] finish req1: finishing request
+
+sequence req=req2
+----
+[2] sequence req2: sequencing request
+[2] sequence req2: acquiring latches
+[2] sequence req2: scanning lock table for conflicting locks
+[2] sequence req2: waiting in lock wait-queues
+[2] sequence req2: pushing timestamp of txn 00000001 above 0.000000012,1
+[2] sequence req2: blocked on select in concurrency_test.(*cluster).PushTransaction
+
+debug-lock-table
+----
+global: num=1
+ lock: "k"
+  holder: txn: 00000001-0000-0000-0000-000000000000, ts: 0.000000010,1, info: unrepl epoch: 0, seqs: [0]
+   waiting readers:
+    req: 8, txn: 00000002-0000-0000-0000-000000000000
+   distinguished req: 8
+local: num=0
+
+# --------------------------------
+# Setup complete, test starts here
+# --------------------------------
+
+on-txn-updated txn=txn1 status=pending ts=12,2
+----
+[-] update txn: increasing timestamp of txn1
+[2] sequence req2: resolving intent "k" for txn 00000001 with PENDING status
+[2] sequence req2: acquiring latches
+[2] sequence req2: scanning lock table for conflicting locks
+[2] sequence req2: sequencing complete, returned guard
+
+finish req=req2
+----
+[-] finish req2: finishing request
+
+debug-lock-table
+----
+global: num=1
+ lock: "k"
+  holder: txn: 00000001-0000-0000-0000-000000000000, ts: 0.000000012,2, info: unrepl epoch: 0, seqs: [0]
+local: num=0
+
+# Issue another write to the same key for txn1 at its initial timestamp,
+# this time with a replicated durability. The timestamp in the lock
+# table should regress back down to reflect the replicated lock state.
+
+new-request name=req3 txn=txn1 ts=10,1
+  put key=k value=v2 seq=1
+----
+
+sequence req=req3
+----
+[3] sequence req3: sequencing request
+[3] sequence req3: acquiring latches
+[3] sequence req3: scanning lock table for conflicting locks
+[3] sequence req3: sequencing complete, returned guard
+
+on-lock-acquired req=req3 key=k seq=1 dur=r
+----
+[-] acquire lock: txn 00000001 @ k
+
+finish req=req3
+----
+[-] finish req3: finishing request
+
+debug-lock-table
+----
+global: num=1
+ lock: "k"
+  holder: txn: 00000001-0000-0000-0000-000000000000, ts: 0.000000010,1, info: repl epoch: 0, seqs: [1], unrepl epoch: 0, seqs: [0]
+local: num=0
+
+reset namespace
+----

pkg/kv/kvserver/concurrency/testdata/lock_table/lock_changes

@@ -49,6 +49,19 @@ global: num=1
   holder: txn: 00000000-0000-0000-0000-000000000001, ts: 0.000000010,0, info: unrepl epoch: 0, seqs: [2, 3]
 local: num=0
 
+# ---------------------------------------------------------------------------------
+# Lock is reacquired at same epoch with lower timestamp and different durability.
+# The lock's timestamp is allowed to regress in this case, because it must never
+# diverge from the replicated state machine.
+# ---------------------------------------------------------------------------------
+
+acquire r=req2 k=a durability=r
+----
+global: num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001, ts: 0.000000008,0, info: repl epoch: 0, seqs: [3], unrepl epoch: 0, seqs: [2, 3]
+local: num=0
+
 # ---------------------------------------------------------------------------------
 # Lock is reacquired at a different epoch. The old sequence numbers are discarded.
 # ---------------------------------------------------------------------------------
@@ -63,7 +76,7 @@ acquire r=req3 k=a durability=u
 ----
 global: num=1
  lock: "a"
-  holder: txn: 00000000-0000-0000-0000-000000000001, ts: 0.000000010,0, info: unrepl epoch: 1, seqs: [0]
+  holder: txn: 00000000-0000-0000-0000-000000000001, ts: 0.000000008,0, info: repl epoch: 0, seqs: [3], unrepl epoch: 1, seqs: [0]
 local: num=0
 
 # ---------------------------------------------------------------------------------
@@ -72,17 +85,17 @@ local: num=0
 # discarded but the timestamp is not regressed.
 # ---------------------------------------------------------------------------------
 
-new-txn txn=txn1 ts=8 epoch=2 seq=0
+new-txn txn=txn1 ts=6 epoch=2 seq=0
 ----
 
-new-request r=req4 txn=txn1 ts=8 spans=w@a
+new-request r=req4 txn=txn1 ts=6 spans=w@a
 ----
 
 acquire r=req4 k=a durability=u
 ----
 global: num=1
  lock: "a"
-  holder: txn: 00000000-0000-0000-0000-000000000001, ts: 0.000000010,0, info: unrepl epoch: 2, seqs: [0]
+  holder: txn: 00000000-0000-0000-0000-000000000001, ts: 0.000000008,0, info: repl epoch: 0, seqs: [3], unrepl epoch: 2, seqs: [0]
 local: num=0
 
 # ---------------------------------------------------------------------------------
@@ -98,13 +111,13 @@ start-waiting: true
 
 guard-state r=req5
 ----
-new: state=waitForDistinguished txn=txn1 ts=10 key="a" held=true guard-access=read
+new: state=waitForDistinguished txn=txn1 ts=8 key="a" held=true guard-access=read
 
 print
 ----
 global: num=1
  lock: "a"
-  holder: txn: 00000000-0000-0000-0000-000000000001, ts: 0.000000010,0, info: unrepl epoch: 2, seqs: [0]
+  holder: txn: 00000000-0000-0000-0000-000000000001, ts: 0.000000008,0, info: repl epoch: 0, seqs: [3], unrepl epoch: 2, seqs: [0]
    waiting readers:
     req: 1, txn: 00000000-0000-0000-0000-000000000002
    distinguished req: 1
@@ -116,11 +129,25 @@ new-txn txn=txn1 ts=14 epoch=1 seq=1
 new-request r=req6 txn=txn1 ts=14 spans=w@a
 ----
 
+acquire r=req6 k=a durability=r
+----
+global: num=1
+ lock: "a"
+  holder: txn: 00000000-0000-0000-0000-000000000001, ts: 0.000000010,0, info: repl epoch: 1, seqs: [1], unrepl epoch: 2, seqs: [0]
+   waiting readers:
+    req: 1, txn: 00000000-0000-0000-0000-000000000002
+   distinguished req: 1
+local: num=0
+
+guard-state r=req5
+----
+old: state=waitForDistinguished txn=txn1 ts=8 key="a" held=true guard-access=read
+
 acquire r=req6 k=a durability=u
 ----
 global: num=1
  lock: "a"
-  holder: txn: 00000000-0000-0000-0000-000000000001, ts: 0.000000014,0, info: unrepl epoch: 1, seqs: [0, 1]
+  holder: txn: 00000000-0000-0000-0000-000000000001, ts: 0.000000014,0, info: repl epoch: 1, seqs: [1], unrepl epoch: 1, seqs: [0, 1]
 local: num=0
 
 guard-state r=req5