Merge pull request #58243 from RaduBerinde/backport20.2-58208

release-20.2: sql: fix crash when explaining a constrained virtual table scan

pkg/sql/explain_plan.go

@@ -17,15 +17,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/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/physicalplan"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
+	"github.com/cockroachdb/cockroach/pkg/util/errorutil"
 )
 
 // explainPlanNode implements EXPLAIN (PLAN); it produces the output of
@@ -67,19 +66,31 @@ func emitExplain(
 	explainPlan *explain.Plan,
 	distribution physicalplan.PlanDistribution,
 	vectorized bool,
-) 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)
+			}
+		}
+	}()
 	ob.AddField("distribution", distribution.String())
 	ob.AddField("vectorized", fmt.Sprintf("%t", vectorized))
 	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(codec, tabDesc, idxDesc, scanParams)
 		if err != nil {
 			return err.Error()

pkg/sql/opt/exec/execbuilder/testdata/explain

@@ -1203,3 +1203,23 @@ insert                                       ·                      ·
 ·                                            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 TTTTT
+EXPLAIN (VERBOSE) SELECT attnum FROM pg_attribute WHERE attrelid = 'ab'::regclass AND attname = 'b';
+----
+·                        distribution         local                                   ·                            ·
+·                        vectorized           false                                   ·                            ·
+project                  ·                    ·                                       (attnum)                     ·
+ │                       estimated row count  1 (missing stats)                       ·                            ·
+ └── filter              ·                    ·                                       (attrelid, attname, attnum)  ·
+      │                  estimated row count  1 (missing stats)                       ·                            ·
+      │                  filter               attname = 'b'                           ·                            ·
+      └── virtual table  ·                    ·                                       (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

@@ -821,7 +821,7 @@ EXPLAIN (VERBOSE) SELECT * FROM pg_attribute WHERE attrelid='t'::regclass
 virtual table  ·                    ·                                       (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 TTTTT
 EXPLAIN (VERBOSE) SELECT CASE WHEN current_database() = 'test' THEN 42 ELSE 1/3 END

pkg/sql/opt/exec/explain/emit.go

@@ -112,8 +112,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

@@ -359,7 +359,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
@@ -1238,11 +1238,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{}))