From d0b3e2db4c557149b004ffb902161df08c46dbe4 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Tue, 22 Dec 2020 16:24:19 -0500 Subject: [PATCH] sql: fix crash when explaining a constrained virtual table scan When emitting `EXPLAIN (VERBOSE)` output, we use a callback to generate and format the physical spans; this callback currently crashes for virtual tables. Note that this code path is used when collecting statement diagnostics, which can lead to a severe situation. The fix is to always show the logical spans and use the callback only for physical tables. In addition, we add logic to catch panics inside emitExplain; unfortunately, it would not have helped here because the panic object was a bare string (and not an assertion error). Fixes #58193. Release note (bug fix): fixed a "column family 0 not found" crash caused by explaining or gathering statement diagnostics on certain queries involving virtual tables. --- pkg/sql/explain_plan.go | 31 +++++++++++++------ pkg/sql/opt/exec/execbuilder/testdata/explain | 26 ++++++++++++++++ pkg/sql/opt/exec/execbuilder/testdata/scalar | 2 +- pkg/sql/opt/exec/explain/emit.go | 9 +++--- pkg/sql/rowenc/index_encoding.go | 6 ++-- 5 files changed, 56 insertions(+), 18 deletions(-) diff --git a/pkg/sql/explain_plan.go b/pkg/sql/explain_plan.go index 93af4a9c20e5..f28080edd0c6 100644 --- a/pkg/sql/explain_plan.go +++ b/pkg/sql/explain_plan.go @@ -18,8 +18,6 @@ import ( "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/catalog/descpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/colflow" "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb" "github.com/cockroachdb/cockroach/pkg/sql/opt/cat" @@ -27,6 +25,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/opt/exec/explain" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" + "github.com/cockroachdb/cockroach/pkg/util/errorutil" "github.com/cockroachdb/errors" ) @@ -150,17 +149,29 @@ func emitExplain( evalCtx *tree.EvalContext, codec keys.SQLCodec, explainPlan *explain.Plan, -) error { +) (err error) { + // Guard against bugs in the explain code. + defer func() { + if r := recover(); r != nil { + // This code allows us to propagate internal and runtime errors without + // having to add error checks everywhere throughout the code. This is only + // possible because the code does not update shared state and does not + // manipulate locks. + if ok, e := errorutil.ShouldCatch(r); ok { + err = e + } else { + // Other panic objects can't be considered "safe" and thus are + // propagated as crashes that terminate the session. + panic(r) + } + } + }() spanFormatFn := func(table cat.Table, index cat.Index, scanParams exec.ScanParams) string { - var tabDesc *tabledesc.Immutable - var idxDesc *descpb.IndexDescriptor if table.IsVirtualTable() { - tabDesc = table.(*optVirtualTable).desc - idxDesc = index.(*optVirtualIndex).desc - } else { - tabDesc = table.(*optTable).desc - idxDesc = index.(*optIndex).desc + return "" } + tabDesc := table.(*optTable).desc + idxDesc := index.(*optIndex).desc spans, err := generateScanSpans(evalCtx, codec, tabDesc, idxDesc, scanParams) if err != nil { return err.Error() diff --git a/pkg/sql/opt/exec/execbuilder/testdata/explain b/pkg/sql/opt/exec/execbuilder/testdata/explain index 80e7ac85316c..1983c8ea22b3 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/explain +++ b/pkg/sql/opt/exec/execbuilder/testdata/explain @@ -1431,3 +1431,29 @@ vectorized: true missing stats table: u@primary spans: [/1 - /1] + +# Make sure EXPLAIN (VERBOSE) works when there is a constrained scan of a +# virtual table (#58193). +statement ok +CREATE TABLE ab (a INT, b INT) + +query T +EXPLAIN (VERBOSE) SELECT attnum FROM pg_attribute WHERE attrelid = 'ab'::regclass AND attname = 'b'; +---- +distribution: local +vectorized: true +· +• project +│ columns: (attnum) +│ estimated row count: 1 (missing stats) +│ +└── • filter + │ columns: (attrelid, attname, attnum) + │ estimated row count: 1 (missing stats) + │ filter: attname = 'b' + │ + └── • virtual table + columns: (attrelid, attname, attnum) + estimated row count: 10 (missing stats) + table: pg_attribute@pg_attribute_attrelid_idx + spans: [/ab - /ab] diff --git a/pkg/sql/opt/exec/execbuilder/testdata/scalar b/pkg/sql/opt/exec/execbuilder/testdata/scalar index 23b8256f3bd3..867a7d8fe2b3 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/scalar +++ b/pkg/sql/opt/exec/execbuilder/testdata/scalar @@ -1061,7 +1061,7 @@ vectorized: true columns: (attrelid, attname, atttypid, attstattarget, attlen, attnum, attndims, attcacheoff, atttypmod, attbyval, attstorage, attalign, attnotnull, atthasdef, attidentity, attgenerated, attisdropped, attislocal, attinhcount, attcollation, attacl, attoptions, attfdwoptions) estimated row count: 10 (missing stats) table: pg_attribute@pg_attribute_attrelid_idx - spans: /53-/54 + spans: [/t - /t] query T EXPLAIN (VERBOSE) SELECT CASE WHEN current_database() = 'test' THEN 42 ELSE 1/3 END diff --git a/pkg/sql/opt/exec/explain/emit.go b/pkg/sql/opt/exec/explain/emit.go index 3f0e835e7f41..24ee0450466c 100644 --- a/pkg/sql/opt/exec/explain/emit.go +++ b/pkg/sql/opt/exec/explain/emit.go @@ -116,8 +116,9 @@ func Emit(plan *Plan, ob *OutputBuilder, spanFormatFn SpanFormatFn) error { return nil } -// SpanFormatFn is a function used to format spans for EXPLAIN. Only called when -// there is an index constraint or an inverted constraint. +// SpanFormatFn is a function used to format spans for EXPLAIN. Only called on +// non-virtual tables, when there is an index constraint or an inverted +// constraint. type SpanFormatFn func(table cat.Table, index cat.Index, scanParams exec.ScanParams) string // omitTrivialProjections returns the given node and its result columns and @@ -723,8 +724,8 @@ func (e *emitter) spansStr(table cat.Table, index cat.Index, scanParams exec.Sca return "FULL SCAN" } - // In verbose mode show the physical spans. - if e.ob.flags.Verbose { + // In verbose mode show the physical spans, unless the table is virtual. + if e.ob.flags.Verbose && !table.IsVirtualTable() { return e.spanFormatFn(table, index, scanParams) } diff --git a/pkg/sql/rowenc/index_encoding.go b/pkg/sql/rowenc/index_encoding.go index e49ddaebabbc..60a394389c1a 100644 --- a/pkg/sql/rowenc/index_encoding.go +++ b/pkg/sql/rowenc/index_encoding.go @@ -358,7 +358,7 @@ func NeededColumnFamilyIDs( return nil }) if family0 == nil { - panic("column family 0 not found") + panic(errors.AssertionFailedf("column family 0 not found")) } // If all the needed families are nullable, we also need family 0 as a @@ -1367,11 +1367,11 @@ func EncodeSecondaryIndexes( indexBoundAccount mon.BoundAccount, ) ([]IndexEntry, error) { if len(secondaryIndexEntries) > 0 { - panic("Length of secondaryIndexEntries was non-zero") + panic(errors.AssertionFailedf("length of secondaryIndexEntries was non-zero")) } if indexBoundAccount.Monitor() == nil { - panic("Memory monitor passed to EncodeSecondaryIndexes was nil") + panic(errors.AssertionFailedf("memory monitor passed to EncodeSecondaryIndexes was nil")) } const sizeOfIndexEntry = int64(unsafe.Sizeof(IndexEntry{}))