From f98fc0006457487928bc7649d3297f8bbea21790 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Mon, 17 Aug 2020 21:42:55 -0400 Subject: [PATCH 1/3] sql: don't key statement statistics on distribution/vectorization The statement statistics code buckets statements separately depending on the distribution and vectorization flags. This is not very useful: we normally don't expect these flags to change for the same statement; and the UI doesn't show them separately anyway. In addition, it poses a problem to switching the explain tree (presented to UI) to use the new explain infrastructure: we need to know if we will need the tree upfront (before execbuild). This change removes these flags from the key. The flags are still populated in the `StatementStatisticsKey`. Release note: None --- pkg/sql/app_stats.go | 44 ++++++++++++++++------------------------ pkg/sql/crdb_internal.go | 9 +++++++- 2 files changed, 25 insertions(+), 28 deletions(-) diff --git a/pkg/sql/app_stats.go b/pkg/sql/app_stats.go index b7f754f86898..5fd90a0ac6a7 100644 --- a/pkg/sql/app_stats.go +++ b/pkg/sql/app_stats.go @@ -37,8 +37,6 @@ import ( type stmtKey struct { stmt string failed bool - distSQLUsed bool - vectorized bool implicitTxn bool } @@ -56,6 +54,14 @@ type stmtStats struct { syncutil.Mutex data roachpb.StatementStatistics + + // distSQLUsed records whether the last instance of this statement used + // distribution. + distSQLUsed bool + + // vectorized records whether the last instance of this statement used + // vectorization. + vectorized bool } // transactionStats holds per-application transaction statistics. @@ -105,18 +111,10 @@ var logicalPlanCollectionPeriod = settings.RegisterPublicNonNegativeDurationSett ) func (s stmtKey) String() string { - return s.flags() + s.stmt -} - -func (s stmtKey) flags() string { - var b bytes.Buffer if s.failed { - b.WriteByte('!') - } - if s.distSQLUsed { - b.WriteByte('+') + return "!" + s.stmt } - return b.String() + return s.stmt } // recordStatement saves per-statement statistics. @@ -144,7 +142,7 @@ func (a *appStats) recordStatement( // Get the statistics object. s := a.getStatsForStmt( - stmt, distSQLUsed, vectorized, implicitTxn, + stmt, implicitTxn, err, true, /* createIfNonexistent */ ) @@ -172,24 +170,19 @@ func (a *appStats) recordStatement( s.data.OverheadLat.Record(s.data.Count, ovhLat) s.data.BytesRead.Record(s.data.Count, float64(stats.bytesRead)) s.data.RowsRead.Record(s.data.Count, float64(stats.rowsRead)) + s.distSQLUsed = distSQLUsed + s.vectorized = vectorized s.Unlock() } // getStatsForStmt retrieves the per-stmt stat object. func (a *appStats) getStatsForStmt( - stmt *Statement, - distSQLUsed bool, - vectorized bool, - implicitTxn bool, - err error, - createIfNonexistent bool, + stmt *Statement, implicitTxn bool, err error, createIfNonexistent bool, ) *stmtStats { // Extend the statement key with various characteristics, so // that we use separate buckets for the different situations. key := stmtKey{ failed: err != nil, - distSQLUsed: distSQLUsed, - vectorized: vectorized, implicitTxn: implicitTxn, } if stmt.AnonymizedStr != "" { @@ -305,10 +298,7 @@ func (a *appStats) shouldSaveLogicalPlanDescription( if !sampleLogicalPlans.Get(&a.st.SV) { return false } - stats := a.getStatsForStmt( - stmt, useDistSQL, vectorized, implicitTxn, - err, false, /* createIfNonexistent */ - ) + stats := a.getStatsForStmt(stmt, implicitTxn, err, false /* createIfNonexistent */) if stats == nil { // Save logical plan the first time we see new statement fingerprint. return true @@ -506,9 +496,9 @@ func (s *sqlStats) getStmtStats( if ok { k := roachpb.StatementStatisticsKey{ Query: maybeScrubbed, - DistSQL: q.distSQLUsed, + DistSQL: stats.distSQLUsed, Opt: true, - Vec: q.vectorized, + Vec: stats.vectorized, ImplicitTxn: q.implicitTxn, Failed: q.failed, App: maybeHashedAppName, diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index abded746784f..7bae76f68aa5 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -778,10 +778,17 @@ CREATE TABLE crdb_internal.node_statement_statistics ( if s.data.SensitiveInfo.LastErr != "" { errString = tree.NewDString(s.data.SensitiveInfo.LastErr) } + var flags string + if s.distSQLUsed { + flags = "+" + } + if stmtKey.failed { + flags = "!" + flags + } err := addRow( tree.NewDInt(tree.DInt(nodeID)), tree.NewDString(appName), - tree.NewDString(stmtKey.flags()), + tree.NewDString(flags), tree.NewDString(stmtKey.stmt), anonymized, tree.NewDInt(tree.DInt(s.data.Count)), From de2674fc1dd5d8a1e030044d6755ea920777dad2 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Tue, 18 Aug 2020 00:00:19 -0400 Subject: [PATCH 2/3] sql: pass exec factory to apply-join/recursive-cte functions To support executing plans that were generated with `explain.Factory`, we must not use the same factory for "inner" plans generated by apply joins and recursive CTEs. We modify the functions to pass the exec factory. Note that we were already doing this for cascade planning. Release note: None --- pkg/sql/apply_join.go | 2 +- pkg/sql/opt/exec/execbuilder/relational.go | 8 ++++---- pkg/sql/opt/exec/factory.go | 4 ++-- pkg/sql/recursive_cte.go | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/sql/apply_join.go b/pkg/sql/apply_join.go index 470c5ac62a30..a6bc1e3e1a53 100644 --- a/pkg/sql/apply_join.go +++ b/pkg/sql/apply_join.go @@ -200,7 +200,7 @@ func (a *applyJoinNode) Next(params runParams) (bool, error) { // the right side of the join using the optimizer, with all outer columns // in the right side replaced by the bindings that were defined by the most // recently read left row. - p, err := a.planRightSideFn(leftRow) + p, err := a.planRightSideFn(newExecFactory(params.p), leftRow) if err != nil { return false, err } diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go index d1ef86242d6b..4ab93d64cb80 100644 --- a/pkg/sql/opt/exec/execbuilder/relational.go +++ b/pkg/sql/opt/exec/execbuilder/relational.go @@ -723,7 +723,7 @@ func (b *Builder) buildApplyJoin(join memo.RelExpr) (execPlan, error) { // // Note: we put o outside of the function so we allocate it only once. var o xform.Optimizer - planRightSideFn := func(leftRow tree.Datums) (exec.Plan, error) { + planRightSideFn := func(ef exec.Factory, leftRow tree.Datums) (exec.Plan, error) { o.Init(b.evalCtx, b.catalog) f := o.Factory() @@ -746,7 +746,7 @@ func (b *Builder) buildApplyJoin(join memo.RelExpr) (execPlan, error) { return nil, err } - eb := New(b.factory, f.Memo(), b.catalog, newRightSide, b.evalCtx, false /* allowAutoCommit */) + eb := New(ef, f.Memo(), b.catalog, newRightSide, b.evalCtx, false /* allowAutoCommit */) eb.disableTelemetry = true plan, err := eb.Build() if err != nil { @@ -1795,7 +1795,6 @@ func (b *Builder) buildRecursiveCTE(rec *memo.RecursiveCTEExpr) (execPlan, error // To implement exec.RecursiveCTEIterationFn, we create a special Builder. innerBldTemplate := &Builder{ - factory: b.factory, mem: b.mem, catalog: b.catalog, evalCtx: b.evalCtx, @@ -1806,9 +1805,10 @@ func (b *Builder) buildRecursiveCTE(rec *memo.RecursiveCTEExpr) (execPlan, error withExprs: b.withExprs[:len(b.withExprs):len(b.withExprs)], } - fn := func(bufferRef exec.Node) (exec.Plan, error) { + fn := func(ef exec.Factory, bufferRef exec.Node) (exec.Plan, error) { // Use a separate builder each time. innerBld := *innerBldTemplate + innerBld.factory = ef innerBld.addBuiltWithExpr(rec.WithID, initial.outputCols, bufferRef) plan, err := innerBld.build(rec.Recursive) if err != nil { diff --git a/pkg/sql/opt/exec/factory.go b/pkg/sql/opt/exec/factory.go index 65fb883d794c..0461e788212a 100644 --- a/pkg/sql/opt/exec/factory.go +++ b/pkg/sql/opt/exec/factory.go @@ -196,12 +196,12 @@ type KVOption struct { // RecursiveCTEIterationFn creates a plan for an iteration of WITH RECURSIVE, // given the result of the last iteration (as a node created by // ConstructBuffer). -type RecursiveCTEIterationFn func(bufferRef Node) (Plan, error) +type RecursiveCTEIterationFn func(ef Factory, bufferRef Node) (Plan, error) // ApplyJoinPlanRightSideFn creates a plan for an iteration of ApplyJoin, given // a row produced from the left side. The plan is guaranteed to produce the // rightColumns passed to ConstructApplyJoin (in order). -type ApplyJoinPlanRightSideFn func(leftRow tree.Datums) (Plan, error) +type ApplyJoinPlanRightSideFn func(ef Factory, leftRow tree.Datums) (Plan, error) // Cascade describes a cascading query. The query uses a node created by // ConstructBuffer as an input; it should only be triggered if this buffer is diff --git a/pkg/sql/recursive_cte.go b/pkg/sql/recursive_cte.go index 568bdef376c9..c6c355aa076a 100644 --- a/pkg/sql/recursive_cte.go +++ b/pkg/sql/recursive_cte.go @@ -116,7 +116,7 @@ func (n *recursiveCTENode) Next(params runParams) (bool, error) { bufferedRows: lastWorkingRows, label: n.label, } - newPlan, err := n.genIterationFn(buf) + newPlan, err := n.genIterationFn(newExecFactory(params.p), buf) if err != nil { return false, err } From 5f2f57e8fcd628f2913f3aaeb7ae5d6bdffb9892 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Mon, 17 Aug 2020 21:42:55 -0400 Subject: [PATCH 3/3] sql: use the new EXPLAIN infrastructure for UI plans This change reworks how the `roachpb.ExplainPlanTreeNode` tree (used by the UI to show statement plans) is populated. We use the new EXPLAIN infrastructure. The main change is that we need to decide upfront if we will need the plan or not. Release note (admin ui change): various improvements to the statement plans in the UI. --- pkg/sql/app_stats.go | 9 +- pkg/sql/conn_executor_exec.go | 10 +- pkg/sql/executor_statement_metrics.go | 2 +- pkg/sql/explain_plan.go | 4 - pkg/sql/explain_tree.go | 86 ------------ pkg/sql/explain_tree_test.go | 8 +- .../testdata/logic_test/statement_statistics | 2 - pkg/sql/opt/exec/explain/emit.go | 10 +- pkg/sql/opt/exec/explain/flags.go | 4 + pkg/sql/opt/exec/explain/output.go | 3 + pkg/sql/opt/exec/explain/result_columns.go | 2 +- pkg/sql/plan.go | 79 +++++------ pkg/sql/plan_opt.go | 11 +- pkg/sql/sem/tree/hide_constants.go | 6 +- pkg/sql/testdata/explain_tree | 128 ++++++++---------- 15 files changed, 146 insertions(+), 218 deletions(-) delete mode 100644 pkg/sql/explain_tree.go diff --git a/pkg/sql/app_stats.go b/pkg/sql/app_stats.go index 5fd90a0ac6a7..3edc8cf5433e 100644 --- a/pkg/sql/app_stats.go +++ b/pkg/sql/app_stats.go @@ -292,13 +292,14 @@ func (a *appStats) recordTransaction(txnTimeSec float64, ev txnEvent, implicit b // sample logical plan for its corresponding fingerprint. We use // `logicalPlanCollectionPeriod` to assess how frequently to sample logical // plans. -func (a *appStats) shouldSaveLogicalPlanDescription( - stmt *Statement, useDistSQL bool, vectorized bool, implicitTxn bool, err error, -) bool { +func (a *appStats) shouldSaveLogicalPlanDescription(stmt *Statement, implicitTxn bool) bool { if !sampleLogicalPlans.Get(&a.st.SV) { return false } - stats := a.getStatsForStmt(stmt, implicitTxn, err, false /* createIfNonexistent */) + // We don't know yet if we will hit an error, so we assume we don't. The worst + // that can happen is that for statements that always error out, we will + // always save the tree plan. + stats := a.getStatsForStmt(stmt, implicitTxn, nil /* error */, false /* createIfNonexistent */) if stats == nil { // Save logical plan the first time we see new statement fingerprint. return true diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go index e189814d6903..a57b621ff852 100644 --- a/pkg/sql/conn_executor_exec.go +++ b/pkg/sql/conn_executor_exec.go @@ -909,10 +909,12 @@ func (ex *connExecutor) dispatchToExecutionEngine( // makeExecPlan creates an execution plan and populates planner.curPlan using // the cost-based optimizer. func (ex *connExecutor) makeExecPlan(ctx context.Context, planner *planner) error { - planner.curPlan.init(planner.stmt, ex.appStats) - if planner.collectBundle { - planner.curPlan.savePlanString = true - } + savePlanString := planner.collectBundle + planner.curPlan.init( + planner.stmt, + ex.appStats, + savePlanString, + ) if err := planner.makeOptimizerPlan(ctx); err != nil { log.VEventf(ctx, 1, "optimizer plan failed: %v", err) diff --git a/pkg/sql/executor_statement_metrics.go b/pkg/sql/executor_statement_metrics.go index a12176876d62..2d8aa97a3ccf 100644 --- a/pkg/sql/executor_statement_metrics.go +++ b/pkg/sql/executor_statement_metrics.go @@ -159,7 +159,7 @@ func (ex *connExecutor) recordStatementSummary( } ex.statsCollector.recordStatement( - stmt, planner.curPlan.savedPlanForStats, + stmt, planner.curPlan.planForStats, flags.IsDistributed(), flags.IsSet(planFlagVectorized), flags.IsSet(planFlagImplicitTxn), automaticRetryCount, rowsAffected, err, parseLat, planLat, runLat, svcLat, execOverhead, stats, diff --git a/pkg/sql/explain_plan.go b/pkg/sql/explain_plan.go index bd866099f3c9..55429405e5de 100644 --- a/pkg/sql/explain_plan.go +++ b/pkg/sql/explain_plan.go @@ -30,10 +30,6 @@ const ( // explainSubqueryFmtFlags is the format for subqueries within `EXPLAIN SQL` statements. // Since these are individually run, we don't need to scrub any data from subqueries. explainSubqueryFmtFlags = tree.FmtSimple - - // sampledLogicalPlanFmtFlags is the format for sampled logical plans. Because these exposed - // in the Admin UI, sampled plans should be scrubbed of sensitive information. - sampledLogicalPlanFmtFlags = tree.FmtHideConstants ) // explainPlanNode wraps the logic for EXPLAIN as a planNode. diff --git a/pkg/sql/explain_tree.go b/pkg/sql/explain_tree.go deleted file mode 100644 index 69b0482b37b7..000000000000 --- a/pkg/sql/explain_tree.go +++ /dev/null @@ -1,86 +0,0 @@ -// Copyright 2018 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -package sql - -import ( - "context" - "fmt" - - "github.com/cockroachdb/cockroach/pkg/roachpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" - "github.com/cockroachdb/cockroach/pkg/sql/opt/exec/explain" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" - "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" - "github.com/cockroachdb/cockroach/pkg/util" - "github.com/cockroachdb/errors" -) - -// planToTree returns a representation of the plan as a -// roachpb.ExplainTreePlanNode tree. -func planToTree(ctx context.Context, top *planTop) *roachpb.ExplainTreePlanNode { - var ob explain.OutputBuilder - observer := planObserver{ - // We set followRowSourceToPlanNode to true, to instruct the plan observer - // to follow the edges from rowSourceToPlanNodes (indicating that the prior - // node was not plannable by DistSQL) to the original planNodes that were - // replaced by DistSQL nodes. This prevents the walk from ending at these - // special replacement nodes. - // TODO(jordan): this is pretty hacky. We should modify DistSQL physical - // planning to avoid mutating its input planNode tree instead. - followRowSourceToPlanNode: true, - enterNode: func(ctx context.Context, nodeName string, plan planNode) (bool, error) { - if plan == nil { - ob.EnterMetaNode(nodeName) - } else { - ob.EnterNode(nodeName, planColumns(plan), planReqOrdering(plan)) - } - return true, nil - }, - expr: func(_ observeVerbosity, nodeName, fieldName string, n int, expr tree.Expr) { - if expr == nil { - return - } - ob.AddField(fieldName, tree.AsStringWithFlags(expr, sampledLogicalPlanFmtFlags)) - }, - spans: func(nodeName, fieldName string, index *descpb.IndexDescriptor, spans []roachpb.Span, hardLimitSet bool) { - // TODO(jordan): it's expensive to serialize long span - // strings. It's unfortunate that we're still calling - // PrettySpans, just to check to see whether the output is - or - // not. Unfortunately it's not so clear yet how to write a - // shorter function. Suggestions welcome. - spanss := sqlbase.PrettySpans(index, spans, 2) - if spanss != "" { - if spanss == "-" { - spanss = getAttrForSpansAll(hardLimitSet) - } else { - // Spans contain literal values from the query and thus - // cannot be spelled out in the collected plan. - spanss = fmt.Sprintf("%d span%s", len(spans), util.Pluralize(int64(len(spans)))) - } - ob.AddField(fieldName, spanss) - } - }, - attr: func(nodeName, fieldName, attr string) { - ob.AddField(fieldName, attr) - }, - leaveNode: func(nodeName string, plan planNode) error { - ob.LeaveNode() - return nil - }, - } - - if err := observePlan( - ctx, &top.planComponents, observer, true /* returnError */, sampledLogicalPlanFmtFlags, - ); err != nil { - panic(errors.AssertionFailedf("error while walking plan to save it to statement stats: %s", err.Error())) - } - return ob.BuildProtoTree() -} diff --git a/pkg/sql/explain_tree_test.go b/pkg/sql/explain_tree_test.go index 580e30e0f9c7..25d0650b99fe 100644 --- a/pkg/sql/explain_tree_test.go +++ b/pkg/sql/explain_tree_test.go @@ -66,16 +66,16 @@ func TestPlanToTreeAndPlanToString(t *testing.T) { p.stmt = &Statement{Statement: stmt} p.curPlan.savePlanString = true + p.curPlan.savePlanForStats = true if err := p.makeOptimizerPlan(ctx); err != nil { t.Fatal(err) } + p.curPlan.flags.Set(planFlagExecDone) + p.curPlan.close(ctx) if d.Cmd == "plan-string" { - p.curPlan.flags.Set(planFlagExecDone) - p.curPlan.close(ctx) return p.curPlan.planString } - tree := planToTree(ctx, &p.curPlan) - treeYaml, err := yaml.Marshal(tree) + treeYaml, err := yaml.Marshal(p.curPlan.planForStats) if err != nil { t.Fatal(err) } diff --git a/pkg/sql/logictest/testdata/logic_test/statement_statistics b/pkg/sql/logictest/testdata/logic_test/statement_statistics index fa85e91efe0f..faa70234b6eb 100644 --- a/pkg/sql/logictest/testdata/logic_test/statement_statistics +++ b/pkg/sql/logictest/testdata/logic_test/statement_statistics @@ -120,7 +120,6 @@ SELECT sqrt(_) ! SELECT x FROM (VALUES (_, _, __more1__), (__more1__)) AS t (x) · SELECT x FROM test WHERE y = (_ / z) !+ SELECT x FROM test WHERE y IN (_, _, _ + x, _, _) · -SELECT x FROM test WHERE y IN (_, _, __more3__) · SELECT x FROM test WHERE y IN (_, _, __more3__) + SELECT x FROM test WHERE y NOT IN (_, _, __more3__) · SET CLUSTER SETTING "debug.panic_on_failed_assertions" = DEFAULT · @@ -147,7 +146,6 @@ SELECT _ FROM (VALUES (_, _, __more1__), (__more1__)) AS _ (_) SELECT _ FROM _ WHERE _ = (_ / _) SELECT _ FROM _ WHERE _ IN (_, _, _ + _, _, _) SELECT _ FROM _ WHERE _ IN (_, _, __more3__) -SELECT _ FROM _ WHERE _ IN (_, _, __more3__) SELECT _ FROM _ WHERE _ NOT IN (_, _, __more3__) SET CLUSTER SETTING "debug.panic_on_failed_assertions" = DEFAULT SET CLUSTER SETTING "debug.panic_on_failed_assertions" = _ diff --git a/pkg/sql/opt/exec/explain/emit.go b/pkg/sql/opt/exec/explain/emit.go index ae3f9c4bc6a7..9344a6697a72 100644 --- a/pkg/sql/opt/exec/explain/emit.go +++ b/pkg/sql/opt/exec/explain/emit.go @@ -641,7 +641,15 @@ func (e *emitter) emitSpans( e.ob.Attr(field, "FULL SCAN") } } else { - e.ob.Attr(field, e.spanFormatFn(table, index, scanParams)) + if e.ob.flags.HideValues { + n := len(scanParams.InvertedConstraint) + if scanParams.IndexConstraint != nil { + n = scanParams.IndexConstraint.Spans.Count() + } + e.ob.Attrf(field, "%d span%s", n, util.Pluralize(int64(n))) + } else { + e.ob.Attr(field, e.spanFormatFn(table, index, scanParams)) + } } } diff --git a/pkg/sql/opt/exec/explain/flags.go b/pkg/sql/opt/exec/explain/flags.go index 89ee432e37b9..094c0da428e8 100644 --- a/pkg/sql/opt/exec/explain/flags.go +++ b/pkg/sql/opt/exec/explain/flags.go @@ -20,6 +20,10 @@ type Flags struct { // ShowTypes indicates that the types of columns are shown. // If ShowTypes is true, then Verbose is also true. ShowTypes bool + // If HideValues is true, we hide fields that may contain values from the + // query (e.g. spans). Used internally for the plan visible in the UI. + // If HideValues is true, then Verbose must be false. + HideValues bool } // MakeFlags crates Flags from ExplainOptions. diff --git a/pkg/sql/opt/exec/explain/output.go b/pkg/sql/opt/exec/explain/output.go index 397a1f83fe0f..f9978072cb44 100644 --- a/pkg/sql/opt/exec/explain/output.go +++ b/pkg/sql/opt/exec/explain/output.go @@ -116,6 +116,9 @@ func (ob *OutputBuilder) Expr(key string, expr tree.TypedExpr, varColumns sqlbas if ob.flags.ShowTypes { flags |= tree.FmtShowTypes } + if ob.flags.HideValues { + flags |= tree.FmtHideConstants + } f := tree.NewFmtCtx(flags) f.SetIndexedVarFormat(func(ctx *tree.FmtCtx, idx int) { // Ensure proper quoting. diff --git a/pkg/sql/opt/exec/explain/result_columns.go b/pkg/sql/opt/exec/explain/result_columns.go index 54f383bce26a..1aa7615b8892 100644 --- a/pkg/sql/opt/exec/explain/result_columns.go +++ b/pkg/sql/opt/exec/explain/result_columns.go @@ -196,7 +196,7 @@ func getResultColumns( } return sqlbase.ShowTraceColumns, nil - case createTableOp, createTableAsOp, createViewOp, controlJobsOp, + case createTableOp, createTableAsOp, createViewOp, controlJobsOp, controlSchedulesOp, cancelQueriesOp, cancelSessionsOp, errorIfRowsOp, deleteRangeOp: // These operations produce no columns. return nil, nil diff --git a/pkg/sql/plan.go b/pkg/sql/plan.go index f1df28b7905f..c811778ce47e 100644 --- a/pkg/sql/plan.go +++ b/pkg/sql/plan.go @@ -12,6 +12,7 @@ package sql import ( "context" + "fmt" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" @@ -25,7 +26,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" - "github.com/cockroachdb/errors" + "github.com/cockroachdb/cockroach/pkg/util/log" ) // runParams is a struct containing all parameters passed to planNode.Next() and @@ -301,14 +302,18 @@ type planTop struct { // If we are collecting query diagnostics, flow diagrams are saved here. distSQLDiagrams []execinfrapb.FlowDiagram - appStats *appStats - savedPlanForStats *roachpb.ExplainTreePlanNode + // If savePlanForStats is true, an ExplainTreePlanNode tree will be saved in + // planForStats when the plan is closed. + savePlanForStats bool + // appStats is used to populate savePlanForStats. + appStats *appStats + planForStats *roachpb.ExplainTreePlanNode // If savePlanString is set to true, an EXPLAIN (VERBOSE)-style plan string - // will be saved in planString. + // will be saved in planString when the plan is closed. savePlanString bool - explainPlan *explain.Plan planString string + explainPlan *explain.Plan } // physicalPlanTop is a utility wrapper around PhysicalPlan that allows for @@ -419,40 +424,24 @@ func (p *planComponents) close(ctx context.Context) { // init resets planTop to point to a given statement; used at the start of the // planning process. -func (p *planTop) init(stmt *Statement, appStats *appStats) { - *p = planTop{stmt: stmt, appStats: appStats} +func (p *planTop) init(stmt *Statement, appStats *appStats, savePlanString bool) { + *p = planTop{ + stmt: stmt, + appStats: appStats, + savePlanString: savePlanString, + } } // close ensures that the plan's resources have been deallocated. func (p *planTop) close(ctx context.Context) { - if p.main.planNode != nil && p.flags.IsSet(planFlagExecDone) { - // TODO(yuzefovich): update this once we support creating table reader - // specs directly in the optimizer (see #47474). - if p.appStats != nil && p.appStats.shouldSaveLogicalPlanDescription( - p.stmt, - p.flags.IsDistributed(), - p.flags.IsSet(planFlagVectorized), - p.flags.IsSet(planFlagImplicitTxn), - p.execErr, - ) { - p.savedPlanForStats = planToTree(ctx, p) - } - - if p.savePlanString && p.explainPlan != nil { - var err error - p.planString, err = p.formatExplain() - if err != nil { - p.planString = err.Error() - } - } + if p.explainPlan != nil && p.flags.IsSet(planFlagExecDone) { + p.savePlanInfo(ctx) } p.planComponents.close(ctx) } -func (p *planTop) formatExplain() (string, error) { - if p.explainPlan == nil { - return "", errors.AssertionFailedf("no plan") - } +// savePlanInfo uses p.explainPlan to populate the plan string and/or tree. +func (p *planTop) savePlanInfo(ctx context.Context) { vectorized := p.flags.IsSet(planFlagVectorized) distribution := physicalplan.LocalPlan if p.flags.IsSet(planFlagFullyDistributed) { @@ -461,14 +450,28 @@ func (p *planTop) formatExplain() (string, error) { distribution = physicalplan.PartiallyDistributedPlan } - ob := explain.NewOutputBuilder(explain.Flags{ - Verbose: true, - ShowTypes: true, - }) - if err := emitExplain(ob, p.codec, p.explainPlan, distribution, vectorized); err != nil { - return "", err + if p.savePlanForStats { + ob := explain.NewOutputBuilder(explain.Flags{ + HideValues: true, + }) + if err := emitExplain(ob, p.codec, p.explainPlan, distribution, vectorized); err != nil { + log.Warningf(ctx, "unable to emit explain plan tree: %v", err) + } else { + p.planForStats = ob.BuildProtoTree() + } + } + + if p.savePlanString { + ob := explain.NewOutputBuilder(explain.Flags{ + Verbose: true, + ShowTypes: true, + }) + if err := emitExplain(ob, p.codec, p.explainPlan, distribution, vectorized); err != nil { + p.planString = fmt.Sprintf("error emitting plan: %v", err) + } else { + p.planString = ob.BuildString() + } } - return ob.BuildString(), nil } // formatOptPlan returns a visual representation of the optimizer plan that was diff --git a/pkg/sql/plan_opt.go b/pkg/sql/plan_opt.go index 543c556cc3db..f6d78364613e 100644 --- a/pkg/sql/plan_opt.go +++ b/pkg/sql/plan_opt.go @@ -552,7 +552,16 @@ func (opc *optPlanningCtx) runExecBuilder( var result *planComponents var explainPlan *explain.Plan var isDDL bool - if !planTop.savePlanString { + if planTop.appStats != nil { + // We do not set this flag upfront when initializing planTop because the + // planning process could in principle modify the AST, resulting in a + // different statement signature. + planTop.savePlanForStats = planTop.appStats.shouldSaveLogicalPlanDescription( + planTop.stmt, + allowAutoCommit, + ) + } + if !planTop.savePlanString && !planTop.savePlanForStats { // No instrumentation. bld := execbuilder.New(f, mem, &opc.catalog, mem.RootExpr(), evalCtx, allowAutoCommit) plan, err := bld.Build() diff --git a/pkg/sql/sem/tree/hide_constants.go b/pkg/sql/sem/tree/hide_constants.go index bf133f845dcb..40bf1302e81a 100644 --- a/pkg/sql/sem/tree/hide_constants.go +++ b/pkg/sql/sem/tree/hide_constants.go @@ -118,9 +118,9 @@ func (node *Tuple) formatHideConstants(ctx *FmtCtx) { v2.Exprs = append(make(Exprs, 0, 3), v2.Exprs[:2]...) if len(node.Exprs) > 2 { v2.Exprs = append(v2.Exprs, arityIndicator(len(node.Exprs)-2)) - } - if node.Labels != nil { - v2.Labels = node.Labels[:2] + if node.Labels != nil { + v2.Labels = node.Labels[:2] + } } v2.Format(ctx) return diff --git a/pkg/sql/testdata/explain_tree b/pkg/sql/testdata/explain_tree index 10e8b8741848..58cf5849d7be 100644 --- a/pkg/sql/testdata/explain_tree +++ b/pkg/sql/testdata/explain_tree @@ -18,6 +18,8 @@ SELECT oid FROM t.orders WHERE oid = 123 ---- name: scan attrs: +- key: missing stats + value: "" - key: table value: orders@primary - key: spans @@ -38,17 +40,13 @@ project (cid int, date date, value plan-tree SELECT cid, date, value FROM t.orders ---- -name: render -attrs: -- key: render - value: cid -- key: render - value: date -- key: render - value: value +name: project +attrs: [] children: - name: scan attrs: + - key: missing stats + value: "" - key: table value: orders@primary - key: spans @@ -85,12 +83,8 @@ project plan-tree SELECT cid, sum(value) FROM t.orders WHERE date > '2015-01-01' GROUP BY cid ORDER BY 1 - sum(value) ---- -name: render -attrs: -- key: render - value: cid -- key: render - value: sum +name: project +attrs: [] children: - name: sort attrs: @@ -98,13 +92,7 @@ children: value: +column7 children: - name: render - attrs: - - key: render - value: _ - sum - - key: render - value: cid - - key: render - value: sum + attrs: [] children: - name: group attrs: @@ -113,12 +101,8 @@ children: - key: group by value: cid children: - - name: render - attrs: - - key: render - value: cid - - key: render - value: value + - name: project + attrs: [] children: - name: filter attrs: @@ -127,6 +111,8 @@ children: children: - name: scan attrs: + - key: missing stats + value: "" - key: table value: orders@primary - key: spans @@ -148,6 +134,8 @@ SELECT value FROM (SELECT cid, date, value FROM t.orders) ---- name: scan attrs: +- key: missing stats + value: "" - key: table value: orders@primary - key: spans @@ -182,43 +170,44 @@ project (cid int, d plan-tree SELECT cid, date, value FROM t.orders WHERE date IN (SELECT date FROM t.orders) ---- -name: render -attrs: -- key: render - value: cid -- key: render - value: date -- key: render - value: value +name: project +attrs: [] children: -- name: hash join - attrs: - - key: type - value: inner - - key: equality - value: (date) = (date) - - key: right cols are key - value: "" +- name: project + attrs: [] children: - - name: scan + - name: hash join attrs: - - key: table - value: orders@primary - - key: spans - value: FULL SCAN - children: [] - - name: distinct - attrs: - - key: distinct on - value: date + - key: type + value: inner + - key: equality + value: (date) = (date) + - key: right cols are key + value: "" children: - name: scan attrs: + - key: missing stats + value: "" - key: table value: orders@primary - key: spans value: FULL SCAN children: [] + - name: distinct + attrs: + - key: distinct on + value: date + children: + - name: scan + attrs: + - key: missing stats + value: "" + - key: table + value: orders@primary + - key: spans + value: FULL SCAN + children: [] exec CREATE TABLE t.movies ( @@ -272,28 +261,27 @@ SELECT id AS movie_id, title, (SELECT name FROM t.actors WHERE name = 'Foo') FRO name: root attrs: [] children: -- name: render - attrs: - - key: render - value: id - - key: render - value: title - - key: render - value: (SELECT name FROM t.actors WHERE name = _) +- name: project + attrs: [] children: - - name: scan - attrs: - - key: table - value: movies@primary - - key: spans - value: FULL SCAN - children: [] + - name: render + attrs: [] + children: + - name: scan + attrs: + - key: missing stats + value: "" + - key: table + value: movies@primary + - key: spans + value: FULL SCAN + children: [] - name: subquery attrs: - key: id value: '@S1' - key: original sql - value: (SELECT name FROM t.actors WHERE name = _) + value: (SELECT name FROM t.actors WHERE name = 'Foo') - key: exec mode value: one row children: @@ -307,6 +295,8 @@ children: children: - name: scan attrs: + - key: missing stats + value: "" - key: table value: actors@primary - key: spans