Merge #151305 #151326

151305: sql/tests: for now skip st_snap in TestRandomSyntaxFunctions r=yuzefovich a=yuzefovich

There appears to be a regression with geos bump from 3.11 to 3.12. It's not fixed in 3.13 either (latest stable available release), so until we fix it proper, skip `st_snap` function.

Informs: #151103.
Epic: None

Release note: None

151326: mmaprototype: fix missing change id bug r=sumeerbhola a=wenyihu6

Previously, processStoreLeaseholderMsgInternal iterated over the slice
rs.pendingChanges while modifying it in-place by swapping elements to the back
and shortening the slice. Since the slice header is copied at the start of a
range loop, this led to attempts to delete changes that are technically already
out of the length had already been removed. This commit fixes the issue by
copying the change IDs before the loop to avoid iterating over a mutating slice.

Epic: none
Release note: none

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: wenyihu6 <[email protected]>

Author: 3 people

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

@@ -1377,8 +1377,12 @@ func (cs *clusterState) processStoreLeaseholderMsgInternal(
 		// Since this range is going away, mark all the pending changes as
 		// enacted. This will allow the load adjustments to also be garbage
 		// collected in the future.
-		for _, change := range rs.pendingChanges {
-			cs.pendingChangeEnacted(change.ChangeID, now, true)
+		changeIDs := make([]ChangeID, len(rs.pendingChanges))
+		for i, change := range rs.pendingChanges {
+			changeIDs[i] = change.ChangeID
+		}
+		for _, changeID := range changeIDs {
+			cs.pendingChangeEnacted(changeID, now, true)
 		}
 		// Remove from the storeStates.
 		for _, replica := range rs.replicas {

pkg/kv/kvserver/allocator/mmaprototype/testdata/cluster_state/removed_ranges

@@ -0,0 +1,57 @@
+# This test tests an edge case where the leaseholder is removed from the range.
+# Regression test for a bug where processStoreLeaseholderMsgInternal was
+# iterating over the pending changes and removing them inside the loop.
+set-store
+  store-id=1 node-id=1 attrs=purple locality-tiers=region=us-west-1,zone=us-west-1a
+  store-id=2 node-id=2 attrs=yellow locality-tiers=region=us-east-1,zone=us-east-1a
+----
+node-id=1 failure-summary=ok locality-tiers=region=us-west-1,zone=us-west-1a,node=1
+  store-id=1 membership=full attrs=purple locality-code=1:2:3:
+node-id=2 failure-summary=ok locality-tiers=region=us-east-1,zone=us-east-1a,node=2
+  store-id=2 membership=full attrs=yellow locality-code=4:5:6:
+
+store-leaseholder-msg 
+store-id=1
+  range-id=1 load=[80,80,80] raft-cpu=20 config=(num_replicas=3 constraints={'+region=us-west-1:1'} voter_constraints={'+region=us-west-1:1'})
+    store-id=1 replica-id=1 type=VOTER_FULL leaseholder=true
+----
+
+ranges
+----
+range-id=1 load=[cpu:80, write-bandwidth:80, byte-size:80] raft-cpu=20
+  store-id=1 replica-id=1 type=VOTER_FULL leaseholder=true
+
+make-pending-changes range-id=1
+  rebalance-replica: remove-store-id=1 add-store-id=2
+----
+pending(2)
+change-id=1 store-id=2 node-id=2 range-id=1 load-delta=[cpu:88, write-bandwidth:88, byte-size:88] start=0s
+  prev=(replica-id=none type=VOTER_FULL)
+  next=(replica-id=unknown type=VOTER_FULL leaseholder=true)
+change-id=2 store-id=1 node-id=1 range-id=1 load-delta=[cpu:-80, write-bandwidth:-80, byte-size:-80] start=0s
+  prev=(replica-id=1 type=VOTER_FULL leaseholder=true)
+  next=(replica-id=none type=VOTER_FULL)
+
+# Advance time to 1s so that we can see pending changes for removed ranges are
+# properly enacted.
+tick seconds=1
+----
+t=1s
+
+store-leaseholder-msg 
+store-id=1
+----
+
+# The pending changes are enacted with enactedAtTime set to 1s.
+get-pending-changes
+----
+pending(2)
+change-id=1 store-id=2 node-id=2 range-id=1 load-delta=[cpu:88, write-bandwidth:88, byte-size:88] start=0s enacted=1s
+  prev=(replica-id=none type=VOTER_FULL)
+  next=(replica-id=unknown type=VOTER_FULL leaseholder=true)
+change-id=2 store-id=1 node-id=1 range-id=1 load-delta=[cpu:-80, write-bandwidth:-80, byte-size:-80] start=0s enacted=1s
+  prev=(replica-id=1 type=VOTER_FULL leaseholder=true)
+  next=(replica-id=none type=VOTER_FULL)
+
+ranges
+----

pkg/sql/tests/rsg_test.go

@@ -435,6 +435,9 @@ func TestRandomSyntaxFunctions(t *testing.T) {
 					"crdb_internal.fingerprint":
 					// Skipped due to long execution time.
 					continue
+				case "st_snap":
+					// TODO(#151103): unskip st_snap.
+					continue
 				}
 				_, variations := builtinsregistry.GetBuiltinProperties(name)
 				for _, builtin := range variations {