Skip to content

Commit abefdb6

Browse files
craig[bot]yuzefovich
craig[bot]
and
yuzefovich
committed
Merge #143088
143088: sql: fix recent regression in EXPLAIN ANALYZE r=yuzefovich a=yuzefovich This commit fixes a regression in EXPLAIN ANALYZE output recently added in 44da781. Before that change we always performed the association of the current planNode with the last stage of the physical plan, and after that change we started doing the association only when a new stage is added. However, this resulted in missing associations when a particular planNode didn't result in a new stage of the physical plan (for example, renderNode is handled as a projection by adjusting PostProcessSpec of already existing stage). As a result, we could lose the ability to associate execution statistics to some nodes in EXPLAIN ANALYZE output. This is now fixed by bringing back the unconditional call to associate the current planNode with the last stage of the physical plan, even if a new stage isn't added. This required teaching `associateNodeWithComponents` to "swallow" duplicate associations and make them no-ops (otherwise, we would double-count statistics in some cases). Epic: None Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]>
2 parents 7984c75 + d7a8dbe commit abefdb6

File tree

4 files changed

+140
-3
lines changed

4 files changed

+140
-3
lines changed

‎pkg/backup/backup_compaction_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,6 +428,8 @@ func TestScheduledBackupCompaction(t *testing.T) {
428428
defer leaktest.AfterTest(t)()
429429
defer log.Scope(t).Close(t)
430430

431+
skip.WithIssue(t, 143394, "flaky test")
432+
431433
ctx := context.Background()
432434
th, cleanup := newTestHelper(t)
433435
defer cleanup()

‎pkg/sql/distsql_physical_planner.go‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,8 +1066,9 @@ func (p *PlanningCtx) setUpForMainQuery(
10661066
}
10671067

10681068
// associateWithPlanNode returns a callback function (possibly nil) that should
1069-
// be invoked when a new stage of processors is added that corresponds to the
1070-
// given planNode.
1069+
// be invoked to associate the last stage of processors with the given planNode.
1070+
// The returned function can be safely invoked multiple times for the given
1071+
// planNode and the same last stage of processors.
10711072
func (p *PlanningCtx) associateWithPlanNode(node planNode) func(*physicalplan.PhysicalPlan) {
10721073
if p.associateNodeWithComponents == nil {
10731074
return nil
@@ -4227,6 +4228,19 @@ func (dsp *DistSQLPlanner) createPhysPlanForPlanNode(
42274228
return nil, err
42284229
}
42294230

4231+
if associateWithPlanNode := planCtx.associateWithPlanNode(node); associateWithPlanNode != nil {
4232+
// Even though we might not have created a new stage in the physical
4233+
// plan for the current planNode, we still need to associate it with the
4234+
// last stage. This is needed to handle cases where a single physical
4235+
// plan stage handles multiple planNodes (e.g. renderNode that is done
4236+
// by adjusting PostProcessSpec).
4237+
//
4238+
// Note that it is ok if we already did the association with the current
4239+
// planNode when created the last stage of the physical plan - the
4240+
// callback handles this case safely (i.e. without double-counting).
4241+
associateWithPlanNode(&plan.PhysicalPlan)
4242+
}
4243+
42304244
return plan, err
42314245
}
42324246

‎pkg/sql/instrumentation.go‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"bytes"
1010
"context"
1111
"fmt"
12+
"slices"
1213
"time"
1314

1415
"github.com/cockroachdb/cockroach/pkg/keys"
@@ -959,11 +960,22 @@ type execNodeTraceMetadata map[exec.Node][]execComponents
959960
type execComponents []execinfrapb.ComponentID
960961

961962
// associateNodeWithComponents is called during planning, as processors are
962-
// planned for an execution operator.
963+
// planned for an execution operator. This function can be called multiple times
964+
// for the same exec.Node and execComponents.
963965
func (m execNodeTraceMetadata) associateNodeWithComponents(
964966
node exec.Node, components execComponents,
965967
) {
966968
if prevComponents, ok := m[node]; ok {
969+
// We already have some components associated with this node. Check
970+
// whether this is a duplicate association (that should be a no-op).
971+
for _, oldComponents := range prevComponents {
972+
if slices.Equal(oldComponents, components) {
973+
// This association has already been performed.
974+
return
975+
}
976+
}
977+
// This must be a new stage in the physical plan, so we want to extend
978+
// the mapping for the exec.Node.
967979
m[node] = append(prevComponents, components)
968980
} else {
969981
m[node] = []execComponents{components}

‎pkg/sql/opt/exec/execbuilder/testdata/explain_analyze‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,3 +82,112 @@ vectorized: true
8282
estimated row count: 10 (missing stats)
8383
table: privileges@privileges_path_user_id_key
8484
spans: /"vtable/crdb_internal/tables"-/"vtable/crdb_internal/tables"/PrefixEnd
85+
86+
# Regression test for not showing the execution statistics that correspond to
87+
# scans of the virtual tables.
88+
query T
89+
EXPLAIN ANALYZE SHOW TABLES;
90+
----
91+
planning time: 10µs
92+
execution time: 100µs
93+
distribution: <hidden>
94+
vectorized: <hidden>
95+
plan type: custom
96+
maximum memory usage: <hidden>
97+
network usage: <hidden>
98+
regions: <hidden>
99+
isolation level: serializable
100+
priority: normal
101+
quality of service: regular
102+
·
103+
• sort
104+
│ sql nodes: <hidden>
105+
│ regions: <hidden>
106+
│ actual row count: 1
107+
│ estimated max memory allocated: 0 B
108+
│ order: +nspname,+relname
109+
│
110+
└── • render
111+
│
112+
└── • hash join (left outer)
113+
│ sql nodes: <hidden>
114+
│ regions: <hidden>
115+
│ actual row count: 1
116+
│ estimated max memory allocated: 0 B
117+
│ equality: (column80) = (table_id)
118+
│
119+
├── • render
120+
│ │
121+
│ └── • hash join (left outer)
122+
│ │ sql nodes: <hidden>
123+
│ │ regions: <hidden>
124+
│ │ actual row count: 1
125+
│ │ estimated max memory allocated: 0 B
126+
│ │ equality: (column62) = (table_id)
127+
│ │ right cols are key
128+
│ │
129+
│ ├── • render
130+
│ │ │
131+
│ │ └── • hash join (right outer)
132+
│ │ │ sql nodes: <hidden>
133+
│ │ │ regions: <hidden>
134+
│ │ │ actual row count: 1
135+
│ │ │ estimated max memory allocated: 0 B
136+
│ │ │ equality: (oid) = (relowner)
137+
│ │ │
138+
│ │ ├── • virtual table
139+
│ │ │ sql nodes: <hidden>
140+
│ │ │ regions: <hidden>
141+
│ │ │ actual row count: 4
142+
│ │ │ table: pg_roles@primary
143+
│ │ │
144+
│ │ └── • hash join
145+
│ │ │ sql nodes: <hidden>
146+
│ │ │ regions: <hidden>
147+
│ │ │ actual row count: 1
148+
│ │ │ estimated max memory allocated: 0 B
149+
│ │ │ equality: (oid) = (relnamespace)
150+
│ │ │
151+
│ │ ├── • filter
152+
│ │ │ │ sql nodes: <hidden>
153+
│ │ │ │ regions: <hidden>
154+
│ │ │ │ actual row count: 1
155+
│ │ │ │ filter: nspname NOT IN ('crdb_internal', 'information_schema', __more1_10__, 'pg_extension')
156+
│ │ │ │
157+
│ │ │ └── • virtual table
158+
│ │ │ sql nodes: <hidden>
159+
│ │ │ regions: <hidden>
160+
│ │ │ actual row count: 5
161+
│ │ │ table: pg_namespace@primary
162+
│ │ │
163+
│ │ └── • filter
164+
│ │ │ sql nodes: <hidden>
165+
│ │ │ regions: <hidden>
166+
│ │ │ actual row count: 330
167+
│ │ │ filter: relkind IN ('S', 'm', __more1_10__, 'v')
168+
│ │ │
169+
│ │ └── • virtual table
170+
│ │ sql nodes: <hidden>
171+
│ │ regions: <hidden>
172+
│ │ actual row count: 362
173+
│ │ table: pg_class@primary
174+
│ │
175+
│ └── • distinct
176+
│ │ sql nodes: <hidden>
177+
│ │ regions: <hidden>
178+
│ │ actual row count: 330
179+
│ │ estimated max memory allocated: 0 B
180+
│ │ distinct on: table_id
181+
│ │
182+
│ └── • virtual table
183+
│ sql nodes: <hidden>
184+
│ regions: <hidden>
185+
│ actual row count: 330
186+
│ table: table_row_statistics@primary
187+
│
188+
└── • virtual table
189+
sql nodes: <hidden>
190+
regions: <hidden>
191+
actual row count: 1
192+
table: tables@tables_database_name_idx (partial index)
193+
spans: [/'test' - /'test']

0 commit comments

Comments
 (0)