From 5f2f57e8fcd628f2913f3aaeb7ae5d6bdffb9892 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Mon, 17 Aug 2020 21:42:55 -0400 Subject: [PATCH] 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