Merge cockroachdb#105324

105324: sql: fix CTAS when selecting from some internal virtual tables r=yuzefovich a=yuzefovich This commit fixes an oversight where we forgot to set the SQL stats provider in some code paths (namely when `NewInternalPlanner` is called, one example is backfilling query into table that powers CREATE TABLE AS and CREATE MATERIALIZED VIEW AS features), and this is now fixed. Fixes: cockroachdb#76710. Release note (bug fix): Previously, CockroachDB would crash when evaluating CREATE TABLE .. AS or CREATE MATERIALIZED VIEW .. AS statements when AS clause selected data from `crdb_internal.cluster_statement_statistics` or `crdb_internal.cluster_transaction_statistics` virtual tables. The bug has been present since at least 22.1 and is now fixed. Co-authored-by: Yahor Yuzefovich <[email protected]>
wenyihu6 · Jun 22, 2023 · 71228e5 · 71228e5
2 parents 51a41bc + cb097b5
commit 71228e5
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 38 deletions.
diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go
@@ -82,7 +82,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlliveness"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats"
-	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/sslocal"
 	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
@@ -1419,18 +1418,6 @@ func execStatVar(alloc *tree.DatumAlloc, count int64, n appstatspb.NumericStat)
 	return alloc.NewDFloat(tree.DFloat(n.GetVariance(count)))
 }
 
-// getSQLStats retrieves a sqlStats provider from the planner or
-// returns an error if not available. virtualTableName specifies the virtual
-// table for which this sqlStats object is needed.
-func getSQLStats(
-	p *planner, virtualTableName string,
-) (*persistedsqlstats.PersistedSQLStats, error) {
-	if p.extendedEvalCtx.statsProvider == nil {
-		return nil, errors.Newf("%s cannot be used in this context", virtualTableName)
-	}
-	return p.extendedEvalCtx.statsProvider, nil
-}
-
 // legacyAnonymizedStmt is a placeholder value for the
 // crdb_internal.node_statement_statistics(anonymized) column. The column used
 // to contain the SQL statement scrubbed of all identifiers. At the time
@@ -1538,12 +1525,7 @@ CREATE TABLE crdb_internal.node_statement_statistics (
 		}
 
 		var alloc tree.DatumAlloc
-
-		sqlStats, err := getSQLStats(p, "crdb_internal.node_statement_statistics")
-		if err != nil {
-			return err
-		}
-
+		sqlStats := p.extendedEvalCtx.statsProvider
 		nodeID, _ := p.execCfg.NodeInfo.NodeID.OptionalNodeID() // zero if not available
 
 		statementVisitor := func(_ context.Context, stats *appstatspb.CollectedStatementStatistics) error {
@@ -1758,11 +1740,7 @@ CREATE TABLE crdb_internal.node_transaction_statistics (
 			return noViewActivityOrViewActivityRedactedRoleError(p.User())
 		}
 
-		sqlStats, err := getSQLStats(p, "crdb_internal.node_transaction_statistics")
-		if err != nil {
-			return err
-		}
-
+		sqlStats := p.extendedEvalCtx.statsProvider
 		nodeID, _ := p.execCfg.NodeInfo.NodeID.OptionalNodeID() // zero if not available
 
 		var alloc tree.DatumAlloc
@@ -1864,11 +1842,7 @@ CREATE TABLE crdb_internal.node_txn_stats (
 			return err
 		}
 
-		sqlStats, err := getSQLStats(p, "crdb_internal.node_txn_stats")
-		if err != nil {
-			return err
-		}
-
+		sqlStats := p.extendedEvalCtx.statsProvider
 		nodeID, _ := p.execCfg.NodeInfo.NodeID.OptionalNodeID() // zero if not available
 
 		appTxnStatsVisitor := func(appName string, stats *appstatspb.TxnStats) error {

diff --git a/pkg/sql/logictest/testdata/logic_test/crdb_internal b/pkg/sql/logictest/testdata/logic_test/crdb_internal
@@ -1424,3 +1424,10 @@ user root
 
 statement ok
 REVOKE SYSTEM VIEWACTIVITY FROM testuser
+
+# Regression tests for not setting SQL stats provider in the backfill (#76710).
+statement ok
+CREATE TABLE t76710_1 AS SELECT * FROM crdb_internal.statement_statistics;
+
+statement ok
+CREATE MATERIALIZED VIEW t76710_2 AS SELECT fingerprint_id FROM crdb_internal.cluster_statement_statistics;
diff --git a/pkg/sql/logictest/testdata/logic_test/create_table b/pkg/sql/logictest/testdata/logic_test/create_table
@@ -463,17 +463,14 @@ CREATE TABLE error (a INT, b INT, INDEX idx (a), UNIQUE INDEX idx (b))
 statement error pgcode 42P07 duplicate index name: \"idx\"
 CREATE TABLE error (a INT, b INT, UNIQUE INDEX idx (a), UNIQUE INDEX idx (b))
 
-# Regression test for using some virtual tables in CREATE TABLE AS which is not
-# supported at the moment (#65512).
-
-query error crdb_internal.node_statement_statistics cannot be used in this context
-CREATE TABLE ctas AS (SELECT * FROM crdb_internal.node_statement_statistics);
+statement ok
+CREATE TABLE ctas1 AS (SELECT * FROM crdb_internal.node_statement_statistics);
 
-query error crdb_internal.node_transaction_statistics cannot be used in this context
-CREATE TABLE ctas AS (SELECT * FROM crdb_internal.node_transaction_statistics);
+statement ok
+CREATE TABLE ctas2 AS (SELECT * FROM crdb_internal.node_transaction_statistics);
 
-query error crdb_internal.node_txn_stats cannot be used in this context
-CREATE TABLE ctas AS (SELECT * FROM crdb_internal.node_txn_stats);
+statement ok
+CREATE TABLE ctas3 AS (SELECT * FROM crdb_internal.node_txn_stats);
 
 subtest generated_as_identity
 statement ok

diff --git a/pkg/sql/planner.go b/pkg/sql/planner.go
@@ -502,12 +502,14 @@ func internalExtendedEvalCtx(
 	var sqlStatsController eval.SQLStatsController
 	var schemaTelemetryController eval.SchemaTelemetryController
 	var indexUsageStatsController eval.IndexUsageStatsController
+	var sqlStatsProvider *persistedsqlstats.PersistedSQLStats
 	if ief := execCfg.InternalDB; ief != nil {
 		if ief.server != nil {
 			indexUsageStats = ief.server.indexUsageStats
 			sqlStatsController = ief.server.sqlStatsController
 			schemaTelemetryController = ief.server.schemaTelemetryController
 			indexUsageStatsController = ief.server.indexUsageStatsController
+			sqlStatsProvider = ief.server.sqlStats
 		} else {
 			// If the indexUsageStats is nil from the sql.Server, we create a dummy
 			// index usage stats collector. The sql.Server in the ExecutorConfig
@@ -518,6 +520,7 @@ func internalExtendedEvalCtx(
 			sqlStatsController = &persistedsqlstats.Controller{}
 			schemaTelemetryController = &schematelemetrycontroller.Controller{}
 			indexUsageStatsController = &idxusage.Controller{}
+			sqlStatsProvider = &persistedsqlstats.PersistedSQLStats{}
 		}
 	}
 	ret := extendedEvalContext{
@@ -540,6 +543,7 @@ func internalExtendedEvalCtx(
 		Tracing:         &SessionTracing{},
 		Descs:           tables,
 		indexUsageStats: indexUsageStats,
+		statsProvider:   sqlStatsProvider,
 	}
 	ret.SetDeprecatedContext(ctx)
 	ret.copyFromExecCfg(execCfg)