Merge #159123 #159201

159123: mma: cleanup and populate counter metrics r=wenyihu6 a=sumeerbhola

These metrics use static labels representing origin={external,rebalance},
type={replica,lease}, result={success,failure}.

Epic: [CRDB-55052](https://cockroachlabs.atlassian.net/browse/CRDB-55052)

Release note: None

159201: opt: do not build placeholder scans with stable functions r=mgartner a=mgartner

In v25.4.0 we introduced an optimization for some types of generic query
plans which converts parameterized lookup joins into placeholder scans.
Prior to this commit the spans of these placeholder scans could contain
scalar expressions that were not constants nor placeholders, which
execbuilder does not support. This has been fixed by being more strict
in the application of this rule.

Fixes #159124

Release note (bug fix): A bug has been fixed which could cause prepared
statements to fail with the error message "non-const expression" when
they contained filters with stable functions. This bug has been present
since 25.4.0.


Co-authored-by: sumeerbhola <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>

Author: 3 people

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

@@ -169,6 +169,38 @@ func (a *allocatorState) ProcessStoreLoadMsg(ctx context.Context, msg *StoreLoad
 func (a *allocatorState) AdjustPendingChangeDisposition(change ExternalRangeChange, success bool) {
 	a.mu.Lock()
 	defer a.mu.Unlock()
+	metrics := a.ensureMetricsForLocalStoreLocked(change.localStoreID)
+	isLeaseTransfer := change.IsPureTransferLease()
+	switch change.origin {
+	case OriginExternal:
+		if isLeaseTransfer {
+			if success {
+				metrics.ExternalLeaseChangeSuccess.Inc(1)
+			} else {
+				metrics.ExternalLeaseChangeFailure.Inc(1)
+			}
+		} else {
+			if success {
+				metrics.ExternalReplicaChangeSuccess.Inc(1)
+			} else {
+				metrics.ExternalReplicaChangeFailure.Inc(1)
+			}
+		}
+	case originMMARebalance:
+		if isLeaseTransfer {
+			if success {
+				metrics.RebalanceLeaseChangeSuccess.Inc(1)
+			} else {
+				metrics.RebalanceLeaseChangeFailure.Inc(1)
+			}
+		} else {
+			if success {
+				metrics.RebalanceReplicaChangeSuccess.Inc(1)
+			} else {
+				metrics.RebalanceReplicaChangeFailure.Inc(1)
+			}
+		}
+	}
 	_, ok := a.cs.ranges[change.RangeID]
 	if !ok {
 		// Range no longer exists. This can happen if the StoreLeaseholderMsg
@@ -218,15 +250,15 @@ func (a *allocatorState) RegisterExternalChange(
 	defer a.mu.Unlock()
 	counterMetrics := a.ensureMetricsForLocalStoreLocked(localStoreID)
 	if err := a.cs.preCheckOnApplyReplicaChanges(change); err != nil {
-		counterMetrics.ExternalFailedToRegister.Inc(1)
+		counterMetrics.ExternalRegisterFailure.Inc(1)
 		log.KvDistribution.Infof(context.Background(),
 			"did not register external changes: due to %v", err)
 		return ExternalRangeChange{}, false
 	} else {
-		counterMetrics.ExternaRegisterSuccess.Inc(1)
+		counterMetrics.ExternalRegisterSuccess.Inc(1)
 	}
 	a.cs.addPendingRangeChange(change)
-	return MakeExternalRangeChange(change), true
+	return MakeExternalRangeChange(OriginExternal, localStoreID, change), true
 }
 
 // ComputeChanges implements the Allocator interface.

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

@@ -548,7 +548,8 @@ func (re *rebalanceEnv) rebalanceReplicas(
 				replicaChanges, rangeID))
 		}
 		re.addPendingRangeChange(rangeChange)
-		re.changes = append(re.changes, MakeExternalRangeChange(rangeChange))
+		re.changes = append(re.changes,
+			MakeExternalRangeChange(originMMARebalance, localStoreID, rangeChange))
 		re.rangeMoveCount++
 		log.KvDistribution.VEventf(ctx, 2,
 			"result(success): rebalancing r%v from s%v to s%v [change: %v] with resulting loads source: %v target: %v",
@@ -722,7 +723,8 @@ func (re *rebalanceEnv) rebalanceLeasesFromLocalStoreID(
 			panic(errors.Wrapf(err, "pre-check failed for lease transfer %v", leaseChange))
 		}
 		re.addPendingRangeChange(leaseChange)
-		re.changes = append(re.changes, MakeExternalRangeChange(leaseChange))
+		re.changes = append(re.changes,
+			MakeExternalRangeChange(originMMARebalance, store.StoreID, leaseChange))
 		log.KvDistribution.Infof(ctx,
 			"result(success): shedding r%v lease from s%v to s%v [change:%v] with "+
 				"resulting loads source:%v target:%v (means: %v) (frac_pending: (src:%.2f,target:%.2f) (src:%.2f,target:%.2f))",

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

@@ -7,124 +7,139 @@ package mmaprototype
 
 import "github.com/cockroachdb/cockroach/pkg/util/metric"
 
+type ChangeOrigin uint8
+
+const (
+	OriginExternal ChangeOrigin = iota
+	originMMARebalance
+)
+
 type counterMetrics struct {
-	DroppedDueToStateInconsistency  *metric.Counter
-	ExternalFailedToRegister        *metric.Counter
-	ExternaRegisterSuccess          *metric.Counter
-	ExternalReplicaRebalanceSuccess *metric.Counter
-	ExternalReplicaRebalanceFailure *metric.Counter
-	ExternalLeaseTransferSuccess    *metric.Counter
-	ExternalLeaseTransferFailure    *metric.Counter
-	MMAReplicaRebalanceSuccess      *metric.Counter
-	MMAReplicaRebalanceFailure      *metric.Counter
-	MMALeaseTransferSuccess         *metric.Counter
-	MMALeaseTransferFailure         *metric.Counter
-	MMARegisterLeaseSuccess         *metric.Counter
-	MMARegisterRebalanceSuccess     *metric.Counter
+	DroppedDueToStateInconsistency *metric.Counter
+
+	ExternalRegisterSuccess *metric.Counter
+	ExternalRegisterFailure *metric.Counter
+
+	ExternalReplicaChangeSuccess *metric.Counter
+	ExternalReplicaChangeFailure *metric.Counter
+	ExternalLeaseChangeSuccess   *metric.Counter
+	ExternalLeaseChangeFailure   *metric.Counter
+
+	RebalanceReplicaChangeSuccess *metric.Counter
+	RebalanceReplicaChangeFailure *metric.Counter
+	RebalanceLeaseChangeSuccess   *metric.Counter
+	RebalanceLeaseChangeFailure   *metric.Counter
 }
 
 func makeCounterMetrics() *counterMetrics {
 	return &counterMetrics{
-		DroppedDueToStateInconsistency:  metric.NewCounter(metaDroppedDueToStateInconsistency),
-		ExternalFailedToRegister:        metric.NewCounter(metaExternalFailedToRegister),
-		ExternaRegisterSuccess:          metric.NewCounter(metaExternaRegisterSuccess),
-		MMARegisterLeaseSuccess:         metric.NewCounter(metaMMARegisterLeaseSuccess),
-		MMARegisterRebalanceSuccess:     metric.NewCounter(metaMMARegisterRebalanceSuccess),
-		ExternalReplicaRebalanceSuccess: metric.NewCounter(metaExternalReplicaRebalanceSuccess),
-		ExternalReplicaRebalanceFailure: metric.NewCounter(metaExternalReplicaRebalanceFailure),
-		ExternalLeaseTransferSuccess:    metric.NewCounter(metaExternalLeaseTransferSuccess),
-		ExternalLeaseTransferFailure:    metric.NewCounter(metaExternalLeaseTransferFailure),
-		MMAReplicaRebalanceSuccess:      metric.NewCounter(metaMMAReplicaRebalanceSuccess),
-		MMAReplicaRebalanceFailure:      metric.NewCounter(metaMMAReplicaRebalanceFailure),
-		MMALeaseTransferSuccess:         metric.NewCounter(metaMMALeaseTransferSuccess),
-		MMALeaseTransferFailure:         metric.NewCounter(metaMMALeaseTransferFailure),
+		DroppedDueToStateInconsistency: metric.NewCounter(metaDroppedDueToStateInconsistency),
+		ExternalRegisterSuccess:        metric.NewCounter(metaExternalRegisterSuccess),
+		ExternalRegisterFailure:        metric.NewCounter(metaExternalRegisterFailure),
+		ExternalReplicaChangeSuccess:   metric.NewCounter(metaExternalReplicaChangeSuccess),
+		ExternalReplicaChangeFailure:   metric.NewCounter(metaExternalReplicaChangeFailure),
+		ExternalLeaseChangeSuccess:     metric.NewCounter(metaExternalLeaseChangeSuccess),
+		ExternalLeaseChangeFailure:     metric.NewCounter(metaExternalLeaseChangeFailure),
+		RebalanceReplicaChangeSuccess:  metric.NewCounter(metaRebalanceReplicaChangeSuccess),
+		RebalanceReplicaChangeFailure:  metric.NewCounter(metaRebalanceReplicaChangeFailure),
+		RebalanceLeaseChangeSuccess:    metric.NewCounter(metaRebalanceLeaseChangeSuccess),
+		RebalanceLeaseChangeFailure:    metric.NewCounter(metaRebalanceLeaseChangeFailure),
 	}
 }
 
 var (
 	metaDroppedDueToStateInconsistency = metric.Metadata{
 		Name:        "mma.dropped",
 		Help:        "Number of operations dropped due to MMA state inconsistency",
-		Measurement: "Range Rebalances",
-		Unit:        metric.Unit_COUNT,
-	}
-	metaExternalFailedToRegister = metric.Metadata{
-		Name:        "mma.external.dropped",
-		Help:        "Number of external operations that failed to register with MMA",
-		Measurement: "Range Rebalances",
+		Measurement: "Replica/Lease Change",
 		Unit:        metric.Unit_COUNT,
 	}
-	metaExternaRegisterSuccess = metric.Metadata{
-		Name:        "mma.external.success",
+	metaExternalRegisterSuccess = metric.Metadata{
+		Name:        "mma.external.registration.success",
 		Help:        "Number of external operations successfully registered with MMA",
-		Measurement: "Range Rebalances",
+		Measurement: "Replica/Lease Change",
 		Unit:        metric.Unit_COUNT,
 	}
-	metaMMARegisterLeaseSuccess = metric.Metadata{
-		Name:        "mma.lease.register.success",
-		Help:        "Number of lease transfers successfully registered with MMA",
-		Measurement: "Range Rebalances",
+	metaExternalRegisterFailure = metric.Metadata{
+		Name:        "mma.external.registration.failure",
+		Help:        "Number of external operations that failed to register with MMA",
+		Measurement: "Replica/Lease Change",
 		Unit:        metric.Unit_COUNT,
 	}
-	metaMMARegisterRebalanceSuccess = metric.Metadata{
-		Name:        "mma.rebalance.register.success",
-		Help:        "Number of rebalance operations successfully registered with MMA",
-		Measurement: "Range Rebalances",
+	metaExternalReplicaChangeSuccess = metric.Metadata{
+		Name:        "mma.change.external.replica.success",
+		Help:        "Number of successful external replica change operations",
+		Measurement: "Range Change",
 		Unit:        metric.Unit_COUNT,
+		LabeledName: "mma.change",
+		StaticLabels: metric.MakeLabelPairs(
+			metric.LabelOrigin, "external", metric.LabelType, "replica", metric.LabelResult, "success"),
 	}
-	metaExternalReplicaRebalanceSuccess = metric.Metadata{
-		Name:        "mma.rebalances.external.success",
-		Help:        "Number of successful external replica rebalance operations",
-		Measurement: "Range Rebalances",
+	metaExternalReplicaChangeFailure = metric.Metadata{
+		Name:        "mma.change.external.replica.failure",
+		Help:        "Number of failed external replica change operations",
+		Measurement: "Range Change",
 		Unit:        metric.Unit_COUNT,
+		LabeledName: "mma.change",
+		StaticLabels: metric.MakeLabelPairs(
+			metric.LabelOrigin, "external", metric.LabelType, "replica", metric.LabelResult, "failure"),
 	}
-
-	metaExternalLeaseTransferSuccess = metric.Metadata{
-		Name:        "mma.lease.external.success",
-		Help:        "Number of successful external lease transfer operations",
-		Measurement: "Lease Transfers",
+	metaExternalLeaseChangeSuccess = metric.Metadata{
+		Name:        "mma.change.external.lease.success",
+		Help:        "Number of successful external lease change operations",
+		Measurement: "Lease Change",
 		Unit:        metric.Unit_COUNT,
+		LabeledName: "mma.change",
+		StaticLabels: metric.MakeLabelPairs(
+			metric.LabelOrigin, "external", metric.LabelType, "lease", metric.LabelResult, "success"),
 	}
-
-	metaExternalReplicaRebalanceFailure = metric.Metadata{
-		Name:        "mma.rebalances.external.failure",
-		Help:        "Number of failed external replica rebalance operations",
-		Measurement: "Range Rebalances",
+	metaExternalLeaseChangeFailure = metric.Metadata{
+		Name:        "mma.change.external.lease.failure",
+		Help:        "Number of failed external lease change operations",
+		Measurement: "Lease Change",
 		Unit:        metric.Unit_COUNT,
+		LabeledName: "mma.change",
+		StaticLabels: metric.MakeLabelPairs(
+			metric.LabelOrigin, "external", metric.LabelType, "lease", metric.LabelResult, "failure"),
 	}
-
-	metaExternalLeaseTransferFailure = metric.Metadata{
-		Name:        "mma.lease.external.failure",
-		Help:        "Number of failed external lease transfer operations",
-		Measurement: "Lease Transfers",
+	metaRebalanceReplicaChangeSuccess = metric.Metadata{
+		Name:        "mma.change.rebalance.replica.success",
+		Help:        "Number of successful MMA-initiated rebalance operations that change replicas",
+		Measurement: "Range Change",
 		Unit:        metric.Unit_COUNT,
+		LabeledName: "mma.change",
+		StaticLabels: metric.MakeLabelPairs(
+			metric.LabelOrigin, "rebalance", metric.LabelType, "replica", metric.LabelResult, "success"),
 	}
-
-	metaMMAReplicaRebalanceSuccess = metric.Metadata{
-		Name:        "mma.rebalance.success",
-		Help:        "Number of successful MMA-initiated replica rebalance operations",
-		Measurement: "Range Rebalances",
+	metaRebalanceReplicaChangeFailure = metric.Metadata{
+		Name:        "mma.change.rebalance.replica.failure",
+		Help:        "Number of failed MMA-initiated rebalance operations that change replicas",
+		Measurement: "Range Change",
 		Unit:        metric.Unit_COUNT,
+		LabeledName: "mma.change",
+		StaticLabels: metric.MakeLabelPairs(
+			metric.LabelOrigin, "rebalance", metric.LabelType, "replica", metric.LabelResult, "failure"),
 	}
-
-	metaMMAReplicaRebalanceFailure = metric.Metadata{
-		Name:        "mma.rebalance.failure",
-		Help:        "Number of failed MMA-initiated replica rebalance operations",
-		Measurement: "Range Rebalances",
+	metaRebalanceLeaseChangeSuccess = metric.Metadata{
+		Name:        "mma.change.rebalance.lease.success",
+		Help:        "Number of successful MMA-initiated rebalance operations that transfer the lease",
+		Measurement: "Lease Change",
 		Unit:        metric.Unit_COUNT,
+		LabeledName: "mma.change",
+		StaticLabels: metric.MakeLabelPairs(
+			metric.LabelOrigin, "rebalance", metric.LabelType, "lease", metric.LabelResult, "success"),
 	}
-
-	metaMMALeaseTransferSuccess = metric.Metadata{
-		Name:        "mma.lease.success",
-		Help:        "Number of successful MMA-initiated lease transfer operations",
-		Measurement: "Lease Transfers",
+	metaRebalanceLeaseChangeFailure = metric.Metadata{
+		Name:        "mma.change.rebalance.lease.failure",
+		Help:        "Number of failed MMA-initiated rebalance operations that transfer the lease",
+		Measurement: "Lease Change",
 		Unit:        metric.Unit_COUNT,
+		LabeledName: "mma.change",
+		StaticLabels: metric.MakeLabelPairs(
+			metric.LabelOrigin, "rebalance", metric.LabelType, "lease", metric.LabelResult, "failure"),
 	}
 
-	metaMMALeaseTransferFailure = metric.Metadata{
-		Name:        "mma.lease.failure",
-		Help:        "Number of failed MMA-initiated lease transfer operations",
-		Measurement: "Lease Transfers",
-		Unit:        metric.Unit_COUNT,
-	}
+	// Future: we will add additional origins for MMA-initiated operations that
+	// are other than rebalance. Eventually, the external label value will go
+	// away when everything is migrated to MMA and SMA is removed.
 )

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

@@ -20,6 +20,9 @@ import (
 // It is a partial external representation of a set of changes that are
 // internally modeled using a slice of *pendingReplicaChanges.
 type ExternalRangeChange struct {
+	origin       ChangeOrigin
+	localStoreID roachpb.StoreID
+
 	roachpb.RangeID
 	Changes []ExternalReplicaChange
 }
@@ -48,7 +51,9 @@ type ExternalReplicaChange struct {
 	ChangeType ReplicaChangeType
 }
 
-func MakeExternalRangeChange(change PendingRangeChange) ExternalRangeChange {
+func MakeExternalRangeChange(
+	origin ChangeOrigin, localStoreID roachpb.StoreID, change PendingRangeChange,
+) ExternalRangeChange {
 	changes := make([]ExternalReplicaChange, len(change.pendingReplicaChanges))
 	for i, rc := range change.pendingReplicaChanges {
 		changeType := rc.replicaChangeType()
@@ -64,8 +69,10 @@ func MakeExternalRangeChange(change PendingRangeChange) ExternalRangeChange {
 		}
 	}
 	return ExternalRangeChange{
-		RangeID: change.RangeID,
-		Changes: changes,
+		origin:       origin,
+		localStoreID: localStoreID,
+		RangeID:      change.RangeID,
+		Changes:      changes,
 	}
 }
 

pkg/kv/kvserver/mmaintegration/mma_conversion_test.go

@@ -114,7 +114,8 @@ func TestConvertReplicaChangeToMMA(t *testing.T) {
 					mmaChanges.SortForTesting()
 					var b strings.Builder
 					fmt.Fprintf(&b, "PendingRangeChange: %s", mmaChanges.StringForTesting())
-					externalChange := mmaprototype.MakeExternalRangeChange(mmaChanges)
+					externalChange := mmaprototype.MakeExternalRangeChange(
+						mmaprototype.OriginExternal, 1 /* arbitrary local store */, mmaChanges)
 					if externalChange.IsChangeReplicas() {
 						fmt.Fprintf(&b, "As kvpb.ReplicationChanges:\n %v\n", externalChange.ReplicationChanges())
 					} else if externalChange.IsPureTransferLease() {

pkg/sql/logictest/testdata/logic_test/generic

@@ -690,3 +690,20 @@ quality of service: regular
   regions: <hidden>
   actual row count: 0
   execution time: 0µs
+
+# Regression test for #159124.
+statement ok
+CREATE TABLE t159124 (
+  k INT PRIMARY KEY,
+  s STRING,
+  UNIQUE INDEX (s)
+)
+
+statement ok
+SET plan_cache_mode = force_generic_plan
+
+statement ok
+PREPARE p159124 AS SELECT k FROM t159124 WHERE s = current_user()
+
+statement ok
+EXECUTE p159124

pkg/sql/opt/xform/generic_funcs.go

@@ -23,12 +23,25 @@ func (c *CustomFuncs) GenericRulesEnabled() bool {
 	return c.e.evalCtx.SessionData().PlanCacheMode != sessiondatapb.PlanCacheModeForceCustom
 }
 
-// HasPlaceholdersOrStableExprs returns true if the given relational expression's subtree has
+// HasPlaceholders returns true if the given relational expression's subtree has
 // at least one placeholder.
+func (c *CustomFuncs) HasPlaceholders(e memo.RelExpr) bool {
+	return e.Relational().HasPlaceholder
+}
+
+// HasPlaceholdersOrStableExprs returns true if the given relational
+// expression's subtree has at least one placeholder or stable expression.
 func (c *CustomFuncs) HasPlaceholdersOrStableExprs(e memo.RelExpr) bool {
 	return e.Relational().HasPlaceholder || e.Relational().VolatilitySet.HasStable()
 }
 
+// IsConstantsAndPlaceholders returns true if all scalar expressions in the list
+// are constants, placeholders or tuples containing constants or placeholders.
+// If a tuple nested within a tuple is found, false is returned.
+func (c *CustomFuncs) IsConstantsAndPlaceholders(scalars memo.ScalarListExpr) bool {
+	return scalars.IsConstantsAndPlaceholders()
+}
+
 // GenerateParameterizedJoinValuesAndFilters returns a single-row Values
 // expression containing placeholders and stable expressions in the given
 // filters. It also returns a new set of filters where the placeholders and