Merge #102775

102775: concurrency: cleanup lock table datadriven tests r=nvanbenschoten a=arulajmani

See individual commits for details. 

Last bits of cleanup in support of #102008; the linked issue can be considered completed once this PR lands. 

Co-authored-by: Arul Ajmani <[email protected]>

pkg/kv/kvserver/concurrency/lock_table.go

@@ -21,7 +21,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/lockspanset"
-	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
@@ -137,7 +136,7 @@ func (s waitingState) String() string {
 // - metrics about lockTable state to export to observability debug pages:
 //   number of locks, number of waiting requests, wait time?, ...
 
-// The btree for a particular SpanScope.
+// The btree for a particular key.
 type treeMu struct {
 	mu syncutil.RWMutex // Protects everything in this struct.
 
@@ -276,7 +275,7 @@ func newLockTable(maxLocks int64, rangeID roachpb.RangeID, clock *hlc.Clock) *lo
 
 func (t *lockTableImpl) setMaxLocks(maxLocks int64) {
 	// Check at 5% intervals of the max count.
-	lockAddMaxLocksCheckInterval := maxLocks / (int64(spanset.NumSpanScope) * 20)
+	lockAddMaxLocksCheckInterval := maxLocks / int64(20)
 	if lockAddMaxLocksCheckInterval == 0 {
 		lockAddMaxLocksCheckInterval = 1
 	}
@@ -974,6 +973,11 @@ type lockWaitQueue struct {
 	//   since those are also shared lockers. In that case it will depend on the
 	//   first waiter since that waiter must be desiring a lock that is
 	//   incompatible with a shared lock.
+	//
+	// TODO(arul): The paragraph above still talks about declaring access on keys
+	// in terms of SpanAccess instead of lock strength. Switch over this verbiage
+	// to reference locking/non-locking requests once we support multiple lock
+	// strengths and add support for joint reservations.
 
 	reservation *lockTableGuardImpl
 
@@ -1673,10 +1677,10 @@ func (l *lockState) tryActiveWait(
 			// Already reserved by this request.
 			return false, false
 		}
-		// A non-transactional write request never makes or breaks reservations,
-		// and only waits for a reservation if the reservation has a lower
-		// seqNum. Note that `sa == spanset.SpanRead && lockHolderTxn == nil`
-		// was already checked above.
+		// A non-transactional write request never makes or breaks reservations, and
+		// only waits for a reservation if the reservation has a lower seqNum. Note
+		// that `str == lock.None && lockHolderTxn == nil` was already checked
+		// above.
 		if g.txn == nil && l.reservation.seqNum > g.seqNum {
 			// Reservation is held by a request with a higher seqNum and g is a
 			// non-transactional request. Ignore the reservation.
@@ -2790,34 +2794,30 @@ func (t *lockTableImpl) lockCountForTesting() int64 {
 // Waiters of removed locks are told to wait elsewhere or that they are done
 // waiting.
 func (t *lockTableImpl) tryClearLocks(force bool, numToClear int) {
-	done := false
 	clearCount := 0
-	for i := 0; i < int(spanset.NumSpanScope) && !done; i++ {
-		t.locks.mu.Lock()
-		var locksToClear []*lockState
-		iter := t.locks.MakeIter()
-		for iter.First(); iter.Valid(); iter.Next() {
-			l := iter.Cur()
-			if l.tryClearLock(force) {
-				locksToClear = append(locksToClear, l)
-				clearCount++
-				if !force && clearCount >= numToClear {
-					done = true
-					break
-				}
+	t.locks.mu.Lock()
+	var locksToClear []*lockState
+	iter := t.locks.MakeIter()
+	for iter.First(); iter.Valid(); iter.Next() {
+		l := iter.Cur()
+		if l.tryClearLock(force) {
+			locksToClear = append(locksToClear, l)
+			clearCount++
+			if !force && clearCount >= numToClear {
+				break
 			}
 		}
-		atomic.AddInt64(&t.locks.numLocks, int64(-len(locksToClear)))
-		if t.locks.Len() == len(locksToClear) {
-			// Fast-path full clear.
-			t.locks.Reset()
-		} else {
-			for _, l := range locksToClear {
-				t.locks.Delete(l)
-			}
+	}
+	atomic.AddInt64(&t.locks.numLocks, int64(-len(locksToClear)))
+	if t.locks.Len() == len(locksToClear) {
+		// Fast-path full clear.
+		t.locks.Reset()
+	} else {
+		for _, l := range locksToClear {
+			t.locks.Delete(l)
 		}
-		t.locks.mu.Unlock()
 	}
+	t.locks.mu.Unlock()
 }
 
 // findHighestLockStrengthInSpans returns the highest lock strength specified

pkg/kv/kvserver/concurrency/lock_table_test.go

@@ -64,7 +64,7 @@ new-txn txn=<name> ts=<int>[,<int>] epoch=<int> [seq=<int>]
 
  Creates a TxnMeta.
 
-new-request r=<name> txn=<name>|none ts=<int>[,<int>] spans=r|w@<start>[,<end>]+... [skip-locked] [max-lock-wait-queue-length=<int>]
+new-request r=<name> txn=<name>|none ts=<int>[,<int>] spans=none|shared|update|exclusive|intent@<start>[,<end>]+... [skip-locked] [max-lock-wait-queue-length=<int>]
 ----
 
  Creates a Request.
@@ -112,7 +112,7 @@ add-discovered r=<name> k=<key> txn=<name> [lease-seq=<seq>] [consult-finalized-
 
  Adds a discovered lock that is discovered by the named request.
 
-check-opt-no-conflicts r=<name> spans=r|w@<start>[,<end>]+...
+check-opt-no-conflicts r=<name> spans=none|shared|update|exclusive|intent@<start>[,<end>]+...
 ----
 no-conflicts: <bool>
 
@@ -578,21 +578,8 @@ func TestLockTableBasic(t *testing.T) {
 				if txnS == "" {
 					txnS = fmt.Sprintf("unknown txn with ID: %v", state.txn.ID)
 				}
-				// TODO(arul): We're translating the lock strength back to guardAccess
-				// for now to reduce test churn. A followup patch should teach these
-				// datadriven tests to use lock.Strength correctly -- both in its input
-				// and output.
-				var sa spanset.SpanAccess
-				switch state.guardStrength {
-				case lock.None:
-					sa = spanset.SpanReadOnly
-				case lock.Intent:
-					sa = spanset.SpanReadWrite
-				default:
-					t.Fatalf("unexpected guard strength %s", state.guardStrength)
-				}
-				return fmt.Sprintf("%sstate=%s txn=%s key=%s held=%t guard-access=%s",
-					str, typeStr, txnS, state.key, state.held, sa)
+				return fmt.Sprintf("%sstate=%s txn=%s key=%s held=%t guard-strength=%s",
+					str, typeStr, txnS, state.key, state.held, state.guardStrength)
 
 			case "resolve-before-scanning":
 				var reqName string
@@ -710,35 +697,38 @@ func scanSpans(
 	lockSpans := &lockspanset.LockSpanSet{}
 	var spansStr string
 	d.ScanArgs(t, "spans", &spansStr)
-	parts := strings.Split(spansStr, "+")
-	for _, p := range parts {
-		if len(p) < 2 || p[1] != '@' {
-			d.Fatalf(t, "incorrect span with access format: %s", p)
+	lockSpanStrs := strings.Split(spansStr, "+")
+	for _, lockSpanStr := range lockSpanStrs {
+		parts := strings.Split(lockSpanStr, "@")
+		if len(parts) != 2 {
+			d.Fatalf(t, "incorrect span with strength format: %s", parts)
 		}
-		c := p[0]
-		p = p[2:]
+		strS := parts[0]
+		spanStr := parts[1]
+		str := getStrength(t, d, strS)
+		// Compute latch span access based on the supplied strength.
 		var sa spanset.SpanAccess
-		var str lock.Strength
-		// TODO(arul): Switch the datadriven input to use lock strengths instead.
-		switch c {
-		case 'r':
+		switch str {
+		case lock.None:
 			sa = spanset.SpanReadOnly
-			str = lock.None
-		case 'w':
+		case lock.Intent:
 			sa = spanset.SpanReadWrite
-			str = lock.Intent
 		default:
-			d.Fatalf(t, "incorrect span access: %c", c)
+			d.Fatalf(t, "unsupported lock strength: %s", str)
 		}
-		latchSpans.AddMVCC(sa, getSpan(t, d, p), ts)
-		lockSpans.Add(str, getSpan(t, d, p))
+		latchSpans.AddMVCC(sa, getSpan(t, d, spanStr), ts)
+		lockSpans.Add(str, getSpan(t, d, spanStr))
 	}
 	return latchSpans, lockSpans
 }
 
 func ScanLockStrength(t *testing.T, d *datadriven.TestData) lock.Strength {
 	var strS string
 	d.ScanArgs(t, "strength", &strS)
+	return getStrength(t, d, strS)
+}
+
+func getStrength(t *testing.T, d *datadriven.TestData, strS string) lock.Strength {
 	switch strS {
 	case "none":
 		return lock.None
@@ -748,6 +738,8 @@ func ScanLockStrength(t *testing.T, d *datadriven.TestData) lock.Strength {
 		return lock.Update
 	case "exclusive":
 		return lock.Exclusive
+	case "intent":
+		return lock.Intent
 	default:
 		d.Fatalf(t, "unknown lock strength: %s", strS)
 		return 0

pkg/kv/kvserver/concurrency/testdata/lock_table/acquire_idempotency

@@ -14,7 +14,7 @@ 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=w@a
+new-request r=req1 txn=txn1 ts=10,1 spans=intent@a
 ----
 
 scan r=req1
@@ -36,7 +36,7 @@ num=1
 new-txn txn=txn1 ts=10,1 epoch=0 seq=2
 ----
 
-new-request r=req2 txn=txn1 ts=10,1 spans=w@a
+new-request r=req2 txn=txn1 ts=10,1 spans=intent@a
 ----
 
 scan r=req2
@@ -58,7 +58,7 @@ num=1
 new-txn txn=txn1 ts=10,1 epoch=0 seq=4
 ----
 
-new-request r=req3 txn=txn1 ts=10,1 spans=w@a
+new-request r=req3 txn=txn1 ts=10,1 spans=intent@a
 ----
 
 scan r=req3
@@ -84,7 +84,7 @@ num=1
 new-txn txn=txn1 ts=10,1 epoch=0 seq=4
 ----
 
-new-request r=req3 txn=txn1 ts=10,1 spans=w@a
+new-request r=req3 txn=txn1 ts=10,1 spans=intent@a
 ----
 
 scan r=req3
@@ -110,7 +110,7 @@ num=1
 new-txn txn=txn1 ts=10,1 epoch=0 seq=2
 ----
 
-new-request r=req4 txn=txn1 ts=10,1 spans=w@a
+new-request r=req4 txn=txn1 ts=10,1 spans=intent@a
 ----
 
 scan r=req4
@@ -138,7 +138,7 @@ num=1
 new-txn txn=txn1 ts=10,1 epoch=0 seq=3
 ----
 
-new-request r=req5 txn=txn1 ts=10,1 spans=w@a
+new-request r=req5 txn=txn1 ts=10,1 spans=intent@a
 ----
 
 scan r=req5
@@ -164,7 +164,7 @@ num=1
 new-txn txn=txn1 ts=10,1 epoch=0 seq=5
 ----
 
-new-request r=req6 txn=txn1 ts=10,1 spans=w@a
+new-request r=req6 txn=txn1 ts=10,1 spans=intent@a
 ----
 
 scan r=req6

pkg/kv/kvserver/concurrency/testdata/lock_table/add_discovered

@@ -31,7 +31,7 @@ new-txn txn=txn2 ts=10 epoch=0
 new-txn txn=txn3 ts=10 epoch=0
 ----
 
-new-request r=req1 txn=txn1 ts=10,1 spans=w@a
+new-request r=req1 txn=txn1 ts=10,1 spans=intent@a
 ----
 
 scan r=req1
@@ -50,10 +50,10 @@ num=1
  lock: "a"
   holder: txn: 00000000-0000-0000-0000-000000000001, ts: 10.000000000,1, info: unrepl epoch: 0, seqs: [0]
 
-new-request r=req2 txn=txn2 ts=10,1 spans=w@a
+new-request r=req2 txn=txn2 ts=10,1 spans=intent@a
 ----
 
-new-request r=req3 txn=txn3 ts=10,1 spans=w@a
+new-request r=req3 txn=txn3 ts=10,1 spans=intent@a
 ----
 
 scan r=req2
@@ -79,7 +79,7 @@ new: state=doneWaiting
 
 guard-state r=req3
 ----
-new: state=waitForDistinguished txn=txn2 key="a" held=false guard-access=write
+new: state=waitForDistinguished txn=txn2 key="a" held=false guard-strength=Intent
 
 # --------------------------------
 # Setup complete, test starts here
@@ -99,7 +99,7 @@ start-waiting: true
 
 guard-state r=req2
 ----
-new: state=waitForDistinguished txn=txn3 key="a" held=true guard-access=write
+new: state=waitForDistinguished txn=txn3 key="a" held=true guard-strength=Intent
 
 guard-state r=req3
 ----

pkg/kv/kvserver/concurrency/testdata/lock_table/add_discovered_old_lease

@@ -7,7 +7,7 @@ new-txn txn=txn1 ts=10 epoch=0
 new-txn txn=txn2 ts=10 epoch=0
 ----
 
-new-request r=req1 txn=txn1 ts=10 spans=w@a+r@b+w@c
+new-request r=req1 txn=txn1 ts=10 spans=intent@a+none@b+intent@c
 ----
 
 clear disable