Merge #58208

58208: sql: fix crash when explaining a constrained virtual table scan r=RaduBerinde a=RaduBerinde

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.

Co-authored-by: Radu Berinde <[email protected]>

pkg/sql/explain_plan.go

@@ -18,15 +18,14 @@ 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"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec"
 	"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 "<virtual table spans>"
 		}
+		tabDesc := table.(*optTable).desc
+		idxDesc := index.(*optIndex).desc
 		spans, err := generateScanSpans(evalCtx, codec, tabDesc, idxDesc, scanParams)
 		if err != nil {
 			return err.Error()

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]

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

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)
 	}
 

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{}))