Merge #73493

73493: schemachanger: logictest with new schema changer r=fqazi a=fqazi

This pull request will do a large number of small fixes to allow us to enable the logicttest for the declarative schema changer. It's best to look at this pull request commit by commit, since each fix addresses some part limitation of the declarative schema changer.

Co-authored-by: Faizan Qazi <[email protected]>

pkg/sql/drop_schema.go

@@ -102,7 +102,7 @@ func (p *planner) DropSchema(ctx context.Context, n *tree.DropSchema) (planNode,
 			}
 			if !(isAdmin || hasOwnership) {
 				return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
-					"must be owner of schema %q", sc.GetName())
+					"must be owner of schema %s", tree.Name(sc.GetName()))
 			}
 			namesBefore := len(d.objectNamesToDelete)
 			if err := d.collectObjectsInSchema(ctx, p, db, sc); err != nil {

pkg/sql/logictest/logic.go

@@ -507,6 +507,9 @@ type testClusterConfig struct {
 	// localities is set if nodes should be set to a particular locality.
 	// Nodes are 1-indexed.
 	localities map[int]roachpb.Locality
+	// declarativeSchemaChanger determines if the declarative schema changer
+	// is enabled.
+	declarativeSchemaChanger bool
 }
 
 const threeNodeTenantConfigName = "3node-tenant"
@@ -581,6 +584,13 @@ var logicTestConfigs = []testClusterConfig{
 		overrideDistSQLMode: "off",
 		overrideAutoStats:   "false",
 	},
+	{
+		name:                     "local-declarative-schema",
+		numNodes:                 5,
+		overrideDistSQLMode:      "off",
+		overrideAutoStats:        "false",
+		declarativeSchemaChanger: true,
+	},
 	{
 		name:                "local-vec-off",
 		numNodes:            1,
@@ -799,6 +809,7 @@ var (
 	defaultConfigName  = "default-configs"
 	defaultConfigNames = []string{
 		"local",
+		"local-declarative-schema",
 		"local-vec-off",
 		"local-spec-planning",
 		"fakedist",
@@ -1661,6 +1672,14 @@ func (t *logicTest) newCluster(serverArgs TestServerArgs, opts []clusterOpt) {
 		); err != nil {
 			t.Fatal(err)
 		}
+
+		if cfg.declarativeSchemaChanger {
+			if _, err := conn.Exec(
+				"SET CLUSTER SETTING sql.defaults.experimental_new_schema_changer.enabled = 'on'",
+			); err != nil {
+				t.Fatal(err)
+			}
+		}
 	}
 
 	if cfg.overrideDistSQLMode != "" {

pkg/sql/logictest/testdata/logic_test/create_index

@@ -233,6 +233,14 @@ CREATE INDEX "'t1-esc-index'" ON "'t1-esc'"(name)
 
 subtest resume-with-diff-tenant-resume-spans
 
+let $schema_changer_state
+SELECT value FROM information_schema.session_variables where variable='experimental_use_new_schema_changer'
+
+# Intentionally, disable the declarative schema changer for this
+# part of the test, since we are pausing jobs intentionally below.
+statement ok
+SET experimental_use_new_schema_changer = 'off'
+
 statement ok
 SET CLUSTER SETTING jobs.registry.interval.adopt = '50ms';
 
@@ -300,3 +308,7 @@ succeeded
 
 statement ok
 SET CLUSTER SETTING jobs.registry.interval.cancel = DEFAULT;
+
+# Restore the schema changer state back.
+statement ok
+SET experimental_use_new_schema_changer = $schema_changer_state

pkg/sql/logictest/testdata/logic_test/drop_database

@@ -25,6 +25,7 @@ DROP DATABASE "foo-bar" RESTRICT
 statement ok
 DROP DATABASE "foo-bar" CASCADE
 
+skipif config local-declarative-schema
 query TTT
 SELECT name, database_name, state FROM crdb_internal.tables WHERE name = 't'
 ----
@@ -38,6 +39,7 @@ postgres   root  NULL  {}  NULL
 system     node  NULL  {}  NULL
 test       root  NULL  {}  NULL
 
+skipif config local-declarative-schema
 query TT
 SELECT job_type, status FROM [SHOW JOBS] WHERE user_name = 'root'
 ----
@@ -219,6 +221,7 @@ CREATE TABLE constraint_db.t2 (
 statement ok
 DROP DATABASE constraint_db CASCADE
 
+skipif config local-declarative-schema
 query TTT
 WITH cte AS (
   SELECT job_type, description, status
@@ -276,7 +279,11 @@ statement ok
 DROP DATABASE empty
 
 query TT
-SELECT job_type, status FROM [SHOW JOBS] WHERE description LIKE '%empty%'
+SELECT replace(job_type, 'NEW SCHEMA CHANGE', 'SCHEMA CHANGE'), status
+  FROM [SHOW JOBS]
+ WHERE description LIKE '%empty%'
+    OR job_type = 'NEW SCHEMA CHANGE'
+ LIMIT 1
 ----
 SCHEMA CHANGE       succeeded
 

pkg/sql/logictest/testdata/logic_test/drop_table

@@ -31,7 +31,17 @@ DROP TABLE a
 # TODO (lucy): Update this if/when we decide to change how these jobs queued by
 # the startup migration are handled.
 query TT
-SELECT job_type, status FROM [SHOW JOBS] WHERE user_name = 'root' AND (job_type = 'SCHEMA CHANGE GC' OR (job_type = 'SCHEMA CHANGE' AND description != 'updating privileges'))
+SELECT replace(job_type, 'NEW SCHEMA CHANGE', 'SCHEMA CHANGE'), status
+  FROM [SHOW JOBS]
+ WHERE (user_name = 'root' OR user_name = 'node')
+       AND (
+			job_type = 'SCHEMA CHANGE GC'
+			OR job_type = 'NEW SCHEMA CHANGE'
+			OR (
+					job_type = 'SCHEMA CHANGE'
+					AND description != 'updating privileges'
+				)
+		);
 ----
 SCHEMA CHANGE     succeeded
 SCHEMA CHANGE     succeeded

pkg/sql/logictest/testdata/logic_test/drop_type

@@ -359,7 +359,7 @@ CREATE TYPE schema_to_drop.typ AS ENUM ('a');
 CREATE TABLE t (k schema_to_drop.typ PRIMARY KEY);
 CREATE TABLE schema_to_drop.t (k schema_to_drop.typ PRIMARY KEY);
 
-statement error pgcode 0A000 unimplemented: cannot drop type "test.schema_to_drop._typ" because other objects \(\[test\.public\.t\]\) still depend on it
+statement error pgcode 0A000 unimplemented: cannot drop type "test.schema_to_drop.(_)?typ" because other objects \(\[test\.public\.t\]\) still depend on it
 DROP SCHEMA schema_to_drop CASCADE;
 
 statement ok

pkg/sql/logictest/testdata/logic_test/event_log

@@ -373,10 +373,21 @@ DROP DATABASE IF EXISTS othereventlogtest CASCADE
 # verify event is there, and cascading table drops are logged.
 
 query IT
-SELECT "reportingID", info::JSONB - 'Timestamp' - 'DescriptorID'
-FROM system.eventlog
-WHERE "eventType" = 'drop_database'
-  AND info::JSONB->>'Statement' LIKE 'DROP DATABASE eventlogtest%'
+SELECT "reportingID",
+       (info::JSONB - 'Timestamp' - 'DescriptorID')
+       || (
+			SELECT json_build_object(
+					'DroppedSchemaObjects',
+					json_agg(value ORDER BY value)
+			       )
+			  FROM ROWS FROM (
+					json_array_elements((info::JSONB)->'DroppedSchemaObjects')
+			       )
+		)
+  FROM system.eventlog
+ WHERE "eventType" = 'drop_database'
+       AND info::JSONB->>'Statement' LIKE 'DROP DATABASE eventlogtest%'
+ ORDER BY "timestamp";
 ----
 1  {"DatabaseName": "eventlogtest", "DroppedSchemaObjects": ["eventlogtest.public.anothertesttable", "eventlogtest.public.testtable"], "EventType": "drop_database", "Statement": "DROP DATABASE eventlogtest CASCADE", "Tag": "DROP DATABASE", "User": "root"}
 
@@ -451,6 +462,7 @@ AND info NOT LIKE '%sql.stats.automatic_collection.enabled%'
 AND info NOT LIKE '%sql.defaults.vectorize%'
 AND info NOT LIKE '%sql.testing%'
 AND info NOT LIKE '%sql.defaults.experimental_distsql_planning%'
+AND info NOT LIKE '%sql.defaults.experimental_new_schema_changer.enabled%'
 ORDER BY "timestamp", info
 ----
 0  1  {"ApplicationName": "$ internal-optInToDiagnosticsStatReporting", "EventType": "set_cluster_setting", "SettingName": "diagnostics.reporting.enabled", "Statement": "SET CLUSTER SETTING \"diagnostics.reporting.enabled\" = true", "Tag": "SET CLUSTER SETTING", "User": "root", "Value": "true"}
@@ -471,6 +483,7 @@ AND info NOT LIKE '%sql.stats.automatic_collection.enabled%'
 AND info NOT LIKE '%sql.defaults.vectorize%'
 AND info NOT LIKE '%sql.testing%'
 AND info NOT LIKE '%sql.defaults.experimental_distsql_planning%'
+AND info NOT LIKE '%sql.defaults.experimental_new_schema_changer.enabled%'
 ORDER BY "timestamp", info
 ----
 0  1  {"ApplicationName": "$ internal-optInToDiagnosticsStatReporting", "EventType": "set_cluster_setting", "SettingName": "diagnostics.reporting.enabled", "Statement": "SET CLUSTER SETTING \"diagnostics.reporting.enabled\" = true", "Tag": "SET CLUSTER SETTING", "User": "root", "Value": "true"}

pkg/sql/logictest/testdata/logic_test/fk

@@ -999,14 +999,19 @@ DELETE FROM a WHERE id > 2
 statement ok
 DELETE FROM b WHERE id = 2
 
+let $a_table_id
+SELECT 'a'::REGCLASS::INT::STRING
+
 statement ok
 DROP TABLE a
 
 # Check proper GC job description formatting when removing FK back-references when dropping a table (#59221).
 query T
-SELECT description FROM [SHOW JOBS] WHERE job_type = 'SCHEMA CHANGE GC' AND description LIKE 'GC for updating table "a"%'
+SELECT job_type FROM [SHOW JOBS] WHERE job_type = 'SCHEMA CHANGE GC' AND
+ (description LIKE 'GC for updating table "a"%' OR
+  description = 'GC for dropping descriptor ' || $a_table_id);
 ----
-GC for updating table "a" after removing constraint "fk_self_id" from table "test.public.a"; DROP TABLE test.public.a
+SCHEMA CHANGE GC
 
 statement ok
 DROP TABLE b

pkg/sql/logictest/testdata/logic_test/information_schema

@@ -4632,9 +4632,10 @@ subtest variables
 
 ## excluding crdb_version and session_id variables as they are generated on each
 ## commit/session; distsql, experimental_distsql_planning and vectorize as they
-## depend on distsql and vectorization turned on/off.
+## depend on distsql and vectorization turned on/off. Also excluding
+## experimental_use_new_schema_changer, which can be adjusted based on the mode.
 query TT colnames
-SELECT * FROM information_schema.session_variables where variable not in ('crdb_version', 'session_id', 'distsql', 'vectorize', 'experimental_distsql_planning')
+SELECT * FROM information_schema.session_variables where variable not in ('crdb_version', 'session_id', 'distsql', 'vectorize', 'experimental_distsql_planning', 'experimental_use_new_schema_changer')
 ----
 variable                                              value
 allow_prepare_as_opt_plan                             off
@@ -4676,7 +4677,6 @@ experimental_enable_hash_sharded_indexes              off
 experimental_enable_implicit_column_partitioning      off
 experimental_enable_temp_tables                       off
 experimental_enable_unique_without_index_constraints  on
-experimental_use_new_schema_changer                   off
 extra_float_digits                                    0
 force_savepoint_restart                               off
 foreign_key_cascades_limit                            10000

pkg/sql/logictest/testdata/logic_test/jobs

@@ -1,3 +1,4 @@
+# LogicTest: !local-declarative-schema
 # 3node-tenant fails due to AUTO STATS being returned in SHOW JOBS. Changing
 # the cluster setting to disable auto stats doesn't work:
 # https://github.com/cockroachdb/cockroach/issues/47918

pkg/sql/logictest/testdata/logic_test/new_schema_changer

@@ -457,7 +457,7 @@ CREATE TABLE defaultdb.sq1dep (
 	rand_col INT8 DEFAULT nextval('defaultdb.sq1')
 );
 
-statement error cannot drop table a because other objects depend on it
+statement error cannot drop table shipments because other objects depend on it
 DROP TABLE defaultdb.shipments;
 
 statement ok
@@ -1128,8 +1128,8 @@ SELECT "reportingID", info::JSONB - 'Timestamp' - 'DescriptorID'
 FROM system.eventlog
 ORDER BY timestamp, info DESC;
 ----
-1  {"DatabaseName": "'db1-a'", "DroppedSchemaObjects": ["'db1-a'.public", "'db1-a'.sc1", "'db1-a'.sc2"], "EventType": "drop_database", "Statement": "DROP DATABASE \"'db1-a'\" CASCADE", "Tag": "DROP DATABASE", "User": "root"}
-1  {"DatabaseName": "db2", "DroppedSchemaObjects": ["db2.public", "db2.sc3"], "EventType": "drop_database", "Statement": "DROP DATABASE db2 CASCADE", "Tag": "DROP DATABASE", "User": "root"}
+1  {"DatabaseName": "'db1-a'", "DroppedSchemaObjects": ["\"'db1-a'\".public.\"'t1-esc'\""], "EventType": "drop_database", "Statement": "DROP DATABASE \"'db1-a'\" CASCADE", "Tag": "DROP DATABASE", "User": "root"}
+1  {"DatabaseName": "db2", "EventType": "drop_database", "Statement": "DROP DATABASE db2 CASCADE", "Tag": "DROP DATABASE", "User": "root"}
 
 # Sanity: Dropping multiple objects in the builder or resolving any dependencies
 # should function fine.
@@ -1325,7 +1325,7 @@ user testuser
 statement ok
 SET experimental_use_new_schema_changer = 'on'
 
-statement error must be owner of schema \"sc1\"
+statement error must be owner of schema sc1
 DROP SCHEMA sc1;
 
 statement error user testuser does not have DROP privilege on database db1

pkg/sql/logictest/testdata/logic_test/schema

@@ -423,7 +423,7 @@ CREATE TYPE privs.denied AS ENUM ('denied')
 statement error pq: must be owner of schema "privs"
 ALTER SCHEMA privs RENAME TO denied
 
-statement error must be owner of schema \"privs\"
+statement error must be owner of schema privs
 DROP SCHEMA privs
 
 # Test the usage privilege.

pkg/sql/logictest/testdata/logic_test/system

@@ -1054,6 +1054,7 @@ AND name != 'sql.stats.automatic_collection.enabled'
 AND name NOT LIKE '%sql.defaults.vectorize%'
 AND name NOT LIKE '%sql.testing%'
 AND name NOT LIKE '%sql.defaults.experimental_distsql_planning%'
+AND name != 'sql.defaults.experimental_new_schema_changer.enabled'
 ORDER BY name
 ----
 cluster.secret
@@ -1072,6 +1073,7 @@ AND name != 'sql.stats.automatic_collection.enabled'
 AND name NOT LIKE '%sql.defaults.vectorize%'
 AND name NOT LIKE '%sql.testing%'
 AND name NOT LIKE '%sql.defaults.experimental_distsql_planning%'
+AND name NOT LIKE '%sql.defaults.experimental_new_schema_changer.enabled%'
 ORDER BY name
 ----
 cluster.secret
@@ -1091,7 +1093,8 @@ SELECT name, value
 FROM system.settings
 WHERE name NOT IN ('version', 'sql.defaults.distsql', 'cluster.secret',
   'sql.stats.automatic_collection.enabled', 'sql.defaults.vectorize',
-  'sql.defaults.experimental_distsql_planning')
+  'sql.defaults.experimental_distsql_planning',
+  'sql.defaults.experimental_new_schema_changer.enabled')
 ORDER BY name
 ----
 diagnostics.reporting.enabled                      true
@@ -1106,7 +1109,7 @@ SELECT name, value
 FROM system.settings
 WHERE name NOT IN ('version', 'sql.defaults.distsql', 'cluster.secret',
   'sql.stats.automatic_collection.enabled', 'sql.defaults.vectorize',
-  'sql.defaults.experimental_distsql_planning')
+  'sql.defaults.experimental_distsql_planning', 'sql.defaults.experimental_new_schema_changer.enabled')
 ORDER BY name
 ----
 diagnostics.reporting.enabled                      true

pkg/sql/logictest/testdata/logic_test/views

@@ -1205,10 +1205,20 @@ DROP table t1nest CASCADE
 
 # Validate the objects being dropped
 query IT
-SELECT "reportingID", info::JSONB - 'Timestamp' - 'DescriptorID'
-FROM system.eventlog
-WHERE "eventType" in ('drop_view', 'drop_table')
-ORDER BY "timestamp" DESC, info
-LIMIT 1
+SELECT "reportingID",
+       (info::JSONB - 'Timestamp' - 'DescriptorID')
+       || (
+			SELECT json_build_object(
+					'CascadeDroppedViews',
+					json_agg(value ORDER BY value)
+			       )
+			  FROM ROWS FROM (
+					json_array_elements((info::JSONB)->'CascadeDroppedViews')
+			       )
+		)
+  FROM system.eventlog
+ WHERE "eventType" IN ('drop_view', 'drop_table')
+ ORDER BY "timestamp" DESC
+ LIMIT 1;
 ----
- 1  {"CascadeDroppedViews": ["db2.public.v3nest", "db2.public.v2nest", "db2.public.v1nest"], "EventType": "drop_table", "Statement": "DROP TABLE db2.public.t1nest CASCADE", "TableName": "db2.public.t1nest", "Tag": "DROP TABLE", "User": "root"}
+1  {"CascadeDroppedViews": ["db2.public.v1nest", "db2.public.v2nest", "db2.public.v3nest"], "EventType": "drop_table", "Statement": "DROP TABLE db2.public.t1nest CASCADE", "TableName": "db2.public.t1nest", "Tag": "DROP TABLE", "User": "root"}

pkg/sql/schema_change_cluster_setting.go

@@ -16,6 +16,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/featureflag"
 	"github.com/cockroachdb/cockroach/pkg/settings"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 )
 
 // featureSchemaChangeEnabled is the cluster setting used to enable and disable
@@ -43,3 +44,8 @@ func checkSchemaChangeEnabled(
 	}
 	return nil
 }
+
+// CheckFeature checks if a schema change feature is allowed.
+func (p *planner) CheckFeature(ctx context.Context, featureName tree.SchemaFeatureName) error {
+	return checkSchemaChangeEnabled(ctx, p.ExecCfg(), string(featureName))
+}

pkg/sql/schema_change_plan_node.go

@@ -66,6 +66,7 @@ func (p *planner) SchemaChange(ctx context.Context, stmt tree.Statement) (planNo
 		p,
 		p,
 		p,
+		p,
 		p.SessionData(),
 		p.ExecCfg().Settings,
 		scs.stmts,

pkg/sql/schemachanger/scbuild/BUILD.bazel

@@ -18,12 +18,14 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/sql/catalog",
+        "//pkg/sql/catalog/catconstants",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/tabledesc",
         "//pkg/sql/faketreeeval",
         "//pkg/sql/parser",
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",
+        "//pkg/sql/privilege",
         "//pkg/sql/schemachanger/scbuild/internal/scbuildstmt",
         "//pkg/sql/schemachanger/scpb",
         "//pkg/sql/schemachanger/screl",