Merge #158164 #158277

158164: concurrency: use the correct lock strength for waiting requests r=arulajmani a=arulajmani

Previously, we incorrectly assumed every locking request waiting in the lock table was doing so with Exclusive lock strength when returning output for the cluster locks table.

Epic: none

Release note: None

158277: mma: fix todos related to is-leaseholder assertions r=wenyihu6 a=sumeerbhola

The existing assertions were valid, but needed better commentary. Also added a new assertion.

Epic: CRDB-55052

Release note: None

Co-authored-by: Arul Ajmani <arulajmani@gmail.com>
Co-authored-by: sumeerbhola <sumeer@cockroachlabs.com>

Author: 3 people

pkg/ccl/logictestccl/testdata/logic_test/cluster_locks_tenant

@@ -203,7 +203,7 @@ query TTTTTTTBB colnames,rowsort
 SELECT database_name, schema_name, table_name, lock_key_pretty, lock_strength, durability, isolation_level, granted, contended FROM crdb_internal.cluster_locks WHERE table_name='t' AND txn_id='$txn4'
 ----
 database_name  schema_name  table_name  lock_key_pretty     lock_strength  durability    isolation_level  granted  contended
-test           public       t           /Table/106/1/"b"/0  Exclusive      Unreplicated  SERIALIZABLE     false    true
+test           public       t           /Table/106/1/"b"/0  Intent         Unreplicated  SERIALIZABLE     false    true
 
 query I
 SELECT count(*) FROM crdb_internal.cluster_locks WHERE table_name = 't'

pkg/ccl/logictestccl/testdata/logic_test/cluster_locks_tenant_write_buffering

@@ -206,7 +206,7 @@ query TTTTTTTBB colnames,rowsort
 SELECT database_name, schema_name, table_name, lock_key_pretty, lock_strength, durability, isolation_level, granted, contended FROM crdb_internal.cluster_locks WHERE table_name='t' AND txn_id='$txn4'
 ----
 database_name  schema_name  table_name  lock_key_pretty     lock_strength  durability    isolation_level  granted  contended
-test           public       t           /Table/106/1/"b"/0  Exclusive      Unreplicated  SERIALIZABLE     false    true
+test           public       t           /Table/106/1/"b"/0  Intent         Unreplicated  SERIALIZABLE     false    true
 
 query I
 SELECT count(*) FROM crdb_internal.cluster_locks WHERE table_name = 't'

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_rebalance_stores.go

@@ -524,8 +524,12 @@ func (re *rebalanceEnv) rebalanceLeasesFromLocalStoreID(
 		store.StoreID, store.dimSummary[CPURate], overloadSlow)
 	// This store is local, and cpu overloaded. Shed leases first.
 	//
-	// NB: any ranges at this store that don't have pending changes must
-	// have this local store as the leaseholder.
+	// NB: due to REQUIREMENT(change-computation), the top-k ranges for ss
+	// reflect the latest adjusted state, including pending changes. Thus,
+	// this store must be a replica and the leaseholder, which is asserted
+	// below. The code below additionally ignores the range if it has pending
+	// changes, which while not necessary for the is-leaseholder assertion,
+	// makes the case where we assert even narrower.
 	topKRanges := ss.adjusted.topKRanges[localStoreID]
 	var leaseTransferCount int
 	n := topKRanges.len()
@@ -548,37 +552,38 @@ func (re *rebalanceEnv) rebalanceLeasesFromLocalStoreID(
 			log.KvDistribution.VEventf(ctx, 2, "skipping r%d: has pending changes", rangeID)
 			continue
 		}
+		foundLocalReplica := false
 		for _, repl := range rstate.replicas {
 			if repl.StoreID != localStoreID { // NB: localStoreID == ss.StoreID == store.StoreID
 				continue
 			}
 			if !repl.IsLeaseholder {
-				// TODO(tbg): is this true? Can't there be ranges with replicas on
-				// multiple local stores, and wouldn't this assertion fire in that
-				// case once rebalanceStores is invoked on whichever of the two
-				// stores doesn't hold the lease?
-				//
-				// TODO(tbg): see also the other assertion below (leaseholderID !=
-				// store.StoreID) which seems similar to this one.
-				log.KvDistribution.Fatalf(ctx, "internal state inconsistency: replica considered for lease shedding has no pending"+
-					" changes but is not leaseholder: %+v", rstate)
+				// NB: due to REQUIREMENT(change-computation), the top-k
+				// ranges for ss reflect the latest adjusted state, including
+				// pending changes. Thus, this store must be a replica and the
+				// leaseholder, hence this assertion, and other assertions
+				// below. Additionally, the code above ignored the range if it
+				// has pending changes, which while not necessary for the
+				// is-leaseholder assertion, makes the case where we assert
+				// even narrower.
+				log.KvDistribution.Fatalf(ctx,
+					"internal state inconsistency: replica considered for lease shedding has no pending"+
+						" changes but is not leaseholder: %+v", rstate)
 			}
+			foundLocalReplica = true
+			break
+		}
+		if !foundLocalReplica {
+			log.KvDistribution.Fatalf(
+				ctx, "internal state inconsistency: local store is not a replica: %+v", rstate)
 		}
 		if re.now.Sub(rstate.lastFailedChange) < re.lastFailedChangeDelayDuration {
 			log.KvDistribution.VEventf(ctx, 2, "skipping r%d: too soon after failed change", rangeID)
 			continue
 		}
 		re.ensureAnalyzedConstraints(rstate)
 		if rstate.constraints.leaseholderID != store.StoreID {
-			// We should not panic here since the leaseQueue may have shed the
-			// lease and informed MMA, since the last time MMA computed the
-			// top-k ranges. This is useful for debugging in the prototype, due
-			// to the lack of unit tests.
-			//
-			// TODO(tbg): can the above scenario currently happen? ComputeChanges
-			// first processes the leaseholder message and then, still under the
-			// lock, immediately calls into rebalanceStores (i.e. this store).
-			// Doesn't this mean that the leaseholder view is up to date?
+			// See the earlier comment about assertions.
 			panic(fmt.Sprintf("internal state inconsistency: "+
 				"store=%v range_id=%v should be leaseholder but isn't",
 				store.StoreID, rangeID))

pkg/kv/kvserver/concurrency/lock_table.go

@@ -2069,7 +2069,7 @@ func (kl *keyLocks) lockStateInfo(now time.Time, rangeID roachpb.RangeID) []roac
 		lockWaiters = append(lockWaiters, lock.Waiter{
 			WaitingTxn:   g.txnMeta(),
 			ActiveWaiter: qg.active,
-			Strength:     lock.Exclusive,
+			Strength:     qg.mode.Strength,
 			WaitDuration: now.Sub(g.mu.curLockWaitStart),
 		})
 		g.mu.Unlock()

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

@@ -79,8 +79,8 @@ num locks: 1, bytes returned: 79, resume reason: RESUME_UNKNOWN, resume span: <n
  locks:
   range_id=3 key="a" holder=<nil> durability=Unreplicated duration=0s
    waiters:
-    waiting_txn:00000000-0000-0000-0000-000000000002 active_waiter:false strength:Exclusive wait_duration:0s
-    waiting_txn:00000000-0000-0000-0000-000000000003 active_waiter:true strength:Exclusive wait_duration:0s
+    waiting_txn:00000000-0000-0000-0000-000000000002 active_waiter:false strength:Intent wait_duration:0s
+    waiting_txn:00000000-0000-0000-0000-000000000003 active_waiter:true strength:Intent wait_duration:0s
 
 guard-state r=req2
 ----

pkg/kv/kvserver/concurrency/testdata/lock_table/basic

@@ -710,11 +710,11 @@ num locks: 3, bytes returned: 282, resume reason: RESUME_UNKNOWN, resume span: <
   range_id=3 key="b" holder=<nil> durability=Unreplicated duration=0s
    waiters:
     waiting_txn:00000000-0000-0000-0000-000000000002 active_waiter:false strength:Exclusive wait_duration:2.65s
-    waiting_txn:00000000-0000-0000-0000-000000000001 active_waiter:true strength:Exclusive wait_duration:250ms
+    waiting_txn:00000000-0000-0000-0000-000000000001 active_waiter:true strength:Intent wait_duration:250ms
   range_id=3 key="c" holder=<nil> durability=Unreplicated duration=0s
    waiters:
     waiting_txn:00000000-0000-0000-0000-000000000002 active_waiter:false strength:Exclusive wait_duration:2.65s
-    waiting_txn:00000000-0000-0000-0000-000000000003 active_waiter:true strength:Exclusive wait_duration:0s
+    waiting_txn:00000000-0000-0000-0000-000000000003 active_waiter:true strength:Intent wait_duration:0s
 
 # 100ms passes between before releasing a
 time-tick ms=100
@@ -872,11 +872,11 @@ num locks: 3, bytes returned: 258, resume reason: RESUME_UNKNOWN, resume span: <
   range_id=3 key="b" holder=<nil> durability=Unreplicated duration=0s
    waiters:
     waiting_txn:00000000-0000-0000-0000-000000000002 active_waiter:false strength:Exclusive wait_duration:0s
-    waiting_txn:00000000-0000-0000-0000-000000000001 active_waiter:true strength:Exclusive wait_duration:350ms
+    waiting_txn:00000000-0000-0000-0000-000000000001 active_waiter:true strength:Intent wait_duration:350ms
   range_id=3 key="c" holder=<nil> durability=Unreplicated duration=0s
    waiters:
     waiting_txn:00000000-0000-0000-0000-000000000002 active_waiter:false strength:Exclusive wait_duration:0s
-    waiting_txn:00000000-0000-0000-0000-000000000003 active_waiter:true strength:Exclusive wait_duration:100ms
+    waiting_txn:00000000-0000-0000-0000-000000000003 active_waiter:true strength:Intent wait_duration:100ms
   range_id=3 key="f" holder=00000000-0000-0000-0000-000000000003 durability=Replicated duration=2.75s
    waiters:
     waiting_txn:00000000-0000-0000-0000-000000000002 active_waiter:true strength:None wait_duration:0s

pkg/kv/kvserver/concurrency/testdata/lock_table/dup_access

@@ -227,7 +227,7 @@ num locks: 1, bytes returned: 73, resume reason: RESUME_UNKNOWN, resume span: <n
  locks:
   range_id=3 key="c" holder=00000000-0000-0000-0000-000000000001 durability=Unreplicated duration=0s
    waiters:
-    waiting_txn:00000000-0000-0000-0000-000000000003 active_waiter:true strength:Exclusive wait_duration:0s
+    waiting_txn:00000000-0000-0000-0000-000000000003 active_waiter:true strength:Intent wait_duration:0s
 
 # req5 encounters the reservation by req4 at "b" when looking at it for its read access, but ignores
 # it.

pkg/kv/kvserver/concurrency/testdata/lock_table/query

@@ -151,23 +151,23 @@ num locks: 1, bytes returned: 91, resume reason: RESUME_BYTE_LIMIT, resume span:
  locks:
   range_id=3 key="b" holder=00000000-0000-0000-0000-000000000001 durability=Unreplicated duration=200ms
    waiters:
-    waiting_txn:00000000-0000-0000-0000-000000000002 active_waiter:true strength:Exclusive wait_duration:200ms
+    waiting_txn:00000000-0000-0000-0000-000000000002 active_waiter:true strength:Intent wait_duration:200ms
 
 query span=b max-bytes=100
 ----
 num locks: 1, bytes returned: 91, resume reason: RESUME_UNKNOWN, resume span: <nil>
  locks:
   range_id=3 key="b" holder=00000000-0000-0000-0000-000000000001 durability=Unreplicated duration=200ms
    waiters:
-    waiting_txn:00000000-0000-0000-0000-000000000002 active_waiter:true strength:Exclusive wait_duration:200ms
+    waiting_txn:00000000-0000-0000-0000-000000000002 active_waiter:true strength:Intent wait_duration:200ms
 
 query span=e,/Max max-bytes=100
 ----
 num locks: 1, bytes returned: 91, resume reason: RESUME_UNKNOWN, resume span: <nil>
  locks:
   range_id=3 key="e" holder=00000000-0000-0000-0000-000000000001 durability=Unreplicated duration=200ms
    waiters:
-    waiting_txn:00000000-0000-0000-0000-000000000003 active_waiter:true strength:Exclusive wait_duration:200ms
+    waiting_txn:00000000-0000-0000-0000-000000000003 active_waiter:true strength:Intent wait_duration:200ms
 
 clear
 ----

pkg/sql/logictest/testdata/logic_test/cluster_locks

@@ -268,7 +268,7 @@ query TTTTTTTBB colnames
 SELECT database_name, schema_name, table_name, lock_key_pretty, lock_strength, durability, isolation_level, granted, contended FROM crdb_internal.cluster_locks WHERE range_id=$r1 AND txn_id='$txn4'
 ----
 database_name schema_name table_name  lock_key_pretty     lock_strength   durability    isolation_level   granted contended
-test          public      t           /Table/106/1/"b"/0  Exclusive       Unreplicated  SERIALIZABLE      false   true
+test          public      t           /Table/106/1/"b"/0  Intent       Unreplicated  SERIALIZABLE      false   true
 
 query TTTTTTTBB colnames,rowsort
 SELECT database_name, schema_name, table_name, lock_key_pretty, lock_strength, durability, isolation_level, granted, contended FROM crdb_internal.cluster_locks WHERE range_id=$r2 AND txn_id='$txn3'
@@ -431,3 +431,54 @@ awaitstatement iso2
 
 statement ok
 COMMIT
+
+user root
+
+statement ok
+BEGIN;
+SELECT * FROM t WHERE k = 'a' FOR UPDATE;
+
+user testuser
+
+statement ok
+BEGIN;
+SET enable_shared_locking_for_serializable = true
+
+statement async share1
+SELECT * FROM t WHERE k = 'a' FOR SHARE;
+
+user testuser 2
+
+statement ok
+BEGIN;
+
+statement async put1
+DELETE FROM t WHERE k = 'a';
+
+user root
+
+# Verify that the waiting transaction reports Shared strength, not Exclusive.
+query TTTTTTBB colnames,retry,rowsort
+SELECT database_name, schema_name, table_name, lock_key_pretty, lock_strength, durability, granted, contended FROM crdb_internal.cluster_locks WHERE table_name = 't'
+----
+database_name  schema_name  table_name  lock_key_pretty     lock_strength  durability    granted  contended
+test           public       t           /Table/106/1/"a"/0  Exclusive      Unreplicated  true     true
+test           public       t           /Table/106/1/"a"/0  Shared         Unreplicated  false    true
+test           public       t           /Table/106/1/"a"/0  Intent         Unreplicated  false    true
+
+statement ok
+COMMIT
+
+user testuser
+
+awaitstatement share1
+
+statement ok
+COMMIT
+
+user testuser2
+
+awaitstatement put1
+
+statement ok
+ROLLBACK

pkg/sql/logictest/testdata/logic_test/cluster_locks_write_buffering

@@ -291,7 +291,7 @@ query TTTTTTTBB colnames
 SELECT database_name, schema_name, table_name, lock_key_pretty, lock_strength, durability, isolation_level, granted, contended FROM crdb_internal.cluster_locks WHERE range_id=$r1 AND txn_id='$txn4'
 ----
 database_name schema_name table_name  lock_key_pretty     lock_strength   durability    isolation_level   granted contended
-test          public      t           /Table/106/1/"b"/0  Exclusive       Unreplicated  SERIALIZABLE      false   true
+test          public      t           /Table/106/1/"b"/0  Intent       Unreplicated  SERIALIZABLE      false   true
 
 query TTTTTTTBB colnames,rowsort
 SELECT database_name, schema_name, table_name, lock_key_pretty, lock_strength, durability, isolation_level, granted, contended FROM crdb_internal.cluster_locks WHERE range_id=$r2 AND txn_id='$txn3'