diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel index 6cbbd8c6bee1..86943f7e949d 100644 --- a/pkg/sql/BUILD.bazel +++ b/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", diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go index 818eb95229de..7f59f2bcfa56 100644 --- a/pkg/sql/conn_executor_exec.go +++ b/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 { diff --git a/pkg/sql/explain_plan.go b/pkg/sql/explain_plan.go index 18d1f585c865..08de3d6dfb71 100644 --- a/pkg/sql/explain_plan.go +++ b/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 { diff --git a/pkg/sql/hints/hint_table.go b/pkg/sql/hints/hint_table.go index 3d6707063797..9dbd2d1bc9c9 100644 --- a/pkg/sql/hints/hint_table.go +++ b/pkg/sql/hints/hint_table.go @@ -23,19 +23,29 @@ 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 @@ -43,6 +53,9 @@ type Hint struct { 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,13 +148,12 @@ 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 @@ -149,14 +161,14 @@ func GetStatementHintsFromDB( 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 } diff --git a/pkg/sql/hints/hint_table_test.go b/pkg/sql/hints/hint_table_test.go index 2ea77672cae0..5c2caea0dea3 100644 --- a/pkg/sql/hints/hint_table_test.go +++ b/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 } diff --git a/pkg/sql/instrumentation.go b/pkg/sql/instrumentation.go index 472bd23e3fbf..77a31891a984 100644 --- a/pkg/sql/instrumentation.go +++ b/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 { diff --git a/pkg/sql/opt/exec/execbuilder/testdata/explain b/pkg/sql/opt/exec/execbuilder/testdata/explain index 48ec95b53c74..a508fb33faae 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/explain +++ b/pkg/sql/opt/exec/execbuilder/testdata/explain @@ -2648,7 +2648,7 @@ query T EXPLAIN SELECT k FROM t_hints WHERE k >= 100 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • filter │ filter: k >= 100 @@ -2663,7 +2663,7 @@ EXPLAIN (VERBOSE) SELECT k FROM t_hints WHERE k >= 100 ---- distribution: local vectorized: true -statement hints count: 1 +statement hints: 1 · • filter │ columns: (k) @@ -2675,3 +2675,7 @@ statement hints count: 1 estimated row count: 1,000 (missing stats) table: t_hints@t_hints_v_idx spans: FULL SCAN +· +applied statement hints: 1 + └── REWRITE INLINE HINTS + donor: SELECT k FROM t_hints@t_hints_v_idx WHERE k >= _ diff --git a/pkg/sql/opt/exec/execbuilder/testdata/explain_analyze b/pkg/sql/opt/exec/execbuilder/testdata/explain_analyze index 1753a07ebd95..1d6572e173a8 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/explain_analyze +++ b/pkg/sql/opt/exec/execbuilder/testdata/explain_analyze @@ -240,7 +240,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: custom -statement hints count: 1 +statement hints: 1 rows decoded from KV: 1 (8 B, 2 KVs, 1 gRPC calls) regions: · @@ -261,3 +261,54 @@ regions: missing stats table: kv@kv_v_idx spans: FULL SCAN + +# EXPLAIN ANALYZE (VERBOSE) should include the hint detail tree. +query T +EXPLAIN ANALYZE (VERBOSE) SELECT k FROM kv WHERE k >= 100 +---- +planning time: 10µs +execution time: 100µs +distribution: +vectorized: +plan type: custom +statement hints: 1 +rows decoded from KV: 1 (8 B, 2 KVs, 1 gRPC calls) +maximum memory usage: +DistSQL network usage: +regions: +isolation level: serializable +priority: normal +quality of service: regular +· +• filter +│ columns: (k) +│ sql nodes: +│ regions: +│ vectorized batch count: 0 +│ execution time: 0µs +│ actual row count: 0 +│ estimated row count: 333 (missing stats) +│ filter: k >= 100 +│ +└── • scan + columns: (k) + sql nodes: + kv nodes: + regions: + vectorized batch count: 0 + KV time: 0µs + KV rows decoded: 1 + KV pairs read: 2 + KV bytes read: 8 B + KV gRPC calls: 1 + estimated max memory allocated: 0 B + MVCC step count (ext/int): 0/0 + MVCC seek count (ext/int): 0/0 + actual row count: 1 + estimated row count: 1,000 (missing stats) + table: kv@kv_v_idx + spans: FULL SCAN +· +applied statement hints: 1 + └── REWRITE INLINE HINTS + donor: SELECT k FROM kv@kv_v_idx WHERE k >= _ diff --git a/pkg/sql/opt/exec/execbuilder/testdata/statement_hint_builtins b/pkg/sql/opt/exec/execbuilder/testdata/statement_hint_builtins index 98db22e4f2e1..4c1c903d6cad 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/statement_hint_builtins +++ b/pkg/sql/opt/exec/execbuilder/testdata/statement_hint_builtins @@ -72,7 +72,7 @@ query T EXPLAIN SELECT a FROM abc WHERE a = 10 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • filter │ filter: a = 10 @@ -89,7 +89,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: custom -statement hints count: 1 +statement hints: 1 regions: · • filter @@ -156,7 +156,7 @@ query T EXPLAIN SELECT a, x FROM abc JOIN xy ON y = b WHERE a = 10 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • hash join │ equality: (b) = (y) @@ -179,7 +179,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: custom -statement hints count: 1 +statement hints: 1 rows decoded from KV: 1 (8 B, 2 KVs, 1 gRPC calls) regions: · @@ -245,7 +245,7 @@ query T EXPLAIN SELECT a FROM abc@abc_pkey WHERE b = 10 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • scan missing stats @@ -259,7 +259,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: custom -statement hints count: 1 +statement hints: 1 regions: · • scan @@ -307,7 +307,7 @@ query T EXPLAIN SELECT a + 1 FROM abc WHERE a = 10 ---- distribution: local -statement hints count: 1 +statement hints: 1 (0 applied, 1 skipped) · • render │ @@ -323,7 +323,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: custom -statement hints count: 1 +statement hints: 1 (0 applied, 1 skipped) regions: · • render @@ -375,7 +375,7 @@ query T EXPLAIN SELECT c FROM xy JOIN abc ON c = y WHERE x = 10 ---- distribution: local -statement hints count: 1 +statement hints: 1 (0 applied, 1 skipped) · • hash join │ equality: (c) = (y) @@ -398,7 +398,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: generic, re-optimized -statement hints count: 1 +statement hints: 1 (0 applied, 1 skipped) rows decoded from KV: 1 (8 B, 2 KVs, 1 gRPC calls) regions: · @@ -472,7 +472,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: custom -statement hints count: 1 +statement hints: 1 regions: · • filter @@ -542,7 +542,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: custom -statement hints count: 1 +statement hints: 1 regions: · • render @@ -662,7 +662,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: custom -statement hints count: 1 +statement hints: 1 (0 applied, 1 skipped) regions: · • group (scalar) @@ -732,7 +732,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: custom -statement hints count: 1 +statement hints: 1 (0 applied, 1 skipped) regions: · • group (scalar) @@ -815,7 +815,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: generic, reused -statement hints count: 1 +statement hints: 1 regions: · • render @@ -908,7 +908,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: generic, reused -statement hints count: 1 +statement hints: 1 regions: · • render @@ -970,7 +970,7 @@ query T EXPLAIN SELECT y FROM abc JOIN xy ON x = b WHERE a = 10 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • hash join │ equality: (b) = (x) @@ -994,7 +994,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: custom -statement hints count: 1 +statement hints: 1 rows decoded from KV: 1 (8 B, 2 KVs, 1 gRPC calls) regions: · @@ -1042,7 +1042,7 @@ query T EXPLAIN SELECT y FROM abc JOIN xy ON x = b WHERE a = 10 ---- distribution: local -statement hints count: 1 +statement hints: 2 (1 applied, 1 skipped) · • lookup join │ table: xy@xy_pkey @@ -1064,7 +1064,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: generic, re-optimized -statement hints count: 1 +statement hints: 2 (1 applied, 1 skipped) regions: · • lookup join (streamer) @@ -1131,7 +1131,7 @@ query T EXPLAIN SELECT y FROM abc JOIN xy ON x = b WHERE a = 10 ---- distribution: local -statement hints count: 1 +statement hints: 3 (0 applied, 3 skipped) · • lookup join │ table: xy@xy_pkey @@ -1150,7 +1150,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: generic, re-optimized -statement hints count: 1 +statement hints: 3 (0 applied, 3 skipped) regions: · • lookup join (streamer) @@ -1203,7 +1203,7 @@ query T EXPLAIN SELECT y FROM abc JOIN xy ON x = b WHERE a = 10 ---- distribution: local -statement hints count: 1 +statement hints: 5 (1 applied, 4 skipped) · • merge join │ equality: (b) = (x) @@ -1227,7 +1227,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: generic, re-optimized -statement hints count: 1 +statement hints: 5 (1 applied, 4 skipped) rows decoded from KV: 1 (8 B, 2 KVs, 1 gRPC calls) regions: · @@ -1316,7 +1316,7 @@ JOIN (SELECT i FROM generate_series(1, 1) g(i)) ON b = i ---- distribution: local -statement hints count: 1 +statement hints: 1 · • merge join │ equality: (b) = (generate_series) @@ -1348,7 +1348,7 @@ planning time: 10µs execution time: 100µs distribution: plan type: custom -statement hints count: 1 +statement hints: 1 regions: · • merge join @@ -1458,7 +1458,7 @@ query T EXPLAIN UPDATE abc SET c = c + 1 WHERE a > 5 AND b = 6 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • update │ table: abc @@ -1503,7 +1503,7 @@ query T EXPLAIN DELETE FROM abc WHERE a > 5 AND b = 6 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • delete │ from: abc @@ -1546,7 +1546,7 @@ query T EXPLAIN SELECT x FROM xy WHERE x = 5 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • filter │ filter: x = 5 @@ -1616,7 +1616,7 @@ query T EXPLAIN SELECT x FROM xy WHERE x = 5 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • filter │ filter: x = 5 @@ -1678,7 +1678,7 @@ query T EXPLAIN SELECT a, b FROM abc WHERE a = 5 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • filter │ filter: a = 5 @@ -1700,6 +1700,7 @@ query T EXPLAIN SELECT a, b FROM abc WHERE a = 5 ---- distribution: local +statement hints: 1 (0 applied, 1 skipped) · • scan missing stats @@ -1718,7 +1719,7 @@ query T EXPLAIN SELECT a, b FROM abc WHERE a = 5 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • filter │ filter: a = 5 @@ -1769,7 +1770,7 @@ query T EXPLAIN SELECT * FROM abc WHERE b = 5 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • filter │ filter: b = 5 @@ -1812,12 +1813,12 @@ SELECT crdb_internal.await_statement_hints_cache() # The newer hint (empty value = no disabled rules) wins over the older one, # so the plan should go back to using the constrained index scan. -# The hint count is still 1 because the older hint is de-duplicated. +# The total hint count is 2 (both are loaded), but only 1 is applied. query T EXPLAIN SELECT * FROM abc WHERE b = 5 ---- distribution: local -statement hints count: 1 +statement hints: 2 (1 applied, 1 skipped) · • index join │ table: abc@abc_pkey @@ -1839,7 +1840,7 @@ query T EXPLAIN SELECT * FROM abc WHERE b = 5 ---- distribution: local -statement hints count: 1 +statement hints: 1 · • filter │ filter: b = 5 @@ -1859,7 +1860,7 @@ statement ok SELECT crdb_internal.await_statement_hints_cache() # After deletion, the plan should be the same as the unhinted plan (no -# "statement hints count" annotation). +# "statement hints" annotation). query T EXPLAIN SELECT * FROM abc WHERE b = 5 ---- @@ -1911,3 +1912,407 @@ statement ok SELECT information_schema.crdb_delete_statement_hints( 'SELECT * FROM abc WHERE b = _' ) + +# ============================================================================== +# EXPLAIN (VERBOSE) hint details +# ============================================================================== + +# Start fresh: clear all hints for the tables we use. +statement ok +SELECT information_schema.crdb_delete_statement_hints('SELECT a FROM abc WHERE a = _') + +statement ok +SELECT information_schema.crdb_delete_statement_hints('SELECT * FROM abc WHERE b = _') + +statement ok +SELECT crdb_internal.await_statement_hints_cache() + +# Test 1: EXPLAIN (VERBOSE) with a single applied rewrite inline hint. +statement ok +SELECT information_schema.crdb_rewrite_inline_hints( + 'SELECT a FROM abc WHERE a = _', + 'SELECT a FROM abc@abc_b_idx WHERE a = _' +) + +statement ok +SELECT crdb_internal.await_statement_hints_cache() + +# Verbose EXPLAIN should show the applied hint tree with hint type and details. +# Hint IDs are hidden in test mode (DeterministicExplain). +query T +EXPLAIN (VERBOSE) SELECT a FROM abc WHERE a = 10 +---- +distribution: local +vectorized: true +statement hints: 1 +· +• filter +│ columns: (a) +│ estimated row count: 1 (missing stats) +│ filter: a = 10 +│ +└── • scan + columns: (a) + estimated row count: 1,000 (missing stats) + table: abc@abc_b_idx + spans: FULL SCAN +· +applied statement hints: 1 + └── REWRITE INLINE HINTS + donor: SELECT a FROM abc@abc_b_idx WHERE a = _ + +# Non-verbose EXPLAIN should not show hint details. +query T +EXPLAIN SELECT a FROM abc WHERE a = 10 +---- +distribution: local +statement hints: 1 +· +• filter +│ filter: a = 10 +│ +└── • scan + missing stats + table: abc@abc_b_idx + spans: FULL SCAN + +# Test 2: EXPLAIN (VERBOSE) with a single applied session variable hint. +statement ok +SELECT information_schema.crdb_delete_statement_hints('SELECT a FROM abc WHERE a = _') + +statement ok +SELECT information_schema.crdb_set_session_variable_hint( + 'SELECT * FROM abc WHERE b = _', + 'disable_optimizer_rules', + 'GenerateConstrainedScans' +) + +statement ok +SELECT crdb_internal.await_statement_hints_cache() + +# Verbose EXPLAIN should show the session variable hint with variable and value. +query T +EXPLAIN (VERBOSE) SELECT * FROM abc WHERE b = 5 +---- +distribution: local +vectorized: true +statement hints: 1 +· +• filter +│ columns: (a, b, c) +│ estimated row count: 10 (missing stats) +│ filter: b = 5 +│ +└── • scan + columns: (a, b, c) + estimated row count: 1,000 (missing stats) + table: abc@abc_pkey + spans: FULL SCAN +· +applied statement hints: 1 + └── SET VARIABLE + variable: disable_optimizer_rules + value: GenerateConstrainedScans + +# Test 3: EXPLAIN (VERBOSE) with both applied and skipped (de-duplicated) hints. +# Add a second session variable hint for the same fingerprint + variable. +# The newer hint wins; the older one is skipped (de-duplicated). +statement ok +SELECT information_schema.crdb_set_session_variable_hint( + 'SELECT * FROM abc WHERE b = _', + 'disable_optimizer_rules', + '' +) + +statement ok +SELECT crdb_internal.await_statement_hints_cache() + +# Non-verbose shows the applied/skipped breakdown. +query T +EXPLAIN SELECT * FROM abc WHERE b = 5 +---- +distribution: local +statement hints: 2 (1 applied, 1 skipped) +· +• index join +│ table: abc@abc_pkey +│ +└── • scan + missing stats + table: abc@abc_b_idx + spans: [/5 - /5] + +# Verbose EXPLAIN shows separate trees for applied and skipped hints. +query T +EXPLAIN (VERBOSE) SELECT * FROM abc WHERE b = 5 +---- +distribution: local +vectorized: true +statement hints: 2 (1 applied, 1 skipped) +· +• index join +│ columns: (a, b, c) +│ estimated row count: 10 (missing stats) +│ table: abc@abc_pkey +│ key columns: a +│ parallel +│ +└── • scan + columns: (a, b) + estimated row count: 10 (missing stats) + table: abc@abc_b_idx + spans: /5-/6 +· +applied statement hints: 1 + └── SET VARIABLE + variable: disable_optimizer_rules + value: +· +skipped statement hints: 1 + └── SET VARIABLE + variable: disable_optimizer_rules + value: GenerateConstrainedScans + skip reason: superseded by a newer hint + +# Test 4: EXPLAIN (VERBOSE) with a single hint that is skipped because it +# references a nonexistent index. The hint loads fine but causes a planning +# error, so the optimizer falls back to the unhinted plan. +statement ok +SELECT information_schema.crdb_delete_statement_hints('SELECT * FROM abc WHERE b = _') + +statement ok +SELECT information_schema.crdb_delete_statement_hints('SELECT a FROM abc WHERE a = _') + +statement ok +SELECT information_schema.crdb_rewrite_inline_hints( + 'SELECT a FROM abc WHERE a = _', + 'SELECT a FROM abc@nonexistent_idx WHERE a = _' +) + +statement ok +SELECT crdb_internal.await_statement_hints_cache() + +# The nonexistent index hint fails during planning and falls back to the +# unhinted plan. It shows as skipped with the planning error. +query T +EXPLAIN (VERBOSE) SELECT a FROM abc WHERE a = 10 +---- +distribution: local +vectorized: true +statement hints: 1 (0 applied, 1 skipped) +· +• scan + columns: (a) + estimated row count: 1 (missing stats) + table: abc@abc_pkey + spans: /10/0 +· +skipped statement hints: 1 + └── REWRITE INLINE HINTS + donor: SELECT a FROM abc@nonexistent_idx WHERE a = _ + skip reason: index "nonexistent_idx" not found + +# Clean up test 4. +statement ok +SELECT information_schema.crdb_delete_statement_hints('SELECT a FROM abc WHERE a = _') + +statement ok +SELECT crdb_internal.await_statement_hints_cache() + +# ============================================================================== +# EXPLAIN ANALYZE (VERBOSE) hint details +# ============================================================================== + +# Test 5: EXPLAIN ANALYZE (VERBOSE) with a single applied rewrite inline hint. +statement ok +SELECT information_schema.crdb_rewrite_inline_hints( + 'SELECT a FROM abc WHERE a = _', + 'SELECT a FROM abc@abc_b_idx WHERE a = _' +) + +statement ok +SELECT crdb_internal.await_statement_hints_cache() + +query T +EXPLAIN ANALYZE (VERBOSE) SELECT a FROM abc WHERE a = 10 +---- +planning time: 10µs +execution time: 100µs +distribution: +vectorized: +plan type: custom +statement hints: 1 +maximum memory usage: +DistSQL network usage: +regions: +isolation level: serializable +priority: normal +quality of service: regular +· +• filter +│ columns: (a) +│ sql nodes: +│ regions: +│ vectorized batch count: 0 +│ execution time: 0µs +│ actual row count: 0 +│ estimated row count: 1 (missing stats) +│ filter: a = 10 +│ +└── • scan + columns: (a) + sql nodes: + kv nodes: + regions: + vectorized batch count: 0 + KV time: 0µs + KV rows decoded: 0 + KV pairs read: 0 + KV bytes read: 0 B + KV gRPC calls: 0 + estimated max memory allocated: 0 B + MVCC step count (ext/int): 0/0 + MVCC seek count (ext/int): 0/0 + actual row count: 0 + estimated row count: 1,000 (missing stats) + table: abc@abc_b_idx + spans: FULL SCAN +· +applied statement hints: 1 + └── REWRITE INLINE HINTS + donor: SELECT a FROM abc@abc_b_idx WHERE a = _ + +# Test 6: EXPLAIN ANALYZE (VERBOSE) with a skipped hint (nonexistent index). +statement ok +SELECT information_schema.crdb_delete_statement_hints('SELECT a FROM abc WHERE a = _') + +statement ok +SELECT information_schema.crdb_rewrite_inline_hints( + 'SELECT a FROM abc WHERE a = _', + 'SELECT a FROM abc@nonexistent_idx WHERE a = _' +) + +statement ok +SELECT crdb_internal.await_statement_hints_cache() + +query T +EXPLAIN ANALYZE (VERBOSE) SELECT a FROM abc WHERE a = 10 +---- +planning time: 10µs +execution time: 100µs +distribution: +vectorized: +plan type: generic, re-optimized +statement hints: 1 (0 applied, 1 skipped) +maximum memory usage: +DistSQL network usage: +regions: +isolation level: serializable +priority: normal +quality of service: regular +· +• scan + columns: (a) + sql nodes: + kv nodes: + regions: + vectorized batch count: 0 + KV time: 0µs + KV rows decoded: 0 + KV pairs read: 0 + KV bytes read: 0 B + KV gRPC calls: 0 + estimated max memory allocated: 0 B + MVCC step count (ext/int): 0/0 + MVCC seek count (ext/int): 0/0 + actual row count: 0 + estimated row count: 1 (missing stats) + table: abc@abc_pkey + spans: /10/0 +· +skipped statement hints: 1 + └── REWRITE INLINE HINTS + donor: SELECT a FROM abc@nonexistent_idx WHERE a = _ + skip reason: index "nonexistent_idx" not found + +# Clean up. +statement ok +SELECT information_schema.crdb_delete_statement_hints('SELECT a FROM abc WHERE a = _') + +statement ok +SELECT crdb_internal.await_statement_hints_cache() + +# ============================================================================== +# PREPARE/EXECUTE: hint retry after schema change +# ============================================================================== + +# Verify that a rewrite inline hint forcing a nonexistent index is retried +# after the index is created, even when using a prepared statement. The hint +# should initially fail and succeed after the index exists. + +statement ok +SELECT information_schema.crdb_rewrite_inline_hints( + 'SELECT c FROM abc WHERE a = _', + 'SELECT c FROM abc@abc_c_idx WHERE a = _' +) + +statement ok +SELECT crdb_internal.await_statement_hints_cache() + +statement ok +PREPARE hint_retry_stmt AS SELECT c FROM abc WHERE a = 10 + +# First EXECUTE: abc_c_idx doesn't exist, so the hint should fail and fall +# back to the unhinted plan. +statement ok +SET tracing = on + +statement ok +EXECUTE hint_retry_stmt + +statement ok +SET tracing = off + +query T +SELECT regexp_replace(split_part(message, ': SELECT', 1), E'\\d+', 'x') +FROM [SHOW TRACE FOR SESSION] +WHERE message LIKE '%injected hints%' OR message LIKE '%planning with%' +---- +trying planning with injected hints +planning with injected hints failed with: index "abc_c_idx" not found +falling back to planning without injected hints + +# Create the index that the hint forces. +statement ok +CREATE INDEX abc_c_idx ON abc(c) + +# Second EXECUTE: abc_c_idx now exists, so the runtime error from the first +# execution should be cleared and the hint should succeed. +statement ok +SET tracing = on + +statement ok +EXECUTE hint_retry_stmt + +statement ok +SET tracing = off + +query T +SELECT regexp_replace(split_part(message, ': SELECT', 1), E'\\d+', 'x') +FROM [SHOW TRACE FOR SESSION] +WHERE message LIKE '%injected hints%' OR message LIKE '%planning with%' +---- +trying planning with injected hints + +# Clean up. +statement ok +DEALLOCATE hint_retry_stmt + +statement ok +DROP INDEX abc@abc_c_idx + +statement ok +SELECT information_schema.crdb_delete_statement_hints('SELECT c FROM abc WHERE a = _') + +statement ok +SELECT crdb_internal.await_statement_hints_cache() diff --git a/pkg/sql/opt/exec/explain/BUILD.bazel b/pkg/sql/opt/exec/explain/BUILD.bazel index f18e628f088c..1885c98c87b7 100644 --- a/pkg/sql/opt/exec/explain/BUILD.bazel +++ b/pkg/sql/opt/exec/explain/BUILD.bazel @@ -22,6 +22,7 @@ go_library( "//pkg/sql/appstatspb", "//pkg/sql/catalog/colinfo", "//pkg/sql/catalog/descpb", + "//pkg/sql/hints", "//pkg/sql/inverted", # keep "//pkg/sql/opt", # keep "//pkg/sql/opt/cat", diff --git a/pkg/sql/opt/exec/explain/output.go b/pkg/sql/opt/exec/explain/output.go index 755e462fc34a..a45f5355e45e 100644 --- a/pkg/sql/opt/exec/explain/output.go +++ b/pkg/sql/opt/exec/explain/output.go @@ -15,6 +15,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/sql/appstatspb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" + "github.com/cockroachdb/cockroach/pkg/sql/hints" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" @@ -328,13 +329,31 @@ func (ob *OutputBuilder) AddPlanType(generic, optimized bool) { } } -// AddStmtHintCount adds a top-level field displaying the number of statement -// hints applied to the query. Cannot be called while inside a node. -func (ob *OutputBuilder) AddStmtHintCount(hintCount uint64) { - if hintCount == 0 { +// AddStmtHintCount adds a top-level field displaying a summary of the +// statement hints loaded for the query (applied and skipped). Cannot be called +// while inside a node. +func (ob *OutputBuilder) AddStmtHintCount(hints []hints.Hint, runtimeErrors map[int]error) { + if len(hints) == 0 { return } - ob.AddTopLevelField("statement hints count", string(humanizeutil.Count(hintCount))) + total := uint64(len(hints)) + applied := uint64(0) + for i := range hints { + if hints[i].Enabled() && runtimeErrors[i] == nil { + applied++ + } + } + if total == applied { + ob.AddTopLevelField("statement hints", string(humanizeutil.Count(total))) + } else { + skipped := total - applied + ob.AddTopLevelField("statement hints", fmt.Sprintf( + "%s (%s applied, %s skipped)", + humanizeutil.Count(total), + humanizeutil.Count(applied), + humanizeutil.Count(skipped), + )) + } } // AddPlanningTime adds a top-level planning time field. Cannot be called diff --git a/pkg/sql/plan_opt.go b/pkg/sql/plan_opt.go index 68aabb37bd52..998e23679494 100644 --- a/pkg/sql/plan_opt.go +++ b/pkg/sql/plan_opt.go @@ -323,6 +323,16 @@ func (p *planner) makeOptimizerPlan(ctx context.Context) error { // planning again without injected hints. log.Eventf(ctx, "planning with injected hints failed with: %v", err) opc.log(ctx, "falling back to planning without injected hints") + // Record the injection error so that EXPLAIN output correctly + // reflects this hint as skipped. The error is stored on the + // instrumentation helper rather than on the hint itself to avoid + // mutating shared hint state across prepared statement executions. + for i := range p.stmt.Hints { + if p.stmt.Hints[i].HintInjectionDonor != nil && p.stmt.Hints[i].Enabled() { + p.instrumentation.recordHintError(i, err) + break + } + } } opc.reset(ctx) return p.makeOptimizerPlanInternal(ctx) diff --git a/pkg/sql/statement.go b/pkg/sql/statement.go index ec50f07b13a4..19cdee37153a 100644 --- a/pkg/sql/statement.go +++ b/pkg/sql/statement.go @@ -188,8 +188,9 @@ func (s *Statement) ReloadHintsIfStale( len(s.Hints), s.HintIDs, ) - for i, hint := range s.Hints { - if !hint.Enabled || hint.Err != nil { + for i := range s.Hints { + hint := &s.Hints[i] + if !hint.Enabled() { continue } if hd := hint.HintInjectionDonor; hd != nil && s.ASTWithInjectedHints == nil { @@ -200,6 +201,7 @@ func (s *Statement) ReloadHintsIfStale( ) // Do not return the error. Instead we'll simply execute the query // without this hint. + hint.Err = err continue } injectedAST, injected, err := hd.InjectHints(ast) @@ -209,6 +211,7 @@ func (s *Statement) ReloadHintsIfStale( ) // Do not return the error. Instead we'll simply execute the query // without this hint. + hint.Err = err continue } if injected {