sql,hints: improve EXPLAIN output for statement hints

pkg/sql/BUILD.bazel

@@ -629,6 +629,7 @@ go_library(
         "//pkg/util/tracing",
         "//pkg/util/tracing/collector",
         "//pkg/util/tracing/tracingpb",
+        "//pkg/util/treeprinter",
         "//pkg/util/tsearch",
         "//pkg/util/uint128",
         "//pkg/util/unique",

pkg/sql/conn_executor_exec.go

@@ -4798,7 +4798,7 @@ func (ex *connExecutor) applySessionVariableHints(
 	var pushedSessionData bool
 	for i := range stmt.Hints {
 		hint := &stmt.Hints[i]
-		if !hint.Enabled || hint.Err != nil || hint.SessionVariable == nil {
+		if !hint.Enabled() || hint.SessionVariable == nil {
 			continue
 		}
 		varHint := hint.SessionVariable
@@ -4807,6 +4807,7 @@ func (ex *connExecutor) applySessionVariableHints(
 			pushedSessionData = true
 		}
 		if err := ex.applySessionVariableHint(ctx, p, varHint); err != nil {
+			p.instrumentation.recordHintError(i, err)
 			log.Eventf(ctx, "skipping session variable hint for %s: %v",
 				redact.Safe(varHint.VariableName), err)
 		} else {

pkg/sql/explain_plan.go

@@ -9,12 +9,15 @@ import (
 	"context"
 	"fmt"
 	"net/url"
+	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/colflow"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
+	"github.com/cockroachdb/cockroach/pkg/sql/hintpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/hints"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/cat"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec/explain"
@@ -24,6 +27,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
 	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil"
+	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
+	"github.com/cockroachdb/cockroach/pkg/util/treeprinter"
 	"github.com/cockroachdb/errors"
 )
 
@@ -121,16 +126,7 @@ func (e *explainPlanNode) startExec(params runParams) error {
 		}
 
 		ob.AddTableStatsMode(params.EvalContext().StatsRollout.String())
-
-		if len(params.p.stmt.Hints) > 0 {
-			var hintCount uint64
-			for _, hint := range params.p.stmt.Hints {
-				if hint.Enabled && hint.Err == nil {
-					hintCount += 1
-				}
-			}
-			ob.AddStmtHintCount(hintCount)
-		}
+		ob.AddStmtHintCount(params.p.stmt.Hints, params.p.instrumentation.runtimeHintErrors)
 
 		if e.options.Flags[tree.ExplainFlagJSON] {
 			// For the JSON flag, we only want to emit the diagram JSON.
@@ -150,6 +146,13 @@ func (e *explainPlanNode) startExec(params runParams) error {
 				return err
 			}
 			rows = ob.BuildStringRows()
+			if e.options.Flags[tree.ExplainFlagVerbose] && len(params.p.stmt.Hints) > 0 {
+				hideIDs := params.p.execCfg.TestingKnobs.DeterministicExplain
+				rows = append(rows, buildStmtHintTreeRows(
+					params.p.stmt.Hints, params.p.stmt.HintIDs, hideIDs,
+					params.p.instrumentation.runtimeHintErrors,
+				)...)
+			}
 			if e.options.Mode == tree.ExplainDistSQL {
 				rows = append(rows, "", fmt.Sprintf("Diagram: %s", diagramURL.String()))
 			}
@@ -289,6 +292,108 @@ func closeExplainPlan(ctx context.Context, ep *explain.Plan) {
 	}
 }
 
+// buildStmtHintTreeRows builds the verbose EXPLAIN output for statement hints,
+// returning rows for applied and skipped hint trees. If hideIDs is true, hint
+// IDs are omitted for deterministic test output. runtimeErrors contains
+// per-hint runtime errors keyed by hint index, tracked separately from
+// hint.Err to avoid mutating shared hint state.
+func buildStmtHintTreeRows(
+	allHints []hints.Hint, hintIDs []int64, hideIDs bool, runtimeErrors map[int]error,
+) []string {
+	var applied, skipped []int
+	for i := range allHints {
+		if !allHints[i].Enabled() || runtimeErrors[i] != nil {
+			skipped = append(skipped, i)
+		} else {
+			applied = append(applied, i)
+		}
+	}
+
+	var rows []string
+	if len(applied) > 0 {
+		rows = append(rows, "") // blank separator
+		rows = append(rows, buildHintTree(
+			fmt.Sprintf("applied statement hints: %s",
+				humanizeutil.Count(uint64(len(applied)))),
+			allHints, hintIDs, applied, hideIDs, runtimeErrors,
+		)...)
+	}
+	if len(skipped) > 0 {
+		rows = append(rows, "") // blank separator
+		rows = append(rows, buildHintTree(
+			fmt.Sprintf("skipped statement hints: %s",
+				humanizeutil.Count(uint64(len(skipped)))),
+			allHints, hintIDs, skipped, hideIDs, runtimeErrors,
+		)...)
+	}
+	return rows
+}
+
+// getHintErr returns the effective error for a hint, checking both the
+// hint's own Err field and the per-hint runtime errors tracked on the
+// instrumentation helper.
+func getHintErr(allHints []hints.Hint, runtimeErrors map[int]error, idx int) error {
+	if idx < len(allHints) && allHints[idx].Err != nil {
+		return allHints[idx].Err
+	}
+	return runtimeErrors[idx]
+}
+
+// buildHintTree builds a treeprinter tree for a group of hints and returns the
+// formatted rows.
+func buildHintTree(
+	rootLabel string,
+	allHints []hints.Hint,
+	hintIDs []int64,
+	indices []int,
+	hideIDs bool,
+	runtimeErrors map[int]error,
+) []string {
+	tp := treeprinter.New()
+	root := tp.Child(rootLabel)
+	for _, idx := range indices {
+		hint := &allHints[idx]
+		var label string
+		if hideIDs {
+			label = hint.HintType()
+		} else if idx < len(hintIDs) {
+			label = fmt.Sprintf("%s (id: %d)", hint.HintType(), hintIDs[idx])
+		} else {
+			if buildutil.CrdbTestBuild {
+				panic(errors.AssertionFailedf(
+					"hint index %d out of range for hintIDs (len %d)", idx, len(hintIDs),
+				))
+			}
+			label = hint.HintType()
+		}
+		node := root.Child(label)
+
+		switch t := hint.GetValue().(type) {
+		case *hintpb.InjectHints:
+			node.AddLine(fmt.Sprintf("donor: %s", t.DonorSQL))
+		case *hintpb.SessionVariableHint:
+			node.AddLine(fmt.Sprintf("variable: %s", t.VariableName))
+			node.AddLine(fmt.Sprintf("value: %s", t.VariableValue))
+		}
+
+		var skipErr error
+		if idx < len(allHints) && allHints[idx].Err != nil {
+			skipErr = allHints[idx].Err
+		} else {
+			skipErr = runtimeErrors[idx]
+		}
+		if skipErr != nil {
+			errMsg := skipErr.Error()
+			// Ensure the error message is single-line for clean tree output.
+			if i := strings.IndexByte(errMsg, '\n'); i >= 0 {
+				errMsg = errMsg[:i]
+			}
+			node.AddLine(fmt.Sprintf("skip reason: %s", errMsg))
+		}
+	}
+	return tp.FormattedRows()
+}
+
 func (e *explainPlanNode) Close(ctx context.Context) {
 	closeExplainPlan(ctx, e.plan)
 	if e.run.results != nil {

pkg/sql/hints/hint_table.go

@@ -23,26 +23,39 @@ import (
 	"github.com/cockroachdb/errors"
 )
 
+var (
+	// ErrDuplicateHint is set on a hint's Err field when it is superseded by a
+	// newer hint of the same type for the same fingerprint.
+	ErrDuplicateHint = errors.New("superseded by a newer hint")
+
+	// ErrDisabledHint is set on a hint's Err field when it has been disabled by
+	// the user.
+	ErrDisabledHint = errors.New("hint is disabled")
+)
+
 // Hint represents an unmarshaled hint that is ready to apply to statements.
 type Hint struct {
 	hintpb.StatementHintUnion
 
-	// The hint should only be applied to statements if Enabled is true.
-	Enabled bool
-
 	// Database is the database to which this hint is scoped. If empty, the hint
 	// applies regardless of the current database.
 	Database string
 
-	// If Err is not nil it was an error encountered while loading the hint from
-	// system.statement_hints, and Enabled will be false.
+	// Err is non-nil when the hint cannot be applied. This includes errors
+	// encountered during loading or parsing, duplicate suppression
+	// (ErrDuplicateHint) disabled hints (ErrDisabledHint), session variable
+	// application failures, and optimizer planning fallback errors.
+	// A nil Err means the hint is eligible for use.
 	Err error
 
 	// HintInjectionDonor is the fully parsed donor statement fingerprint used for
 	// hint injection.
 	HintInjectionDonor *tree.HintInjectionDonor
 }
 
+// Enabled reports whether the hint is eligible to be applied (no error).
+func (h *Hint) Enabled() bool { return h.Err == nil }
+
 // CheckForStatementHintsInDB queries the system.statement_hints table to
 // determine if there are any hints for the given fingerprint hash. The caller
 // must be able to retry if an error is returned.
@@ -135,28 +148,27 @@ func GetStatementHintsFromDB(
 		}
 		hintID, fingerprint, hint := parseHint(it.Cur(), fingerprintFlags)
 		if hint.Err != nil {
+			// Do not return the error. Instead, we'll simply execute the query
+			// without this hint.
 			log.Dev.Warningf(
 				ctx, "could not decode hint ID %v for statement hash %v fingerprint %v: %v",
 				hintID, statementHash, fingerprint, hint.Err,
 			)
-			// Do not return the error. Instead, we'll simply execute the query without
-			// this hint (which should already be disabled).
-			hint.Enabled = false
 		}
 
 		// Resolve duplicate hints by picking the newer one (which will be ordered
 		// first by the query).
 		switch t := hint.GetValue().(type) {
 		case *hintpb.InjectHints:
 			if _, ok := seenInjections[fingerprint]; ok {
-				hint.Enabled = false
+				hint.Err = ErrDuplicateHint
 			} else {
 				seenInjections[fingerprint] = struct{}{}
 			}
 		case *hintpb.SessionVariableHint:
 			key := [2]string{fingerprint, t.VariableName}
 			if _, ok := seenVarHints[key]; ok {
-				hint.Enabled = false
+				hint.Err = ErrDuplicateHint
 			} else {
 				seenVarHints[key] = struct{}{}
 			}
@@ -190,10 +202,12 @@ func parseHint(
 			return hintID, fingerprint, hint
 		}
 	}
-	hint.Enabled = bool(tree.MustBeDBool(datums[3]))
 	if datums[4] != tree.DNull {
 		hint.Database = string(tree.MustBeDString(datums[4]))
 	}
+	if enabled := bool(tree.MustBeDBool(datums[3])); !enabled {
+		hint.Err = ErrDisabledHint
+	}
 	return hintID, fingerprint, hint
 }
 

pkg/sql/hints/hint_table_test.go

@@ -48,8 +48,6 @@ func TestHintTableOperations(t *testing.T) {
 	var hint1, hint2 hints.Hint
 	hint1.SetValue(&hintpb.InjectHints{DonorSQL: "SELECT a FROM t@t_b_idx WHERE b = $1"})
 	hint2.SetValue(&hintpb.InjectHints{DonorSQL: "SELECT c FROM t@{NO_FULL_SCAN} WHERE d = $2"})
-	hint1.Enabled = true
-	hint2.Enabled = true
 	var err error
 	donorStmt1, err := parserutils.ParseOne(hint1.InjectHints.DonorSQL)
 	require.NoError(t, err)
@@ -173,7 +171,7 @@ func TestHintTableOperations(t *testing.T) {
 	require.NoError(t, err)
 	found := false
 	for _, h := range hintsFromDB {
-		if !h.Enabled {
+		if !h.Enabled() {
 			found = true
 			break
 		}

pkg/sql/instrumentation.go

@@ -22,6 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfra"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/execstats"
+	"github.com/cockroachdb/cockroach/pkg/sql/hints"
 	"github.com/cockroachdb/cockroach/pkg/sql/idxrecommendations"
 	"github.com/cockroachdb/cockroach/pkg/sql/isql"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec"
@@ -211,9 +212,18 @@ type instrumentationHelper struct {
 	// stats scanned by this query.
 	nanosSinceStatsForecasted time.Duration
 
-	// stmtHintsCount is the number of hints from system.statement_hints applied
-	// to the statement.
-	stmtHintsCount uint64
+	// stmtHints and stmtHintIDs are the statement hints loaded for this query.
+	// Hint counts are computed from these at output time (rather than at Setup
+	// time) so that hints which fail during application are correctly reflected.
+	stmtHints            []hints.Hint
+	stmtHintIDs          []int64
+	deterministicExplain bool
+
+	// runtimeHintErrors stores per-hint runtime errors, keyed by hint index
+	// into stmtHints. It is lazily allocated on first error. Errors are stored
+	// here rather than on hint.Err to avoid mutating shared hint state across
+	// prepared statement executions.
+	runtimeHintErrors map[int]error
 
 	// retryCount is the number of times the transaction was retried.
 	retryCount uint64
@@ -437,12 +447,9 @@ func (ih *instrumentationHelper) Setup(
 	ih.implicitTxn = implicitTxn
 	ih.txnPriority = txnPriority
 	ih.txnBufferedWritesEnabled = p.txn.BufferedWritesEnabled()
-	ih.stmtHintsCount = 0
-	for _, hint := range stmt.Hints {
-		if hint.Enabled && hint.Err == nil {
-			ih.stmtHintsCount += 1
-		}
-	}
+	ih.stmtHints = stmt.Hints
+	ih.stmtHintIDs = stmt.HintIDs
+	ih.deterministicExplain = cfg.TestingKnobs.DeterministicExplain
 	ih.retryCount = uint64(retryCount)
 	ih.codec = cfg.Codec
 	ih.origCtx = ctx
@@ -876,7 +883,7 @@ func (ih *instrumentationHelper) emitExplainAnalyzePlanToOutputBuilder(
 	ob.AddVectorized(ih.vectorized)
 	ob.AddPlanType(ih.generic, ih.optimized)
 	ob.AddTableStatsMode(ih.tableStatsRollout.String())
-	ob.AddStmtHintCount(ih.stmtHintsCount)
+	ob.AddStmtHintCount(ih.stmtHints, ih.runtimeHintErrors)
 	ob.AddRetryCount("transaction", ih.retryCount)
 	ob.AddRetryTime("transaction", phaseTimes.GetTransactionRetryLatency())
 	ob.AddRetryCount("statement", ih.retryStmtCount)
@@ -968,6 +975,15 @@ func (ih *instrumentationHelper) emitExplainAnalyzePlanToOutputBuilder(
 	return ob
 }
 
+// recordHintError records a runtime error for the hint at the given index.
+// The map is lazily allocated on first use.
+func (ih *instrumentationHelper) recordHintError(idx int, err error) {
+	if ih.runtimeHintErrors == nil {
+		ih.runtimeHintErrors = make(map[int]error)
+	}
+	ih.runtimeHintErrors[idx] = err
+}
+
 // setExplainAnalyzeResult sets the result for an EXPLAIN ANALYZE or EXPLAIN
 // ANALYZE (DISTSQL) statement (in the former case, distSQLFlowInfos and trace
 // are nil).
@@ -990,6 +1006,12 @@ func (ih *instrumentationHelper) setExplainAnalyzeResult(
 
 	ob := ih.emitExplainAnalyzePlanToOutputBuilder(ctx, ih.explainFlags, phaseTimes, queryLevelStats)
 	rows := ob.BuildStringRows()
+	if ih.explainFlags.Verbose && len(ih.stmtHints) > 0 {
+		rows = append(rows, buildStmtHintTreeRows(
+			ih.stmtHints, ih.stmtHintIDs, ih.deterministicExplain,
+			ih.runtimeHintErrors,
+		)...)
+	}
 	if distSQLFlowInfos != nil {
 		rows = append(rows, "")
 		for i, d := range distSQLFlowInfos {