From 02e9d17f3f599b3a727b145c587853942e792591 Mon Sep 17 00:00:00 2001
From: arulajmani <arulajmani@gmail.com>
Date: Fri, 4 Dec 2020 15:47:17 -0500
Subject: [PATCH 1/9] sql: lift multi-region functions on to immutable
 descriptors

Previously, we added some accessors on type and database descriptors
that operated on the raw proto. Since, I've learned that this stuff
should really be on immutable types instead, as we're moving to a new
world where we move away from raw proto accesses. This patch lifts
accessors from the raw proto and on to the immutable types.

Release note: None
---
 pkg/sql/catalog/dbdesc/database_desc.go | 24 ++++++++++++++++++
 pkg/sql/catalog/descpb/structured.go    | 33 -------------------------
 pkg/sql/catalog/typedesc/type_desc.go   |  9 +++++++
 3 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/pkg/sql/catalog/dbdesc/database_desc.go b/pkg/sql/catalog/dbdesc/database_desc.go
index 087e987f9f19..8bec84cffcc3 100644
--- a/pkg/sql/catalog/dbdesc/database_desc.go
+++ b/pkg/sql/catalog/dbdesc/database_desc.go
@@ -206,6 +206,30 @@ func (desc *Immutable) DescriptorProto() *descpb.Descriptor {
 	}
 }
 
+// IsMultiRegion returns whether the database has multi-region properties
+// configured. If so, desc.RegionConfig can be used.
+func (desc *Immutable) IsMultiRegion() bool {
+	return desc.RegionConfig != nil
+}
+
+// Regions returns the multi-region regions that have been added to a database.
+func (desc *Immutable) Regions() (descpb.Regions, error) {
+	if !desc.IsMultiRegion() {
+		return nil, errors.AssertionFailedf(
+			"can not get regions of a non multi-region database")
+	}
+	return desc.RegionConfig.Regions, nil
+}
+
+// PrimaryRegion returns the primary region for a multi-region database.
+func (desc *Immutable) PrimaryRegion() (descpb.Region, error) {
+	if !desc.IsMultiRegion() {
+		return "", errors.AssertionFailedf(
+			"can not get the primary region of a non multi-region database")
+	}
+	return desc.RegionConfig.PrimaryRegion, nil
+}
+
 // SetName sets the name on the descriptor.
 func (desc *Mutable) SetName(name string) {
 	desc.Name = name
diff --git a/pkg/sql/catalog/descpb/structured.go b/pkg/sql/catalog/descpb/structured.go
index e172b48cb3b2..1d4105d0ff9a 100644
--- a/pkg/sql/catalog/descpb/structured.go
+++ b/pkg/sql/catalog/descpb/structured.go
@@ -324,36 +324,3 @@ func (DescriptorState) SafeValue() {}
 
 // SafeValue implements the redact.SafeValue interface.
 func (ConstraintType) SafeValue() {}
-
-// IsMultiRegion returns whether the database has multi-region properties
-// configured. If so, desc.RegionConfig can be used.
-func (desc *DatabaseDescriptor) IsMultiRegion() bool {
-	return desc.RegionConfig != nil
-}
-
-// Regions returns the multi-region regions that have been added to a database.
-func (desc *DatabaseDescriptor) Regions() (Regions, error) {
-	if !desc.IsMultiRegion() {
-		return nil, errors.AssertionFailedf(
-			"can not get regions of a non multi-region database")
-	}
-	return desc.RegionConfig.Regions, nil
-}
-
-// PrimaryRegion returns the primary region for a multi-region database.
-func (desc *DatabaseDescriptor) PrimaryRegion() (Region, error) {
-	if !desc.IsMultiRegion() {
-		return "", errors.AssertionFailedf(
-			"can not get the primary region of a non multi-region database")
-	}
-	return desc.RegionConfig.PrimaryRegion, nil
-}
-
-// PrimaryRegion returns the primary region for a multi-region type descriptor.
-func (desc *TypeDescriptor) PrimaryRegion() (Region, error) {
-	if desc.Kind != TypeDescriptor_MULTIREGION_ENUM {
-		return "", errors.AssertionFailedf(
-			"can not get primary region of a non multi-region type desc")
-	}
-	return desc.RegionConfig.PrimaryRegion, nil
-}
diff --git a/pkg/sql/catalog/typedesc/type_desc.go b/pkg/sql/catalog/typedesc/type_desc.go
index a502f3d2acc5..9d065e954f2d 100644
--- a/pkg/sql/catalog/typedesc/type_desc.go
+++ b/pkg/sql/catalog/typedesc/type_desc.go
@@ -201,6 +201,15 @@ func (desc *Immutable) DescriptorProto() *descpb.Descriptor {
 	}
 }
 
+// PrimaryRegion returns the primary region for a multi-region type descriptor.
+func (desc *Immutable) PrimaryRegion() (descpb.Region, error) {
+	if desc.Kind != descpb.TypeDescriptor_MULTIREGION_ENUM {
+		return "", errors.AssertionFailedf(
+			"can not get primary region of a non multi-region type desc")
+	}
+	return desc.RegionConfig.PrimaryRegion, nil
+}
+
 // SetDrainingNames implements the MutableDescriptor interface.
 func (desc *Mutable) SetDrainingNames(names []descpb.NameInfo) {
 	desc.DrainingNames = names

From 4945cb5d14ffcb6977fc6ff72c41e4505be99d6f Mon Sep 17 00:00:00 2001
From: Radu Berinde <radu@cockroachlabs.com>
Date: Sat, 5 Dec 2020 13:43:52 -0500
Subject: [PATCH 2/9] sql: support virtual columns in parser

Support VIRTUAL keyword in the parser. We still error out if it is
ever used.

Release note: None
---
 docs/generated/sql/bnf/col_qualification.bnf     |  4 ++++
 docs/generated/sql/bnf/stmt_block.bnf            |  1 +
 pkg/sql/add_column.go                            |  4 ++++
 pkg/sql/create_table.go                          |  4 ++++
 pkg/sql/logictest/testdata/logic_test/computed   | 11 ++++++++++-
 pkg/sql/parser/parse_test.go                     |  3 ++-
 pkg/sql/parser/sql.y                             |  4 ++--
 pkg/sql/sem/tree/create.go                       | 16 ++++++++++++++--
 pkg/sql/sem/tree/pretty.go                       | 15 +++++++++++++--
 .../create_table.align-deindent.golden.short     |  5 +++++
 .../pretty/create_table.align-only.golden.short  |  5 +++++
 .../pretty/create_table.ref.golden.short         |  3 +++
 .../sem/tree/testdata/pretty/create_table.sql    |  1 +
 13 files changed, 68 insertions(+), 8 deletions(-)

diff --git a/docs/generated/sql/bnf/col_qualification.bnf b/docs/generated/sql/bnf/col_qualification.bnf
index 63c584b9d018..8a8a5e3226f5 100644
--- a/docs/generated/sql/bnf/col_qualification.bnf
+++ b/docs/generated/sql/bnf/col_qualification.bnf
@@ -9,6 +9,8 @@ col_qualification ::=
 	| 'CONSTRAINT' constraint_name 'REFERENCES' table_name opt_name_parens key_match reference_actions
 	| 'CONSTRAINT' constraint_name 'AS' '(' a_expr ')' 'STORED'
 	| 'CONSTRAINT' constraint_name 'GENERATED_ALWAYS' 'ALWAYS' 'AS' '(' a_expr ')' 'STORED'
+	| 'CONSTRAINT' constraint_name 'AS' '(' a_expr ')' 'VIRTUAL'
+	| 'CONSTRAINT' constraint_name 'GENERATED_ALWAYS' 'ALWAYS' 'AS' '(' a_expr ')' 'VIRTUAL'
 	| 'NOT' 'NULL'
 	| 'NULL'
 	| 'UNIQUE' opt_without_index
@@ -19,6 +21,8 @@ col_qualification ::=
 	| 'REFERENCES' table_name opt_name_parens key_match reference_actions
 	| 'AS' '(' a_expr ')' 'STORED'
 	| 'GENERATED_ALWAYS' 'ALWAYS' 'AS' '(' a_expr ')' 'STORED'
+	| 'AS' '(' a_expr ')' 'VIRTUAL'
+	| 'GENERATED_ALWAYS' 'ALWAYS' 'AS' '(' a_expr ')' 'VIRTUAL'
 	| 'COLLATE' collation_name
 	| 'FAMILY' family_name
 	| 'CREATE' 'FAMILY' family_name
diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf
index fe927813c106..5690cdd30ad0 100644
--- a/docs/generated/sql/bnf/stmt_block.bnf
+++ b/docs/generated/sql/bnf/stmt_block.bnf
@@ -2872,6 +2872,7 @@ col_qualification_elem ::=
 	| 'DEFAULT' b_expr
 	| 'REFERENCES' table_name opt_name_parens key_match reference_actions
 	| generated_as '(' a_expr ')' 'STORED'
+	| generated_as '(' a_expr ')' 'VIRTUAL'
 
 family_name ::=
 	name
diff --git a/pkg/sql/add_column.go b/pkg/sql/add_column.go
index eee5bd6ad6d0..4f72e3399e7f 100644
--- a/pkg/sql/add_column.go
+++ b/pkg/sql/add_column.go
@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
+	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/errors"
 )
 
@@ -142,6 +143,9 @@ func (p *planner) addColumnImpl(
 	}
 
 	if d.IsComputed() {
+		if d.IsVirtual() {
+			return unimplemented.NewWithIssue(57608, "virtual computed columns")
+		}
 		computedColValidator := schemaexpr.MakeComputedColumnValidator(
 			params.ctx,
 			n.tableDesc,
diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go
index 2719448d053c..cc3ea108a13c 100644
--- a/pkg/sql/create_table.go
+++ b/pkg/sql/create_table.go
@@ -1297,6 +1297,10 @@ func NewTableDesc(
 				n.Defs = append(n.Defs, checkConstraint)
 				columnDefaultExprs = append(columnDefaultExprs, nil)
 			}
+			if d.IsVirtual() {
+				return nil, unimplemented.NewWithIssue(57608, "virtual computed columns")
+			}
+
 			col, idx, expr, err := tabledesc.MakeColumnDefDescs(ctx, d, semaCtx, evalCtx)
 			if err != nil {
 				return nil, err
diff --git a/pkg/sql/logictest/testdata/logic_test/computed b/pkg/sql/logictest/testdata/logic_test/computed
index d3e445f6e27a..2694e7f976d7 100644
--- a/pkg/sql/logictest/testdata/logic_test/computed
+++ b/pkg/sql/logictest/testdata/logic_test/computed
@@ -271,11 +271,20 @@ CREATE TABLE y (
   a INT AS (3)
 )
 
-statement error at or near "virtual": syntax error: unimplemented
+statement error unimplemented: virtual computed columns
 CREATE TABLE y (
   a INT AS (3) VIRTUAL
 )
 
+statement ok
+CREATE TABLE tmp (x INT)
+
+statement error unimplemented: virtual computed columns
+ALTER TABLE tmp ADD COLUMN y INT AS (x+1) VIRTUAL
+
+statement ok
+DROP TABLE tmp
+
 statement error expected computed column expression to have type int, but .* has type string
 CREATE TABLE y (
   a INT AS ('not an integer!'::STRING) STORED
diff --git a/pkg/sql/parser/parse_test.go b/pkg/sql/parser/parse_test.go
index 2f1f16c40e68..f3797c30505f 100644
--- a/pkg/sql/parser/parse_test.go
+++ b/pkg/sql/parser/parse_test.go
@@ -280,6 +280,7 @@ func TestParse(t *testing.T) {
 		{`CREATE TABLE a.b (b INT8)`},
 		{`CREATE TABLE IF NOT EXISTS a (b INT8)`},
 		{`CREATE TABLE a (b INT8 AS (a + b) STORED)`},
+		{`CREATE TABLE a (b INT8 AS (a + b) VIRTUAL)`},
 		{`CREATE TABLE view (view INT8)`},
 
 		{`CREATE TABLE a (b INT8 CONSTRAINT c PRIMARY KEY)`},
@@ -2720,6 +2721,7 @@ SKIP_MISSING_FOREIGN_KEYS, SKIP_MISSING_SEQUENCES, SKIP_MISSING_SEQUENCE_OWNERS,
 			`CREATE TABLE a (b INT8, c STRING, FOREIGN KEY (b, c) REFERENCES other (x, y) ON DELETE CASCADE ON UPDATE SET NULL)`,
 		},
 		{`CREATE TABLE a (b INT8 GENERATED ALWAYS AS (a + b) STORED)`, `CREATE TABLE a (b INT8 AS (a + b) STORED)`},
+		{`CREATE TABLE a (b INT8 GENERATED ALWAYS AS (a + b) VIRTUAL)`, `CREATE TABLE a (b INT8 AS (a + b) VIRTUAL)`},
 
 		{`ALTER TABLE a ALTER b DROP STORED`, `ALTER TABLE a ALTER COLUMN b DROP STORED`},
 		{`ALTER TABLE a ADD b INT8`, `ALTER TABLE a ADD COLUMN b INT8`},
@@ -3186,7 +3188,6 @@ func TestUnimplementedSyntax(t *testing.T) {
 
 		{`CREATE TABLE a AS SELECT b WITH NO DATA`, 0, `create table as with no data`, ``},
 
-		{`CREATE TABLE a(b INT8 AS (123) VIRTUAL)`, 0, `virtual computed columns`, ``},
 		{`CREATE TABLE a(b INT8 REFERENCES c(x) MATCH PARTIAL`, 20305, `match partial`, ``},
 		{`CREATE TABLE a(b INT8, FOREIGN KEY (b) REFERENCES c(x) MATCH PARTIAL)`, 20305, `match partial`, ``},
 
diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y
index 32d97929bfc6..65f1b1d7d521 100644
--- a/pkg/sql/parser/sql.y
+++ b/pkg/sql/parser/sql.y
@@ -6104,11 +6104,11 @@ col_qualification_elem:
  }
 | generated_as '(' a_expr ')' STORED
  {
-    $$.val = &tree.ColumnComputedDef{Expr: $3.expr()}
+    $$.val = &tree.ColumnComputedDef{Expr: $3.expr(), Virtual: false}
  }
 | generated_as '(' a_expr ')' VIRTUAL
  {
-    return unimplemented(sqllex, "virtual computed columns")
+    $$.val = &tree.ColumnComputedDef{Expr: $3.expr(), Virtual: true}
  }
 | generated_as error
  {
diff --git a/pkg/sql/sem/tree/create.go b/pkg/sql/sem/tree/create.go
index 7b3ce72a6ab8..f98bcc900f9e 100644
--- a/pkg/sql/sem/tree/create.go
+++ b/pkg/sql/sem/tree/create.go
@@ -405,6 +405,7 @@ type ColumnTableDef struct {
 	Computed struct {
 		Computed bool
 		Expr     Expr
+		Virtual  bool
 	}
 	Family struct {
 		Name        Name
@@ -532,6 +533,7 @@ func NewColumnTableDef(
 		case *ColumnComputedDef:
 			d.Computed.Computed = true
 			d.Computed.Expr = t.Expr
+			d.Computed.Virtual = t.Virtual
 		case *ColumnFamilyConstraint:
 			if d.HasColumnFamily() {
 				return nil, pgerror.Newf(pgcode.InvalidTableDefinition,
@@ -562,6 +564,11 @@ func (node *ColumnTableDef) IsComputed() bool {
 	return node.Computed.Computed
 }
 
+// IsVirtual returns if the ColumnTableDef is a virtual column.
+func (node *ColumnTableDef) IsVirtual() bool {
+	return node.Computed.Virtual
+}
+
 // HasColumnFamily returns if the ColumnTableDef has a column family.
 func (node *ColumnTableDef) HasColumnFamily() bool {
 	return node.Family.Name != "" || node.Family.Create
@@ -644,7 +651,11 @@ func (node *ColumnTableDef) Format(ctx *FmtCtx) {
 	if node.IsComputed() {
 		ctx.WriteString(" AS (")
 		ctx.FormatNode(node.Computed.Expr)
-		ctx.WriteString(") STORED")
+		if node.Computed.Virtual {
+			ctx.WriteString(") VIRTUAL")
+		} else {
+			ctx.WriteString(") STORED")
+		}
 	}
 	if node.HasColumnFamily() {
 		if node.Family.Create {
@@ -748,7 +759,8 @@ type ColumnFKConstraint struct {
 
 // ColumnComputedDef represents the description of a computed column.
 type ColumnComputedDef struct {
-	Expr Expr
+	Expr    Expr
+	Virtual bool
 }
 
 // ColumnFamilyConstraint represents FAMILY on a column.
diff --git a/pkg/sql/sem/tree/pretty.go b/pkg/sql/sem/tree/pretty.go
index 4c766e6f50c0..9e5d7c1862f7 100644
--- a/pkg/sql/sem/tree/pretty.go
+++ b/pkg/sql/sem/tree/pretty.go
@@ -1819,8 +1819,19 @@ func (node *ColumnTableDef) docRow(p *PrettyCfg) pretty.TableRow {
 
 	// Compute expression (for computed columns).
 	if node.IsComputed() {
-		clauses = append(clauses, pretty.ConcatSpace(pretty.Keyword("AS"),
-			p.bracket("(", p.Doc(node.Computed.Expr), ") STORED"),
+		var typ string
+		if node.Computed.Virtual {
+			typ = "VIRTUAL"
+		} else {
+			typ = "STORED"
+		}
+
+		clauses = append(clauses, pretty.ConcatSpace(
+			pretty.Keyword("AS"),
+			pretty.ConcatSpace(
+				p.bracket("(", p.Doc(node.Computed.Expr), ")"),
+				pretty.Keyword(typ),
+			),
 		))
 	}
 
diff --git a/pkg/sql/sem/tree/testdata/pretty/create_table.align-deindent.golden.short b/pkg/sql/sem/tree/testdata/pretty/create_table.align-deindent.golden.short
index 83ff6a889728..c8ee1977e3c3 100644
--- a/pkg/sql/sem/tree/testdata/pretty/create_table.align-deindent.golden.short
+++ b/pkg/sql/sem/tree/testdata/pretty/create_table.align-deindent.golden.short
@@ -42,6 +42,11 @@ CREATE TABLE product_information (
 								last_name
 							)
 	                    ) STORED,
+	virt_col            INT8
+	                    AS (
+							product_id
+							* 10
+	                    ) VIRTUAL,
 	INDEX date_added_idx (date_added),
 	INDEX supp_id_prod_status_idx (
 		supplier_id,
diff --git a/pkg/sql/sem/tree/testdata/pretty/create_table.align-only.golden.short b/pkg/sql/sem/tree/testdata/pretty/create_table.align-only.golden.short
index 83ff6a889728..c8ee1977e3c3 100644
--- a/pkg/sql/sem/tree/testdata/pretty/create_table.align-only.golden.short
+++ b/pkg/sql/sem/tree/testdata/pretty/create_table.align-only.golden.short
@@ -42,6 +42,11 @@ CREATE TABLE product_information (
 								last_name
 							)
 	                    ) STORED,
+	virt_col            INT8
+	                    AS (
+							product_id
+							* 10
+	                    ) VIRTUAL,
 	INDEX date_added_idx (date_added),
 	INDEX supp_id_prod_status_idx (
 		supplier_id,
diff --git a/pkg/sql/sem/tree/testdata/pretty/create_table.ref.golden.short b/pkg/sql/sem/tree/testdata/pretty/create_table.ref.golden.short
index 640f9518c869..f240f0d906ee 100644
--- a/pkg/sql/sem/tree/testdata/pretty/create_table.ref.golden.short
+++ b/pkg/sql/sem/tree/testdata/pretty/create_table.ref.golden.short
@@ -46,6 +46,9 @@ CREATE TABLE product_information (
 				last_name
 			)
 		) STORED,
+	virt_col
+		INT8
+		AS (product_id * 10) VIRTUAL,
 	INDEX date_added_idx (date_added),
 	INDEX supp_id_prod_status_idx (
 		supplier_id,
diff --git a/pkg/sql/sem/tree/testdata/pretty/create_table.sql b/pkg/sql/sem/tree/testdata/pretty/create_table.sql
index b36957c0b226..29278ce60c8b 100644
--- a/pkg/sql/sem/tree/testdata/pretty/create_table.sql
+++ b/pkg/sql/sem/tree/testdata/pretty/create_table.sql
@@ -13,6 +13,7 @@ CREATE TABLE product_information (
     date_added           DATE DEFAULT current_date(),
     misc                 JSONB,
     full_name STRING AS (concat(first_name, ' ', last_name)) STORED,
+    virt_col INT AS (product_id * 10) VIRTUAL,
     INDEX date_added_idx (date_added),
     INDEX supp_id_prod_status_idx (supplier_id, product_status),
     customer_id INT REFERENCES customers_2(id) MATCH FULL ON UPDATE CASCADE ON DELETE CASCADE,

From e225817592b0ee455f24c092a91a6356cfee984f Mon Sep 17 00:00:00 2001
From: Radu Berinde <radu@cockroachlabs.com>
Date: Sat, 5 Dec 2020 13:54:48 -0500
Subject: [PATCH 3/9] opt: clean up hack in test catalog

To ensure that PK columns are non-nullable, the test catalog
reinitializes them to override the nullable flag. We replace this
hacky mechanism with figuring out the PK columns beforehand and
marking them as non-nullable in the definition before creating the
table columns.

Release note: None
---
 pkg/sql/opt/testutils/testcat/create_table.go | 56 +++++++------------
 1 file changed, 19 insertions(+), 37 deletions(-)

diff --git a/pkg/sql/opt/testutils/testcat/create_table.go b/pkg/sql/opt/testutils/testcat/create_table.go
index 50fad1b9c725..e150aa422ae1 100644
--- a/pkg/sql/opt/testutils/testcat/create_table.go
+++ b/pkg/sql/opt/testutils/testcat/create_table.go
@@ -76,32 +76,39 @@ func (tc *Catalog) CreateTable(stmt *tree.CreateTable) *Table {
 		tab.interleaved = true
 	}
 
-	// Add non-mutation columns.
+	// Find the PK columns; we have to force these to be non-nullable.
+	pkCols := make(map[tree.Name]struct{})
 	for _, def := range stmt.Defs {
 		switch def := def.(type) {
 		case *tree.ColumnTableDef:
-			if !isMutationColumn(def) {
-				tab.addColumn(def)
+			if def.PrimaryKey.IsPrimaryKey {
+				pkCols[def.Name] = struct{}{}
+			}
+
+		case *tree.UniqueConstraintTableDef:
+			if def.PrimaryKey {
+				for i := range def.Columns {
+					pkCols[def.Columns[i].Column] = struct{}{}
+				}
 			}
 		}
 	}
 
-	// If there is no primary index, add the hidden rowid column.
-	hasPrimaryIndex := false
+	// Add non-mutation columns.
 	for _, def := range stmt.Defs {
 		switch def := def.(type) {
 		case *tree.ColumnTableDef:
-			if def.PrimaryKey.IsPrimaryKey {
-				hasPrimaryIndex = true
-			}
-
-		case *tree.UniqueConstraintTableDef:
-			if def.PrimaryKey {
-				hasPrimaryIndex = true
+			if !isMutationColumn(def) {
+				if _, isPKCol := pkCols[def.Name]; isPKCol {
+					def.Nullable.Nullability = tree.NotNull
+				}
+				tab.addColumn(def)
 			}
 		}
 	}
 
+	// If there is no primary index, add the hidden rowid column.
+	hasPrimaryIndex := len(pkCols) > 0
 	if !hasPrimaryIndex {
 		var rowid cat.Column
 		ordinal := len(tab.Columns)
@@ -562,31 +569,6 @@ func (tt *Table) addIndexWithVersion(
 		}
 		col := idx.addColumn(tt, colDef, keyCol, isLastIndexCol)
 
-		if typ == primaryIndex && col.IsNullable() {
-			// Reinitialize the column to make it non-nullable.
-			// TODO(radu): this is very hacky
-			var defaultExpr, computedExpr *string
-			if col.HasDefault() {
-				e := col.DefaultExprStr()
-				defaultExpr = &e
-			}
-			if col.IsComputed() {
-				e := col.ComputedExprStr()
-				computedExpr = &e
-			}
-			col.InitNonVirtual(
-				col.Ordinal(),
-				col.ColID(),
-				col.ColName(),
-				col.Kind(),
-				col.DatumType(),
-				false, /* nullable */
-				col.IsHidden(),
-				defaultExpr,
-				computedExpr,
-			)
-		}
-
 		if col.IsNullable() {
 			notNullIndex = false
 		}

From 2045ba2e86c53818b1c6cd32584583c737a93e55 Mon Sep 17 00:00:00 2001
From: Radu Berinde <radu@cockroachlabs.com>
Date: Sat, 5 Dec 2020 23:05:00 -0500
Subject: [PATCH 4/9] opt: test catalog support for virtual columns

Release note: None
---
 pkg/sql/opt/cat/column.go                     | 15 +++++---
 pkg/sql/opt/cat/utils.go                      |  8 +++--
 pkg/sql/opt/testutils/testcat/create_table.go | 36 +++++++++++++------
 pkg/sql/opt/testutils/testcat/testdata/index  | 16 ++++-----
 pkg/sql/opt/testutils/testcat/testdata/table  |  4 ++-
 5 files changed, 52 insertions(+), 27 deletions(-)

diff --git a/pkg/sql/opt/cat/column.go b/pkg/sql/opt/cat/column.go
index c9e77c40576c..a28d68b04765 100644
--- a/pkg/sql/opt/cat/column.go
+++ b/pkg/sql/opt/cat/column.go
@@ -161,8 +161,7 @@ const (
 	// VirtualInverted columns are implicit columns that are used by inverted
 	// indexes.
 	VirtualInverted
-	// VirtualComputed columns are non-stored computed columns that are used by
-	// expression-based indexes.
+	// VirtualComputed columns are non-stored computed columns.
 	VirtualComputed
 )
 
@@ -228,15 +227,21 @@ func (c *Column) InitVirtualInverted(
 // InitVirtualComputed is used by catalog implementations to populate a
 // VirtualComputed Column. It should not be used anywhere else.
 func (c *Column) InitVirtualComputed(
-	ordinal int, name tree.Name, datumType *types.T, nullable bool, computedExpr string,
+	ordinal int,
+	stableID StableID,
+	name tree.Name,
+	datumType *types.T,
+	nullable bool,
+	hidden bool,
+	computedExpr string,
 ) {
 	c.ordinal = ordinal
-	c.stableID = 0
+	c.stableID = stableID
 	c.name = name
 	c.kind = VirtualComputed
 	c.datumType = datumType
 	c.nullable = nullable
-	c.hidden = true
+	c.hidden = hidden
 	c.defaultExpr = ""
 	c.computedExpr = computedExpr
 	c.invertedSourceColumnOrdinal = -1
diff --git a/pkg/sql/opt/cat/utils.go b/pkg/sql/opt/cat/utils.go
index b63d4e93cf82..62bf04684c15 100644
--- a/pkg/sql/opt/cat/utils.go
+++ b/pkg/sql/opt/cat/utils.go
@@ -306,7 +306,11 @@ func formatColumn(col *Column, buf *bytes.Buffer) {
 		fmt.Fprintf(buf, " not null")
 	}
 	if col.IsComputed() {
-		fmt.Fprintf(buf, " as (%s) stored", col.ComputedExprStr())
+		if col.Kind() == VirtualComputed {
+			fmt.Fprintf(buf, " as (%s) virtual", col.ComputedExprStr())
+		} else {
+			fmt.Fprintf(buf, " as (%s) stored", col.ComputedExprStr())
+		}
 	}
 	if col.HasDefault() {
 		fmt.Fprintf(buf, " default (%s)", col.DefaultExprStr())
@@ -322,7 +326,7 @@ func formatColumn(col *Column, buf *bytes.Buffer) {
 	case VirtualInverted:
 		fmt.Fprintf(buf, " [virtual-inverted]")
 	case VirtualComputed:
-		fmt.Fprintf(buf, " [virtual-computed]")
+		// No need to show anything more (it already shows up as virtual).
 	}
 }
 
diff --git a/pkg/sql/opt/testutils/testcat/create_table.go b/pkg/sql/opt/testutils/testcat/create_table.go
index e150aa422ae1..7844e00531bb 100644
--- a/pkg/sql/opt/testutils/testcat/create_table.go
+++ b/pkg/sql/opt/testutils/testcat/create_table.go
@@ -514,17 +514,29 @@ func (tt *Table) addColumn(def *tree.ColumnTableDef) {
 	}
 
 	var col cat.Column
-	col.InitNonVirtual(
-		ordinal,
-		cat.StableID(1+ordinal),
-		name,
-		kind,
-		typ,
-		nullable,
-		false, /* hidden */
-		defaultExpr,
-		computedExpr,
-	)
+	if def.Computed.Virtual {
+		col.InitVirtualComputed(
+			ordinal,
+			cat.StableID(1+ordinal),
+			name,
+			typ,
+			nullable,
+			false, /* hidden */
+			*computedExpr,
+		)
+	} else {
+		col.InitNonVirtual(
+			ordinal,
+			cat.StableID(1+ordinal),
+			name,
+			kind,
+			typ,
+			nullable,
+			false, /* hidden */
+			defaultExpr,
+			computedExpr,
+		)
+	}
 	tt.Columns = append(tt.Columns, col)
 }
 
@@ -820,9 +832,11 @@ func columnForIndexElemExpr(tt *Table, expr tree.Expr) cat.Column {
 	var col cat.Column
 	col.InitVirtualComputed(
 		len(tt.Columns),
+		cat.StableID(1+len(tt.Columns)),
 		name,
 		typ,
 		true, /* nullable */
+		true, /* hidden */
 		exprStr,
 	)
 	tt.Columns = append(tt.Columns, col)
diff --git a/pkg/sql/opt/testutils/testcat/testdata/index b/pkg/sql/opt/testutils/testcat/testdata/index
index adf0cc3dcc54..389eabc67ba5 100644
--- a/pkg/sql/opt/testutils/testcat/testdata/index
+++ b/pkg/sql/opt/testutils/testcat/testdata/index
@@ -122,24 +122,24 @@ TABLE xyz
  ├── y int
  ├── z string
  ├── crdb_internal_mvcc_timestamp decimal [hidden] [system]
- ├── idx_expr_1 string as (lower(z)) stored [hidden] [virtual-computed]
- ├── idx_expr_2 int as (y + 1) stored [hidden] [virtual-computed]
- ├── idx_expr_3 int as (x + y) stored [hidden] [virtual-computed]
+ ├── idx_expr_1 string as (lower(z)) virtual [hidden]
+ ├── idx_expr_2 int as (y + 1) virtual [hidden]
+ ├── idx_expr_3 int as (x + y) virtual [hidden]
  ├── INDEX primary
  │    └── x int not null
  ├── INDEX idx1
- │    ├── idx_expr_1 string as (lower(z)) stored [hidden] [virtual-computed]
+ │    ├── idx_expr_1 string as (lower(z)) virtual [hidden]
  │    └── x int not null
  ├── INDEX idx2
- │    ├── idx_expr_1 string as (lower(z)) stored [hidden] [virtual-computed]
+ │    ├── idx_expr_1 string as (lower(z)) virtual [hidden]
  │    ├── y int
  │    └── x int not null
  ├── INDEX idx3
- │    ├── idx_expr_2 int as (y + 1) stored [hidden] [virtual-computed]
- │    ├── idx_expr_1 string as (lower(z)) stored [hidden] [virtual-computed]
+ │    ├── idx_expr_2 int as (y + 1) virtual [hidden]
+ │    ├── idx_expr_1 string as (lower(z)) virtual [hidden]
  │    └── x int not null
  ├── INDEX idx4
- │    ├── idx_expr_3 int as (x + y) stored [hidden] [virtual-computed]
+ │    ├── idx_expr_3 int as (x + y) virtual [hidden]
  │    ├── y int
  │    ├── x int not null
  │    ├── z string (storing)
diff --git a/pkg/sql/opt/testutils/testcat/testdata/table b/pkg/sql/opt/testutils/testcat/testdata/table
index 18e3f66eaa61..45fcbd7c5a03 100644
--- a/pkg/sql/opt/testutils/testcat/testdata/table
+++ b/pkg/sql/opt/testutils/testcat/testdata/table
@@ -23,7 +23,8 @@ CREATE TABLE abcdef (
     c INT DEFAULT (10),
     d INT AS (b + c + 1) STORED,
     e INT AS (a) STORED,
-    f INT CHECK (f > 2)
+    f INT CHECK (f > 2),
+    g INT AS (a+b) VIRTUAL
 )
 ----
 
@@ -37,6 +38,7 @@ TABLE abcdef
  ├── d int as ((b + c) + 1) stored
  ├── e int as (a) stored
  ├── f int
+ ├── g int as (a + b) virtual
  ├── rowid int not null default (unique_rowid()) [hidden]
  ├── crdb_internal_mvcc_timestamp decimal [hidden] [system]
  ├── CHECK (f > 2)

From 70179901299218aed204d6d0ebcd61d8fc650474 Mon Sep 17 00:00:00 2001
From: Radu Berinde <radu@cockroachlabs.com>
Date: Mon, 7 Dec 2020 16:56:50 -0500
Subject: [PATCH 5/9] sql: migrate other EXPLAIN variants to new exec factory
 interface

This change migrates the other EXPLAIN variants to the new interface,
which uses an explain factory to build the explained plan.

This fixes a TODO in the experimental factory.

Release note: None
---
 pkg/sql/distsql_spec_exec_factory.go       | 37 ++++++++++++----------
 pkg/sql/opt/exec/execbuilder/statement.go  | 36 +++++++--------------
 pkg/sql/opt/exec/explain/emit.go           |  2 --
 pkg/sql/opt/exec/explain/result_columns.go |  3 --
 pkg/sql/opt/exec/factory.opt               | 12 ++-----
 pkg/sql/opt_exec_factory.go                | 22 ++++++-------
 6 files changed, 46 insertions(+), 66 deletions(-)

diff --git a/pkg/sql/distsql_spec_exec_factory.go b/pkg/sql/distsql_spec_exec_factory.go
index a8eb4bd3bbe7..90f84fd99a0a 100644
--- a/pkg/sql/distsql_spec_exec_factory.go
+++ b/pkg/sql/distsql_spec_exec_factory.go
@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/cat"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/constraint"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec"
+	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec/explain"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/invertedexpr"
 	"github.com/cockroachdb/cockroach/pkg/sql/physicalplan"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/builtins"
@@ -26,6 +27,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/span"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
+	"github.com/cockroachdb/errors"
 )
 
 type distSQLSpecExecFactory struct {
@@ -741,23 +743,32 @@ func (e *distSQLSpecExecFactory) ConstructExplainOpt(
 }
 
 func (e *distSQLSpecExecFactory) ConstructExplain(
-	options *tree.ExplainOptions, analyze bool, stmtType tree.StatementType, plan exec.Plan,
+	options *tree.ExplainOptions,
+	analyze bool,
+	stmtType tree.StatementType,
+	buildFn exec.BuildPlanForExplainFn,
 ) (exec.Node, error) {
+	if options.Flags[tree.ExplainFlagEnv] {
+		return nil, errors.New("ENV only supported with (OPT) option")
+	}
+	if options.Mode == tree.ExplainPlan {
+		// TODO(radu): all we need to do here is wrap an explainPlanNode.
+		return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: explain plan")
+	}
+
 	// We cannot create the explained plan in the same PlanInfrastructure with the
-	// "outer" plan. Create a new PlanningCtx for the rest of the plan.
-	// TODO(radu): this is a hack and won't work if the result of the explain
-	// feeds into a join or union (on the right-hand side). Move to a model like
-	// ConstructExplainPlan, where we can build the inner plan using a separate
-	// factory instance.
-	planCtxCopy := *e.planCtx
-	planCtxCopy.infra = physicalplan.MakePhysicalInfrastructure(e.planCtx.infra.FlowID, e.planCtx.infra.GatewayNodeID)
-	e.planCtx = &planCtxCopy
+	// "outer" plan. Create a separate factory.
+	explainFactory := explain.NewFactory(newDistSQLSpecExecFactory(e.planner, e.planningMode))
+	plan, err := buildFn(explainFactory)
+	if err != nil {
+		return nil, err
+	}
 
 	// TODO(yuzefovich): make sure to return the same nice error in some
 	// variants of EXPLAIN when subqueries are present as we do in the old path.
 	// TODO(yuzefovich): make sure that local plan nodes that create
 	// distributed jobs are shown as "distributed". See distSQLExplainable.
-	p := plan.(*planComponents)
+	p := plan.(*explain.Plan).WrappedPlan.(*planComponents)
 	explain, err := constructExplainDistSQLOrVecNode(options, analyze, stmtType, p, e.planner)
 	if err != nil {
 		return nil, err
@@ -776,12 +787,6 @@ func (e *distSQLSpecExecFactory) ConstructExplain(
 	return makePlanMaybePhysical(physPlan, []planNode{explainNode}), nil
 }
 
-func (e *distSQLSpecExecFactory) ConstructExplainPlan(
-	options *tree.ExplainOptions, buildFn exec.BuildPlanForExplainFn,
-) (exec.Node, error) {
-	return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: explain plan")
-}
-
 func (e *distSQLSpecExecFactory) ConstructShowTrace(
 	typ tree.ShowTraceType, compact bool,
 ) (exec.Node, error) {
diff --git a/pkg/sql/opt/exec/execbuilder/statement.go b/pkg/sql/opt/exec/execbuilder/statement.go
index 030cdb7c125f..d867b719868d 100644
--- a/pkg/sql/opt/exec/execbuilder/statement.go
+++ b/pkg/sql/opt/exec/execbuilder/statement.go
@@ -117,34 +117,20 @@ func (b *Builder) buildExplain(explain *memo.ExplainExpr) (execPlan, error) {
 		return b.buildExplainOpt(explain)
 	}
 
-	if explain.Options.Mode == tree.ExplainPlan {
-		node, err := b.factory.ConstructExplainPlan(
-			&explain.Options,
-			func(ef exec.ExplainFactory) (exec.Plan, error) {
-				// Create a separate builder for the explain query.
-				explainBld := New(ef, b.mem, b.catalog, explain.Input, b.evalCtx, b.initialAllowAutoCommit)
-				explainBld.disableTelemetry = true
-				return explainBld.Build()
-			},
-		)
-		if err != nil {
-			return execPlan{}, err
-		}
-		return planWithColumns(node, explain.ColList), nil
-	}
-
-	// Create a separate builder for the explain query.
-	explainBld := New(b.factory, b.mem, b.catalog, explain.Input, b.evalCtx, b.initialAllowAutoCommit)
-	explainBld.disableTelemetry = true
-	plan, err := explainBld.Build()
-	if err != nil {
-		return execPlan{}, err
-	}
-	node, err := b.factory.ConstructExplain(&explain.Options, explain.Analyze, explain.StmtType, plan)
+	node, err := b.factory.ConstructExplain(
+		&explain.Options,
+		explain.Analyze,
+		explain.StmtType,
+		func(ef exec.ExplainFactory) (exec.Plan, error) {
+			// Create a separate builder for the explain query.
+			explainBld := New(ef, b.mem, b.catalog, explain.Input, b.evalCtx, b.initialAllowAutoCommit)
+			explainBld.disableTelemetry = true
+			return explainBld.Build()
+		},
+	)
 	if err != nil {
 		return execPlan{}, err
 	}
-
 	return planWithColumns(node, explain.ColList), nil
 }
 
diff --git a/pkg/sql/opt/exec/explain/emit.go b/pkg/sql/opt/exec/explain/emit.go
index 4f80d802fc18..d7193c47a420 100644
--- a/pkg/sql/opt/exec/explain/emit.go
+++ b/pkg/sql/opt/exec/explain/emit.go
@@ -261,7 +261,6 @@ var nodeNames = [...]string{
 	errorIfRowsOp:          "error if rows",
 	explainOp:              "explain",
 	explainOptOp:           "explain",
-	explainPlanOp:          "explain",
 	exportOp:               "export",
 	filterOp:               "filter",
 	groupByOp:              "group",
@@ -675,7 +674,6 @@ func (e *emitter) emitNodeAttributes(n *Node) error {
 		max1RowOp,
 		explainOptOp,
 		explainOp,
-		explainPlanOp,
 		showTraceOp,
 		createTableOp,
 		createTableAsOp,
diff --git a/pkg/sql/opt/exec/explain/result_columns.go b/pkg/sql/opt/exec/explain/result_columns.go
index 96649cb8e4ac..c25959af5cd7 100644
--- a/pkg/sql/opt/exec/explain/result_columns.go
+++ b/pkg/sql/opt/exec/explain/result_columns.go
@@ -174,9 +174,6 @@ func getResultColumns(
 			return nil, errors.AssertionFailedf("unknown explain mode %v", o.Mode)
 		}
 
-	case explainPlanOp:
-		return colinfo.ExplainPlanColumns, nil
-
 	case explainOptOp:
 		return colinfo.ExplainPlanColumns, nil
 
diff --git a/pkg/sql/opt/exec/factory.opt b/pkg/sql/opt/exec/factory.opt
index e4bcdb9bf3f4..73d0219b4815 100644
--- a/pkg/sql/opt/exec/factory.opt
+++ b/pkg/sql/opt/exec/factory.opt
@@ -323,19 +323,13 @@ define ExplainOpt {
 }
 
 # Explain implements EXPLAIN, showing information about the given plan.
-define Explain {
-    Options *tree.ExplainOptions
-    Analyze bool
-    StmtType tree.StatementType
-    Plan exec.Plan
-}
-
-# ExplainPlan implements EXPLAIN (PLAN).
 #
 # When the operator is created, it creates an ExplainFactory and calls BuildFn
 # to construct the plan against that factory.
-define ExplainPlan {
+define Explain {
     Options *tree.ExplainOptions
+    Analyze bool
+    StmtType tree.StatementType
     BuildFn exec.BuildPlanForExplainFn
 }
 
diff --git a/pkg/sql/opt_exec_factory.go b/pkg/sql/opt_exec_factory.go
index a044222d8cef..725fd031492e 100644
--- a/pkg/sql/opt_exec_factory.go
+++ b/pkg/sql/opt_exec_factory.go
@@ -1116,13 +1116,6 @@ func (ef *execFactory) ConstructExplainOpt(
 	}, nil
 }
 
-// ConstructExplain is part of the exec.Factory interface.
-func (ef *execFactory) ConstructExplain(
-	options *tree.ExplainOptions, analyze bool, stmtType tree.StatementType, plan exec.Plan,
-) (exec.Node, error) {
-	return constructExplainDistSQLOrVecNode(options, analyze, stmtType, plan.(*planComponents), ef.planner)
-}
-
 // ConstructShowTrace is part of the exec.Factory interface.
 func (ef *execFactory) ConstructShowTrace(typ tree.ShowTraceType, compact bool) (exec.Node, error) {
 	var node planNode = ef.planner.makeShowTraceNode(compact, typ == tree.ShowTraceKV)
@@ -1847,20 +1840,27 @@ func (ef *execFactory) ConstructCancelSessions(input exec.Node, ifExists bool) (
 	}, nil
 }
 
-func (ef *execFactory) ConstructExplainPlan(
-	options *tree.ExplainOptions, buildFn exec.BuildPlanForExplainFn,
+// ConstructExplain is part of the exec.Factory interface.
+func (ef *execFactory) ConstructExplain(
+	options *tree.ExplainOptions,
+	analyze bool,
+	stmtType tree.StatementType,
+	buildFn exec.BuildPlanForExplainFn,
 ) (exec.Node, error) {
 	if options.Flags[tree.ExplainFlagEnv] {
 		return nil, errors.New("ENV only supported with (OPT) option")
 	}
 
-	flags := explain.MakeFlags(options)
-
 	explainFactory := explain.NewFactory(ef)
 	plan, err := buildFn(explainFactory)
 	if err != nil {
 		return nil, err
 	}
+	if options.Mode != tree.ExplainPlan {
+		wrappedPlan := plan.(*explain.Plan).WrappedPlan.(*planComponents)
+		return constructExplainDistSQLOrVecNode(options, analyze, stmtType, wrappedPlan, ef.planner)
+	}
+	flags := explain.MakeFlags(options)
 	n := &explainPlanNode{
 		flags: flags,
 		plan:  plan.(*explain.Plan),

From a66cfddc54b111480054d8fae8dfce94732fe0ab Mon Sep 17 00:00:00 2001
From: Radu Berinde <radu@cockroachlabs.com>
Date: Mon, 7 Dec 2020 16:56:50 -0500
Subject: [PATCH 6/9] opt: add operator for CREATE STATISTICS

This change adds an optimizer operator for CREATE STATISTICS (instead
of using the opaque operator).

Release note: None
---
 pkg/sql/create_stats.go                       | 33 -------------------
 pkg/sql/distsql_spec_exec_factory.go          |  6 ++++
 pkg/sql/opaque.go                             |  6 ----
 pkg/sql/opt/exec/execbuilder/relational.go    |  3 ++
 pkg/sql/opt/exec/execbuilder/statement.go     |  9 +++++
 pkg/sql/opt/exec/explain/emit.go              |  2 ++
 pkg/sql/opt/exec/explain/result_columns.go    |  2 +-
 pkg/sql/opt/exec/factory.opt                  |  5 +++
 pkg/sql/opt/memo/expr_format.go               |  3 ++
 pkg/sql/opt/memo/logical_props_builder.go     |  6 ++++
 pkg/sql/opt/ops/statement.opt                 | 12 +++++++
 pkg/sql/opt/optbuilder/builder.go             |  7 ++++
 pkg/sql/opt/optbuilder/misc_statements.go     |  8 +++++
 .../opt/optbuilder/testdata/misc_statements   | 12 +++++++
 pkg/sql/opt/optgen/cmd/optgen/metadata.go     |  1 +
 pkg/sql/opt_exec_factory.go                   | 19 +++++++++++
 16 files changed, 94 insertions(+), 40 deletions(-)

diff --git a/pkg/sql/create_stats.go b/pkg/sql/create_stats.go
index 35f0f2b4fb0f..c19df2698de0 100644
--- a/pkg/sql/create_stats.go
+++ b/pkg/sql/create_stats.go
@@ -56,39 +56,6 @@ var featureStatsEnabled = settings.RegisterPublicBoolSetting(
 	"set to true to enable CREATE STATISTICS/ANALYZE, false to disable; default is true",
 	featureflag.FeatureFlagEnabledDefault)
 
-func (p *planner) CreateStatistics(ctx context.Context, n *tree.CreateStats) (planNode, error) {
-	if err := featureflag.CheckEnabled(
-		ctx,
-		featureStatsEnabled,
-		&p.ExecCfg().Settings.SV,
-		"ANALYZE/CREATE STATISTICS",
-	); err != nil {
-		return nil, err
-	}
-
-	return &createStatsNode{
-		CreateStats: *n,
-		p:           p,
-	}, nil
-}
-
-// Analyze is syntactic sugar for CreateStatistics.
-func (p *planner) Analyze(ctx context.Context, n *tree.Analyze) (planNode, error) {
-	if err := featureflag.CheckEnabled(
-		ctx,
-		featureStatsEnabled,
-		&p.ExecCfg().Settings.SV,
-		"ANALYZE/CREATE STATISTICS",
-	); err != nil {
-		return nil, err
-	}
-
-	return &createStatsNode{
-		CreateStats: tree.CreateStats{Table: n.Table},
-		p:           p,
-	}, nil
-}
-
 const defaultHistogramBuckets = 200
 const nonIndexColHistogramBuckets = 2
 
diff --git a/pkg/sql/distsql_spec_exec_factory.go b/pkg/sql/distsql_spec_exec_factory.go
index 90f84fd99a0a..4db1087b14f7 100644
--- a/pkg/sql/distsql_spec_exec_factory.go
+++ b/pkg/sql/distsql_spec_exec_factory.go
@@ -982,6 +982,12 @@ func (e *distSQLSpecExecFactory) ConstructCancelSessions(
 	return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: cancel sessions")
 }
 
+func (e *distSQLSpecExecFactory) ConstructCreateStatistics(
+	cs *tree.CreateStats,
+) (exec.Node, error) {
+	return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: create statistics")
+}
+
 func (e *distSQLSpecExecFactory) ConstructExport(
 	input exec.Node, fileName tree.TypedExpr, fileFormat string, options []exec.KVOption,
 ) (exec.Node, error) {
diff --git a/pkg/sql/opaque.go b/pkg/sql/opaque.go
index dfc8e5d0c008..fc2c810699c7 100644
--- a/pkg/sql/opaque.go
+++ b/pkg/sql/opaque.go
@@ -75,8 +75,6 @@ func buildOpaque(
 		plan, err = p.AlterRole(ctx, n)
 	case *tree.AlterSequence:
 		plan, err = p.AlterSequence(ctx, n)
-	case *tree.Analyze:
-		plan, err = p.Analyze(ctx, n)
 	case *tree.CommentOnColumn:
 		plan, err = p.CommentOnColumn(ctx, n)
 	case *tree.CommentOnDatabase:
@@ -97,8 +95,6 @@ func buildOpaque(
 		plan, err = p.CreateRole(ctx, n)
 	case *tree.CreateSequence:
 		plan, err = p.CreateSequence(ctx, n)
-	case *tree.CreateStats:
-		plan, err = p.CreateStatistics(ctx, n)
 	case *tree.CreateExtension:
 		plan, err = p.CreateExtension(ctx, n)
 	case *tree.Deallocate:
@@ -213,7 +209,6 @@ func init() {
 		&tree.AlterType{},
 		&tree.AlterSequence{},
 		&tree.AlterRole{},
-		&tree.Analyze{},
 		&tree.CommentOnColumn{},
 		&tree.CommentOnDatabase{},
 		&tree.CommentOnIndex{},
@@ -223,7 +218,6 @@ func init() {
 		&tree.CreateIndex{},
 		&tree.CreateSchema{},
 		&tree.CreateSequence{},
-		&tree.CreateStats{},
 		&tree.CreateType{},
 		&tree.CreateRole{},
 		&tree.Deallocate{},
diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go
index ae5a08258485..bbfb011c948a 100644
--- a/pkg/sql/opt/exec/execbuilder/relational.go
+++ b/pkg/sql/opt/exec/execbuilder/relational.go
@@ -304,6 +304,9 @@ func (b *Builder) buildRelational(e memo.RelExpr) (execPlan, error) {
 	case *memo.CancelSessionsExpr:
 		ep, err = b.buildCancelSessions(t)
 
+	case *memo.CreateStatisticsExpr:
+		ep, err = b.buildCreateStatistics(t)
+
 	case *memo.ExportExpr:
 		ep, err = b.buildExport(t)
 
diff --git a/pkg/sql/opt/exec/execbuilder/statement.go b/pkg/sql/opt/exec/execbuilder/statement.go
index d867b719868d..e5c4c0189ad0 100644
--- a/pkg/sql/opt/exec/execbuilder/statement.go
+++ b/pkg/sql/opt/exec/execbuilder/statement.go
@@ -272,6 +272,15 @@ func (b *Builder) buildCancelSessions(cancel *memo.CancelSessionsExpr) (execPlan
 	return execPlan{root: node}, nil
 }
 
+func (b *Builder) buildCreateStatistics(c *memo.CreateStatisticsExpr) (execPlan, error) {
+	node, err := b.factory.ConstructCreateStatistics(c.Syntax)
+	if err != nil {
+		return execPlan{}, err
+	}
+	// CreateStatistics returns no columns.
+	return execPlan{root: node}, nil
+}
+
 func (b *Builder) buildExport(export *memo.ExportExpr) (execPlan, error) {
 	input, err := b.buildRelational(export.Input)
 	if err != nil {
diff --git a/pkg/sql/opt/exec/explain/emit.go b/pkg/sql/opt/exec/explain/emit.go
index d7193c47a420..11c02eee7cae 100644
--- a/pkg/sql/opt/exec/explain/emit.go
+++ b/pkg/sql/opt/exec/explain/emit.go
@@ -252,6 +252,7 @@ var nodeNames = [...]string{
 	cancelSessionsOp:       "cancel sessions",
 	controlJobsOp:          "control jobs",
 	controlSchedulesOp:     "control schedules",
+	createStatisticsOp:     "create statistics",
 	createTableOp:          "create table",
 	createTableAsOp:        "create table as",
 	createViewOp:           "create view",
@@ -691,6 +692,7 @@ func (e *emitter) emitNodeAttributes(n *Node) error {
 		controlSchedulesOp,
 		cancelQueriesOp,
 		cancelSessionsOp,
+		createStatisticsOp,
 		exportOp:
 
 	default:
diff --git a/pkg/sql/opt/exec/explain/result_columns.go b/pkg/sql/opt/exec/explain/result_columns.go
index c25959af5cd7..6b8d0f3d92d4 100644
--- a/pkg/sql/opt/exec/explain/result_columns.go
+++ b/pkg/sql/opt/exec/explain/result_columns.go
@@ -184,7 +184,7 @@ func getResultColumns(
 		return colinfo.ShowTraceColumns, nil
 
 	case createTableOp, createTableAsOp, createViewOp, controlJobsOp, controlSchedulesOp,
-		cancelQueriesOp, cancelSessionsOp, errorIfRowsOp, deleteRangeOp:
+		cancelQueriesOp, cancelSessionsOp, createStatisticsOp, errorIfRowsOp, deleteRangeOp:
 		// These operations produce no columns.
 		return nil, nil
 
diff --git a/pkg/sql/opt/exec/factory.opt b/pkg/sql/opt/exec/factory.opt
index 73d0219b4815..7e8859f7c8c9 100644
--- a/pkg/sql/opt/exec/factory.opt
+++ b/pkg/sql/opt/exec/factory.opt
@@ -646,6 +646,11 @@ define CancelSessions {
     IfExists bool
 }
 
+# CreateStatistics implements CREATE STATISTICS.
+define CreateStatistics {
+    Cs *tree.CreateStats
+}
+
 # Export implements EXPORT.
 define Export {
     Input exec.Node
diff --git a/pkg/sql/opt/memo/expr_format.go b/pkg/sql/opt/memo/expr_format.go
index a746668575ad..7aba43cef06d 100644
--- a/pkg/sql/opt/memo/expr_format.go
+++ b/pkg/sql/opt/memo/expr_format.go
@@ -608,6 +608,9 @@ func (f *ExprFmtCtx) formatRelational(e RelExpr, tp treeprinter.Node) {
 			n.Child(f.Buffer.String())
 		}
 
+	case *CreateStatisticsExpr:
+		tp.Child(t.Syntax.String())
+
 	case *ExportExpr:
 		tp.Childf("format: %s", t.FileFormat)
 
diff --git a/pkg/sql/opt/memo/logical_props_builder.go b/pkg/sql/opt/memo/logical_props_builder.go
index ad3b89cc38ef..a0fe9ab9d574 100644
--- a/pkg/sql/opt/memo/logical_props_builder.go
+++ b/pkg/sql/opt/memo/logical_props_builder.go
@@ -995,6 +995,12 @@ func (b *logicalPropsBuilder) buildCancelSessionsProps(
 	b.buildBasicProps(cancel, opt.ColList{}, rel)
 }
 
+func (b *logicalPropsBuilder) buildCreateStatisticsProps(
+	ctl *CreateStatisticsExpr, rel *props.Relational,
+) {
+	b.buildBasicProps(ctl, opt.ColList{}, rel)
+}
+
 func (b *logicalPropsBuilder) buildExportProps(export *ExportExpr, rel *props.Relational) {
 	b.buildBasicProps(export, export.Columns, rel)
 }
diff --git a/pkg/sql/opt/ops/statement.opt b/pkg/sql/opt/ops/statement.opt
index 08d3cee48375..5abfe18f81af 100644
--- a/pkg/sql/opt/ops/statement.opt
+++ b/pkg/sql/opt/ops/statement.opt
@@ -278,3 +278,15 @@ define ExportPrivate {
     # Columns stores the column IDs for the statement result columns.
     Columns ColList
 }
+
+# CreateStatistics represents a CREATE STATISTICS or ANALYZE statement.
+[Relational]
+define CreateStatistics {
+    _ CreateStatisticsPrivate
+}
+
+[Private]
+define CreateStatisticsPrivate {
+    # Syntax is the tree.CreateStats AST node.
+    Syntax CreateStats
+}
diff --git a/pkg/sql/opt/optbuilder/builder.go b/pkg/sql/opt/optbuilder/builder.go
index 0b9765c338b6..c24d353790ba 100644
--- a/pkg/sql/opt/optbuilder/builder.go
+++ b/pkg/sql/opt/optbuilder/builder.go
@@ -318,6 +318,13 @@ func (b *Builder) buildStmt(
 	case *tree.CancelSessions:
 		return b.buildCancelSessions(stmt, inScope)
 
+	case *tree.CreateStats:
+		return b.buildCreateStatistics(stmt, inScope)
+
+	case *tree.Analyze:
+		// ANALYZE is syntactic sugar for CREATE STATISTICS.
+		return b.buildCreateStatistics(&tree.CreateStats{Table: stmt.Table}, inScope)
+
 	case *tree.Export:
 		return b.buildExport(stmt, inScope)
 
diff --git a/pkg/sql/opt/optbuilder/misc_statements.go b/pkg/sql/opt/optbuilder/misc_statements.go
index 0f04615d84d1..506544759ad1 100644
--- a/pkg/sql/opt/optbuilder/misc_statements.go
+++ b/pkg/sql/opt/optbuilder/misc_statements.go
@@ -124,3 +124,11 @@ func (b *Builder) buildControlSchedules(
 	)
 	return outScope
 }
+
+func (b *Builder) buildCreateStatistics(n *tree.CreateStats, inScope *scope) (outScope *scope) {
+	outScope = inScope.push()
+	outScope.expr = b.factory.ConstructCreateStatistics(&memo.CreateStatisticsPrivate{
+		Syntax: n,
+	})
+	return outScope
+}
diff --git a/pkg/sql/opt/optbuilder/testdata/misc_statements b/pkg/sql/opt/optbuilder/testdata/misc_statements
index 9020858270a4..d2c30340a06e 100644
--- a/pkg/sql/opt/optbuilder/testdata/misc_statements
+++ b/pkg/sql/opt/optbuilder/testdata/misc_statements
@@ -174,3 +174,15 @@ export
  └── k-v-options
       └── k-v-options-item foo
            └── $1
+
+build
+CREATE STATISTICS foo FROM ab
+----
+create-statistics
+ └── CREATE STATISTICS foo FROM ab
+
+build
+ANALYZE ab
+----
+create-statistics
+ └── CREATE STATISTICS "" FROM ab
diff --git a/pkg/sql/opt/optgen/cmd/optgen/metadata.go b/pkg/sql/opt/optgen/cmd/optgen/metadata.go
index 991cdc2441a8..e154aa2ea546 100644
--- a/pkg/sql/opt/optgen/cmd/optgen/metadata.go
+++ b/pkg/sql/opt/optgen/cmd/optgen/metadata.go
@@ -217,6 +217,7 @@ func newMetadata(compiled *lang.CompiledExpr, pkg string) *metadata {
 		"Statement":         {fullName: "tree.Statement", isInterface: true},
 		"Subquery":          {fullName: "tree.Subquery", isPointer: true, usePointerIntern: true},
 		"CreateTable":       {fullName: "tree.CreateTable", isPointer: true, usePointerIntern: true},
+		"CreateStats":       {fullName: "tree.CreateStats", isPointer: true, usePointerIntern: true},
 		"TableName":         {fullName: "tree.TableName", isPointer: true, usePointerIntern: true},
 		"Constraint":        {fullName: "constraint.Constraint", isPointer: true, usePointerIntern: true},
 		"FuncProps":         {fullName: "tree.FunctionProperties", isPointer: true, usePointerIntern: true},
diff --git a/pkg/sql/opt_exec_factory.go b/pkg/sql/opt_exec_factory.go
index 725fd031492e..f383d66a85c1 100644
--- a/pkg/sql/opt_exec_factory.go
+++ b/pkg/sql/opt_exec_factory.go
@@ -19,6 +19,7 @@ import (
 	"net/url"
 	"strings"
 
+	"github.com/cockroachdb/cockroach/pkg/featureflag"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
@@ -1840,6 +1841,24 @@ func (ef *execFactory) ConstructCancelSessions(input exec.Node, ifExists bool) (
 	}, nil
 }
 
+// ConstructCreateStatistics is part of the exec.Factory interface.
+func (ef *execFactory) ConstructCreateStatistics(cs *tree.CreateStats) (exec.Node, error) {
+	ctx := ef.planner.extendedEvalCtx.Context
+	if err := featureflag.CheckEnabled(
+		ctx,
+		featureStatsEnabled,
+		&ef.planner.ExecCfg().Settings.SV,
+		"ANALYZE/CREATE STATISTICS",
+	); err != nil {
+		return nil, err
+	}
+
+	return &createStatsNode{
+		CreateStats: *cs,
+		p:           ef.planner,
+	}, nil
+}
+
 // ConstructExplain is part of the exec.Factory interface.
 func (ef *execFactory) ConstructExplain(
 	options *tree.ExplainOptions,

From 49e100433c73002669389c29aafed0a10e7b17e4 Mon Sep 17 00:00:00 2001
From: Radu Berinde <radu@cockroachlabs.com>
Date: Mon, 7 Dec 2020 16:56:50 -0500
Subject: [PATCH 7/9] sql: reimplement EXPLAIN handling of CREATE STATISTICS

CREATE STATISTICS runs the statistics plan inside a job. The plan node
tree ends in a dead end at the createStatsNode. For explain purposes,
we want to see information about the actual plan that runs inside the
job.

The current solution is to pretend that we build the plan directly in
certain contexts. This solution is hard to extend for the new
top-level instrumentation for EXPLAIN ANALYZE.

This commit changes this solution to add a `runAsJob` flag and
actually plan the distributed plan directly when we don't run as a
job. We automatically set `runAsJob=false` whenever we are building
inside Explain.

Release note: None
---
 pkg/sql/create_stats.go              | 21 ++++--------
 pkg/sql/distsql_physical_planner.go  | 21 ++++++++++++
 pkg/sql/distsql_spec_exec_factory.go |  2 --
 pkg/sql/explain_distsql.go           | 51 ++--------------------------
 pkg/sql/explain_plan.go              |  2 +-
 pkg/sql/explain_vec.go               |  2 +-
 pkg/sql/opt_exec_factory.go          | 12 ++++++-
 7 files changed, 43 insertions(+), 68 deletions(-)

diff --git a/pkg/sql/create_stats.go b/pkg/sql/create_stats.go
index c19df2698de0..55be4446a308 100644
--- a/pkg/sql/create_stats.go
+++ b/pkg/sql/create_stats.go
@@ -67,6 +67,13 @@ type createStatsNode struct {
 	tree.CreateStats
 	p *planner
 
+	// runAsJob is true by default, and causes the code below to be executed,
+	// which sets up a job and waits for it.
+	//
+	// If it is false, the flow for create statistics is planned directly; this
+	// is used when the statement is under EXPLAIN or EXPLAIN ANALYZE.
+	runAsJob bool
+
 	run createStatsRun
 }
 
@@ -447,20 +454,6 @@ func makeColStatKey(cols []descpb.ColumnID) string {
 	return colSet.String()
 }
 
-// newPlanForExplainDistSQL is part of the distSQLExplainable interface.
-func (n *createStatsNode) newPlanForExplainDistSQL(
-	planCtx *PlanningCtx, distSQLPlanner *DistSQLPlanner,
-) (*PhysicalPlan, error) {
-	// Create a job record but don't actually start the job.
-	record, err := n.makeJobRecord(planCtx.ctx)
-	if err != nil {
-		return nil, err
-	}
-	job := n.p.ExecCfg().JobRegistry.NewJob(*record)
-
-	return distSQLPlanner.createPlanForCreateStats(planCtx, job)
-}
-
 // createStatsResumer implements the jobs.Resumer interface for CreateStats
 // jobs. A new instance is created for each job.
 type createStatsResumer struct {
diff --git a/pkg/sql/distsql_physical_planner.go b/pkg/sql/distsql_physical_planner.go
index 0b0878e38557..39e888dc731e 100644
--- a/pkg/sql/distsql_physical_planner.go
+++ b/pkg/sql/distsql_physical_planner.go
@@ -17,6 +17,7 @@ import (
 	"sort"
 
 	"github.com/cockroachdb/cockroach/pkg/gossip"
+	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
@@ -525,6 +526,12 @@ func checkSupportForPlanNode(node planNode) (distRecommendation, error) {
 		}
 		return shouldDistribute, nil
 
+	case *createStatsNode:
+		if n.runAsJob {
+			return cannotDistribute, planNodeNotSupportedErr
+		}
+		return shouldDistribute, nil
+
 	default:
 		return cannotDistribute, planNodeNotSupportedErr
 	}
@@ -2697,6 +2704,20 @@ func (dsp *DistSQLPlanner) createPhysPlanForPlanNode(
 	case *zigzagJoinNode:
 		plan, err = dsp.createPlanForZigzagJoin(planCtx, n)
 
+	case *createStatsNode:
+		if n.runAsJob {
+			plan, err = dsp.wrapPlan(planCtx, n)
+		} else {
+			// Create a job record but don't actually start the job.
+			var record *jobs.Record
+			record, err = n.makeJobRecord(planCtx.ctx)
+			if err != nil {
+				return nil, err
+			}
+			job := n.p.ExecCfg().JobRegistry.NewJob(*record)
+			plan, err = dsp.createPlanForCreateStats(planCtx, job)
+		}
+
 	default:
 		// Can't handle a node? We wrap it and continue on our way.
 		plan, err = dsp.wrapPlan(planCtx, n)
diff --git a/pkg/sql/distsql_spec_exec_factory.go b/pkg/sql/distsql_spec_exec_factory.go
index 4db1087b14f7..7b359044e538 100644
--- a/pkg/sql/distsql_spec_exec_factory.go
+++ b/pkg/sql/distsql_spec_exec_factory.go
@@ -766,8 +766,6 @@ func (e *distSQLSpecExecFactory) ConstructExplain(
 
 	// TODO(yuzefovich): make sure to return the same nice error in some
 	// variants of EXPLAIN when subqueries are present as we do in the old path.
-	// TODO(yuzefovich): make sure that local plan nodes that create
-	// distributed jobs are shown as "distributed". See distSQLExplainable.
 	p := plan.(*explain.Plan).WrappedPlan.(*planComponents)
 	explain, err := constructExplainDistSQLOrVecNode(options, analyze, stmtType, p, e.planner)
 	if err != nil {
diff --git a/pkg/sql/explain_distsql.go b/pkg/sql/explain_distsql.go
index cf8458dd3296..ecdfa6726fae 100644
--- a/pkg/sql/explain_distsql.go
+++ b/pkg/sql/explain_distsql.go
@@ -13,12 +13,9 @@ package sql
 import (
 	"context"
 
-	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
-	"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/tracing"
 	"github.com/cockroachdb/errors"
 )
@@ -54,43 +51,9 @@ type explainDistSQLRun struct {
 	executedStatement bool
 }
 
-// distSQLExplainable is an interface used for local plan nodes that create
-// distributed jobs. The plan node should implement this interface so that
-// EXPLAIN (DISTSQL) will show the DistSQL plan instead of the local plan node.
-type distSQLExplainable interface {
-	// newPlanForExplainDistSQL returns the DistSQL physical plan that can be
-	// used by the explainDistSQLNode to generate flow specs (and run in the case
-	// of EXPLAIN ANALYZE).
-	newPlanForExplainDistSQL(*PlanningCtx, *DistSQLPlanner) (*PhysicalPlan, error)
-}
-
-// getPlanDistributionForExplainPurposes returns the PlanDistribution that plan
-// will have. It is similar to getPlanDistribution but also pays attention to
-// whether the logical plan will be handled as a distributed job. It should
-// *only* be used in EXPLAIN variants.
-func getPlanDistributionForExplainPurposes(
-	ctx context.Context,
-	p *planner,
-	nodeID *base.SQLIDContainer,
-	distSQLMode sessiondata.DistSQLExecMode,
-	plan planMaybePhysical,
-) physicalplan.PlanDistribution {
-	if plan.isPhysicalPlan() {
-		return plan.physPlan.Distribution
-	}
-	if _, ok := plan.planNode.(distSQLExplainable); ok {
-		// This is a special case for plans that will be actually distributed
-		// but are represented using local plan nodes (for example, "create
-		// statistics" is handled by the jobs framework which is responsible
-		// for setting up the correct DistSQL infrastructure).
-		return physicalplan.FullyDistributedPlan
-	}
-	return getPlanDistribution(ctx, p, nodeID, distSQLMode, plan)
-}
-
 func (n *explainDistSQLNode) startExec(params runParams) error {
 	distSQLPlanner := params.extendedEvalCtx.DistSQLPlanner
-	distribution := getPlanDistributionForExplainPurposes(
+	distribution := getPlanDistribution(
 		params.ctx, params.p, params.extendedEvalCtx.ExecCfg.NodeID,
 		params.extendedEvalCtx.SessionData.DistSQLMode, n.plan.main,
 	)
@@ -305,15 +268,5 @@ func newPhysPlanForExplainPurposes(
 	if plan.isPhysicalPlan() {
 		return plan.physPlan.PhysicalPlan, nil
 	}
-	var physPlan *PhysicalPlan
-	var err error
-	if planNode, ok := plan.planNode.(distSQLExplainable); ok {
-		physPlan, err = planNode.newPlanForExplainDistSQL(planCtx, distSQLPlanner)
-	} else {
-		physPlan, err = distSQLPlanner.createPhysPlanForPlanNode(planCtx, plan.planNode)
-	}
-	if err != nil {
-		return nil, err
-	}
-	return physPlan, nil
+	return distSQLPlanner.createPhysPlanForPlanNode(planCtx, plan.planNode)
 }
diff --git a/pkg/sql/explain_plan.go b/pkg/sql/explain_plan.go
index 02c22afc533a..7e13fe72bdd9 100644
--- a/pkg/sql/explain_plan.go
+++ b/pkg/sql/explain_plan.go
@@ -129,7 +129,7 @@ func explainGetDistributedAndVectorized(
 	// Determine the "distributed" and "vectorized" values, which we will emit as
 	// special rows.
 	distSQLPlanner := params.extendedEvalCtx.DistSQLPlanner
-	distribution = getPlanDistributionForExplainPurposes(
+	distribution = getPlanDistribution(
 		params.ctx, params.p, params.extendedEvalCtx.ExecCfg.NodeID,
 		params.extendedEvalCtx.SessionData.DistSQLMode, plan.main,
 	)
diff --git a/pkg/sql/explain_vec.go b/pkg/sql/explain_vec.go
index 375417ef0f9f..54e653bae85c 100644
--- a/pkg/sql/explain_vec.go
+++ b/pkg/sql/explain_vec.go
@@ -53,7 +53,7 @@ type flowWithNode struct {
 func (n *explainVecNode) startExec(params runParams) error {
 	n.run.values = make(tree.Datums, 1)
 	distSQLPlanner := params.extendedEvalCtx.DistSQLPlanner
-	distribution := getPlanDistributionForExplainPurposes(
+	distribution := getPlanDistribution(
 		params.ctx, params.p, params.extendedEvalCtx.ExecCfg.NodeID,
 		params.extendedEvalCtx.SessionData.DistSQLMode, n.plan.main,
 	)
diff --git a/pkg/sql/opt_exec_factory.go b/pkg/sql/opt_exec_factory.go
index f383d66a85c1..3879c2a59426 100644
--- a/pkg/sql/opt_exec_factory.go
+++ b/pkg/sql/opt_exec_factory.go
@@ -46,6 +46,9 @@ import (
 
 type execFactory struct {
 	planner *planner
+	// isExplain is true if this factory is used to build a statement inside
+	// EXPLAIN or EXPLAIN ANALYZE.
+	isExplain bool
 }
 
 var _ exec.Factory = &execFactory{}
@@ -1852,10 +1855,14 @@ func (ef *execFactory) ConstructCreateStatistics(cs *tree.CreateStats) (exec.Nod
 	); err != nil {
 		return nil, err
 	}
+	// Don't run as a job if we are inside an EXPLAIN / EXPLAIN ANALYZE. That will
+	// allow us to get insight into the actual execution.
+	runAsJob := !ef.isExplain
 
 	return &createStatsNode{
 		CreateStats: *cs,
 		p:           ef.planner,
+		runAsJob:    runAsJob,
 	}, nil
 }
 
@@ -1870,7 +1877,10 @@ func (ef *execFactory) ConstructExplain(
 		return nil, errors.New("ENV only supported with (OPT) option")
 	}
 
-	explainFactory := explain.NewFactory(ef)
+	explainFactory := explain.NewFactory(&execFactory{
+		planner:   ef.planner,
+		isExplain: true,
+	})
 	plan, err := buildFn(explainFactory)
 	if err != nil {
 		return nil, err

From 44bb14ebe330980f9d568695cf8bac6395419c4a Mon Sep 17 00:00:00 2001
From: Radu Berinde <radu@cockroachlabs.com>
Date: Mon, 7 Dec 2020 16:56:50 -0500
Subject: [PATCH 8/9] sql: make CREATE STATISTICS work properly with EXPLAIN
 ANALYZE (DEBUG)

With this change, the bundle now contains the diagram for the
underlying plan.

Release note: None
---
 pkg/sql/instrumentation.go  | 8 ++++++++
 pkg/sql/opt_exec_factory.go | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/pkg/sql/instrumentation.go b/pkg/sql/instrumentation.go
index 1805f1e9cff3..d1e81aa6d26a 100644
--- a/pkg/sql/instrumentation.go
+++ b/pkg/sql/instrumentation.go
@@ -258,6 +258,14 @@ func (ih *instrumentationHelper) ShouldCollectBundle() bool {
 	return ih.collectBundle
 }
 
+// ShouldUseJobForCreateStats indicates if we should run CREATE STATISTICS as a
+// job (normally true). It is false if we are running a statement under
+// EXPLAIN ANALYZE, in which case we want to run the CREATE STATISTICS plan
+// directly.
+func (ih *instrumentationHelper) ShouldUseJobForCreateStats() bool {
+	return ih.outputMode != explainAnalyzePlanOutput && ih.outputMode != explainAnalyzeDebugOutput
+}
+
 // ShouldBuildExplainPlan returns true if we should build an explain plan and
 // call RecordExplainPlan.
 func (ih *instrumentationHelper) ShouldBuildExplainPlan() bool {
diff --git a/pkg/sql/opt_exec_factory.go b/pkg/sql/opt_exec_factory.go
index 3879c2a59426..301600fbd32e 100644
--- a/pkg/sql/opt_exec_factory.go
+++ b/pkg/sql/opt_exec_factory.go
@@ -1857,7 +1857,7 @@ func (ef *execFactory) ConstructCreateStatistics(cs *tree.CreateStats) (exec.Nod
 	}
 	// Don't run as a job if we are inside an EXPLAIN / EXPLAIN ANALYZE. That will
 	// allow us to get insight into the actual execution.
-	runAsJob := !ef.isExplain
+	runAsJob := !ef.isExplain && ef.planner.instrumentation.ShouldUseJobForCreateStats()
 
 	return &createStatsNode{
 		CreateStats: *cs,

From a5ebd6f76525f15a0d504725ff6c4da8834851db Mon Sep 17 00:00:00 2001
From: Radu Berinde <radu@cockroachlabs.com>
Date: Sat, 5 Dec 2020 23:25:55 -0500
Subject: [PATCH 9/9] opt: optbuilder support for scanning virtual columns

This change adds support for scanning tables with virtual columns. The
virtual columns are projected on top of the Scan.

Release note: None
---
 pkg/sql/opt/optbuilder/select.go              | 168 ++++++++++--------
 .../opt/optbuilder/testdata/virtual-columns   |  37 ++++
 2 files changed, 135 insertions(+), 70 deletions(-)
 create mode 100644 pkg/sql/opt/optbuilder/testdata/virtual-columns

diff --git a/pkg/sql/opt/optbuilder/select.go b/pkg/sql/opt/optbuilder/select.go
index 5722b18f5f42..316d236ecc75 100644
--- a/pkg/sql/opt/optbuilder/select.go
+++ b/pkg/sql/opt/optbuilder/select.go
@@ -117,7 +117,7 @@ func (b *Builder) buildDataSource(
 					includeMutations:       false,
 					includeSystem:          true,
 					includeVirtualInverted: false,
-					includeVirtualComputed: false,
+					includeVirtualComputed: true,
 				}),
 				indexFlags, locking, inScope,
 			)
@@ -401,7 +401,7 @@ func (b *Builder) buildScanFromTableRef(
 			includeMutations:       false,
 			includeSystem:          true,
 			includeVirtualInverted: false,
-			includeVirtualComputed: false,
+			includeVirtualComputed: true,
 		})
 	}
 
@@ -419,9 +419,14 @@ func (b *Builder) addTable(tab cat.Table, alias *tree.TableName) *opt.TableMeta
 	return md.TableMeta(tabID)
 }
 
-// buildScan builds a memo group for a ScanOp expression on the given table.
+// buildScan builds a memo group for a ScanOp expression on the given table. If
+// the ordinals list contains any VirtualComputed columns, a ProjectOp is built
+// on top.
 //
-// The scan projects the given table ordinals.
+// The resulting scope and expression output the given table ordinals. If an
+// ordinal is for a VirtualComputed column, the ordinals it depends on must also
+// be in the list (in practice, this coincides with all "ordinary" table columns
+// being in the list).
 //
 // If scanMutationCols is true, then include columns being added or dropped from
 // the table. These are currently required by the execution engine as "fetch
@@ -453,20 +458,26 @@ func (b *Builder) buildScan(
 
 	outScope = inScope.push()
 
-	var tabColIDs opt.ColSet
+	// We collect VirtualComputed columns separately; these cannot be scanned,
+	// they can only be projected afterward.
+	var tabColIDs, virtualColIDs opt.ColSet
 	outScope.cols = make([]scopeColumn, len(ordinals))
 	for i, ord := range ordinals {
 		col := tab.Column(ord)
 		colID := tabID.ColumnID(ord)
-		tabColIDs.Add(colID)
 		name := col.ColName()
 		kind := col.Kind()
+		if kind != cat.VirtualComputed {
+			tabColIDs.Add(colID)
+		} else {
+			virtualColIDs.Add(colID)
+		}
 		outScope.cols[i] = scopeColumn{
 			id:           colID,
 			name:         name,
 			table:        tabMeta.Alias,
 			typ:          col.DatumType(),
-			hidden:       col.IsHidden() || kind != cat.Ordinary,
+			hidden:       col.IsHidden() || (kind != cat.Ordinary && kind != cat.VirtualComputed),
 			kind:         kind,
 			mutation:     kind == cat.WriteOnly || kind == cat.DeleteOnly,
 			tableOrdinal: ord,
@@ -485,79 +496,96 @@ func (b *Builder) buildScan(
 		private := memo.ScanPrivate{Table: tabID, Cols: tabColIDs}
 		outScope.expr = b.factory.ConstructScan(&private)
 
-		// Virtual tables should not be collected as view dependencies.
-	} else {
-		private := memo.ScanPrivate{Table: tabID, Cols: tabColIDs}
-		if indexFlags != nil {
-			private.Flags.NoIndexJoin = indexFlags.NoIndexJoin
-			if indexFlags.Index != "" || indexFlags.IndexID != 0 {
-				idx := -1
-				for i := 0; i < tab.IndexCount(); i++ {
-					if tab.Index(i).Name() == tree.Name(indexFlags.Index) ||
-						tab.Index(i).ID() == cat.StableID(indexFlags.IndexID) {
-						idx = i
-						break
-					}
+		// Note: virtual tables should not be collected as view dependencies.
+		return outScope
+	}
+
+	private := memo.ScanPrivate{Table: tabID, Cols: tabColIDs}
+	if indexFlags != nil {
+		private.Flags.NoIndexJoin = indexFlags.NoIndexJoin
+		if indexFlags.Index != "" || indexFlags.IndexID != 0 {
+			idx := -1
+			for i := 0; i < tab.IndexCount(); i++ {
+				if tab.Index(i).Name() == tree.Name(indexFlags.Index) ||
+					tab.Index(i).ID() == cat.StableID(indexFlags.IndexID) {
+					idx = i
+					break
 				}
-				if idx == -1 {
-					var err error
-					if indexFlags.Index != "" {
-						err = errors.Errorf("index %q not found", tree.ErrString(&indexFlags.Index))
-					} else {
-						err = errors.Errorf("index [%d] not found", indexFlags.IndexID)
-					}
-					panic(err)
+			}
+			if idx == -1 {
+				var err error
+				if indexFlags.Index != "" {
+					err = errors.Errorf("index %q not found", tree.ErrString(&indexFlags.Index))
+				} else {
+					err = errors.Errorf("index [%d] not found", indexFlags.IndexID)
 				}
-				private.Flags.ForceIndex = true
-				private.Flags.Index = idx
-				private.Flags.Direction = indexFlags.Direction
+				panic(err)
 			}
+			private.Flags.ForceIndex = true
+			private.Flags.Index = idx
+			private.Flags.Direction = indexFlags.Direction
 		}
-		if locking.isSet() {
-			private.Locking = locking.get()
-		}
-
-		b.addCheckConstraintsForTable(tabMeta)
-		b.addComputedColsForTable(tabMeta)
+	}
+	if locking.isSet() {
+		private.Locking = locking.get()
+	}
 
-		outScope.expr = b.factory.ConstructScan(&private)
+	b.addCheckConstraintsForTable(tabMeta)
+	b.addComputedColsForTable(tabMeta)
+
+	outScope.expr = b.factory.ConstructScan(&private)
+
+	if !virtualColIDs.Empty() {
+		// Project the expressions for the virtual columns (and pass through all
+		// scanned columns).
+		// TODO(radu): we don't currently support virtual columns depending on other
+		// virtual columns.
+		proj := make(memo.ProjectionsExpr, 0, virtualColIDs.Len())
+		virtualColIDs.ForEach(func(col opt.ColumnID) {
+			item := b.factory.ConstructProjectionsItem(tabMeta.ComputedCols[col], col)
+			if !item.ScalarProps().OuterCols.SubsetOf(tabColIDs) {
+				panic(errors.AssertionFailedf("scanned virtual column depends on non-scanned column"))
+			}
+			proj = append(proj, item)
+		})
+		outScope.expr = b.factory.ConstructProject(outScope.expr, proj, tabColIDs)
+	}
 
-		// Add the partial indexes after constructing the scan so we can use the
-		// logical properties of the scan to fully normalize the index
-		// predicates. Partial index predicates are only added if the outScope
-		// contains all the table's ordinary columns. If it does not, partial
-		// index predicates cannot be built because they may reference columns
-		// not in outScope. In the most common case, the outScope has the same
-		// number of columns as the table and we can skip checking that each
-		// ordinary column exists in outScope.
-		containsAllOrdinaryTableColumns := true
-		if len(outScope.cols) != tab.ColumnCount() {
-			for i := 0; i < tab.ColumnCount(); i++ {
-				col := tab.Column(i)
-				if col.Kind() == cat.Ordinary && !outScope.colSet().Contains(tabID.ColumnID(col.Ordinal())) {
-					containsAllOrdinaryTableColumns = false
-					break
-				}
+	// Add the partial indexes after constructing the scan so we can use the
+	// logical properties of the scan to fully normalize the index
+	// predicates. Partial index predicates are only added if the outScope
+	// contains all the table's ordinary columns. If it does not, partial
+	// index predicates cannot be built because they may reference columns
+	// not in outScope. In the most common case, the outScope has the same
+	// number of columns as the table and we can skip checking that each
+	// ordinary column exists in outScope.
+	containsAllOrdinaryTableColumns := true
+	if len(outScope.cols) != tab.ColumnCount() {
+		for i := 0; i < tab.ColumnCount(); i++ {
+			col := tab.Column(i)
+			if col.Kind() == cat.Ordinary && !outScope.colSet().Contains(tabID.ColumnID(col.Ordinal())) {
+				containsAllOrdinaryTableColumns = false
+				break
 			}
 		}
-		if containsAllOrdinaryTableColumns {
-			b.addPartialIndexPredicatesForTable(tabMeta, outScope)
-		}
+	}
+	if containsAllOrdinaryTableColumns {
+		b.addPartialIndexPredicatesForTable(tabMeta, outScope)
+	}
 
-		if b.trackViewDeps {
-			dep := opt.ViewDep{DataSource: tab}
-			dep.ColumnIDToOrd = make(map[opt.ColumnID]int)
-			// We will track the ColumnID to Ord mapping so Ords can be added
-			// when a column is referenced.
-			for i, col := range outScope.cols {
-				dep.ColumnIDToOrd[col.id] = ordinals[i]
-			}
-			if private.Flags.ForceIndex {
-				dep.SpecificIndex = true
-				dep.Index = private.Flags.Index
-			}
-			b.viewDeps = append(b.viewDeps, dep)
+	if b.trackViewDeps {
+		dep := opt.ViewDep{DataSource: tab}
+		dep.ColumnIDToOrd = make(map[opt.ColumnID]int)
+		// We will track the ColumnID to Ord mapping so Ords can be added
+		// when a column is referenced.
+		for i, col := range outScope.cols {
+			dep.ColumnIDToOrd[col.id] = ordinals[i]
+		}
+		if private.Flags.ForceIndex {
+			dep.SpecificIndex = true
+			dep.Index = private.Flags.Index
 		}
+		b.viewDeps = append(b.viewDeps, dep)
 	}
 	return outScope
 }
diff --git a/pkg/sql/opt/optbuilder/testdata/virtual-columns b/pkg/sql/opt/optbuilder/testdata/virtual-columns
new file mode 100644
index 000000000000..44f796fa9992
--- /dev/null
+++ b/pkg/sql/opt/optbuilder/testdata/virtual-columns
@@ -0,0 +1,37 @@
+exec-ddl
+CREATE TABLE t (
+  a INT PRIMARY KEY,
+  b INT,
+  c INT AS (a+b) VIRTUAL
+)
+----
+
+build
+SELECT * FROM t
+----
+project
+ ├── columns: a:1!null b:2 c:3
+ └── project
+      ├── columns: c:3 a:1!null b:2 crdb_internal_mvcc_timestamp:4
+      ├── scan t
+      │    ├── columns: a:1!null b:2 crdb_internal_mvcc_timestamp:4
+      │    └── computed column expressions
+      │         └── c:3
+      │              └── a:1 + b:2
+      └── projections
+           └── a:1 + b:2 [as=c:3]
+
+build
+SELECT c FROM t
+----
+project
+ ├── columns: c:3
+ └── project
+      ├── columns: c:3 a:1!null b:2 crdb_internal_mvcc_timestamp:4
+      ├── scan t
+      │    ├── columns: a:1!null b:2 crdb_internal_mvcc_timestamp:4
+      │    └── computed column expressions
+      │         └── c:3
+      │              └── a:1 + b:2
+      └── projections
+           └── a:1 + b:2 [as=c:3]