Merge cockroachdb#109818

109818: schemachanger: Disable ALTER TABLE multiple commands by default r=Xiang-Gu a=Xiang-Gu

Disable ALTER TABLE stmt with multiple ADD COLUMN, DROP COLUMN, ALTER PK commands (which is
added in cockroachdb#99526) by default. Customers can enable it by setting `use_declarative_schema_changer` to `unsafe` or `unsafe_always`, or forcefully enable declarative schema changer on `ALTER TABLE` stmts by setting cluster setting `sql.schema.force_declarative_statements='+ALTER TABLE';`.
The plan is to enable it again by default after v23.2 (be it v24.1 or v23.3).

Fixes cockroachdb#108870
Epic: None
Release note: None

Co-authored-by: Xiang Gu <[email protected]>

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -3186,6 +3186,9 @@ t_96728     t_96728_pkey     PRIMARY KEY      PRIMARY KEY (i ASC)
 # This is only supported in declarative schema changer starting from V23_2.
 subtest alter_table_with_multiple_commands_99035
 
+statement ok
+SET CLUSTER SETTING sql.schema.force_declarative_statements = "+ALTER TABLE";
+
 statement ok
 DROP TABLE IF EXISTS t_99035;
 CREATE TABLE t_99035 (i INT PRIMARY KEY, j INT NOT NULL, FAMILY "primary" (i, j));
@@ -3328,6 +3331,11 @@ CREATE TABLE public.t_99035 (
   CONSTRAINT check_j CHECK (j >= 0:::INT8)
 )
 
+statement ok
+RESET CLUSTER SETTING sql.schema.force_declarative_statements;
+
+subtest end
+
 # This subtest disallows using builtin function `cluster_logical_timestamp()`
 # as the default expression when backfilling a column.
 subtest 98269

pkg/sql/logictest/testdata/logic_test/new_schema_changer

@@ -1531,3 +1531,104 @@ SET CLUSTER SETTING sql.schema.force_declarative_statements='+CREATE SEQUENCE'
 skipif config local-mixed-22.2-23.1
 statement ok
 EXPLAIN (DDL) CREATE SEQUENCE sq2
+
+subtest end
+
+# This subtest ensures ALTER TABLE with multiple ADD COLUMN, DROP COLUMN, and/or
+# ALTER PK is disabled with declarative schema changer by default, unless
+# `use_declarative_schema_changer = (unsafe|unsafe_always)` or
+# `sql.schema.force_declarative_statements='+ALTER TABLE'`.
+# TODO (xiang): Enable it by default on the next major release after v23.2, be it v24.1 or v23.3.
+subtest 108870
+
+statement ok
+SET sql_safe_updates = false;
+SET use_declarative_schema_changer = 'on';
+
+statement ok
+SET CLUSTER SETTING sql.schema.force_declarative_statements = '';
+
+statement ok
+CREATE TABLE t_108870 (i INT PRIMARY KEY, j INT NOT NULL);
+
+# Ensure ALTER TABLE with only ADD COLUMN, only DROP COLUMN, or only ALTER PK can use declarative schema changer.
+statement ok
+EXPLAIN (DDL) ALTER TABLE t_108870 DROP COLUMN j;
+
+statement ok
+EXPLAIN (DDL) ALTER TABLE t_108870 ADD COLUMN k INT DEFAULT 40;
+
+statement ok
+EXPLAIN (DDL) ALTER TABLE t_108870 ADD COLUMN k INT DEFAULT 40, ADD COLUMN p INT DEFAULT 50;
+
+statement ok
+EXPLAIN (DDL) ALTER TABLE t_108870 ALTER PRIMARY KEY USING COLUMNS (j);
+
+# Ensure ALTER TABLE with multiple ADD COLUMN, DROP COLUMN, and/or ALTER PK cannot use declarative schema changer.
+skipif config local-mixed-22.2-23.1
+statement error pgcode 0A000 cannot explain a statement which is not supported by the declarative schema changer
+EXPLAIN (DDL) ALTER TABLE t_108870 ALTER PRIMARY KEY USING COLUMNS (j), DROP COLUMN i;
+
+skipif config local-mixed-22.2-23.1
+statement error pgcode 0A000 cannot explain a statement which is not supported by the declarative schema changer
+EXPLAIN (DDL) ALTER TABLE t_108870 DROP COLUMN j, ADD COLUMN k INT DEFAULT 30;
+
+skipif config local-mixed-22.2-23.1
+statement error pgcode 0A000 cannot explain a statement which is not supported by the declarative schema changer
+EXPLAIN (DDL) ALTER TABLE t_108870 ADD COLUMN k INT DEFAULT 30, ALTER PRIMARY KEY USING COLUMNS (j);
+
+skipif config local-mixed-22.2-23.1
+statement error pgcode 0A000 cannot explain a statement which is not supported by the declarative schema changer
+EXPLAIN (DDL) ALTER TABLE t_108870 ALTER PRIMARY KEY USING COLUMNS (j), DROP COLUMN i, ADD COLUMN k INT DEFAULT 30;
+
+# Ensure we can enable it in declarative schema changer with cluster setting
+# sql.schema.force_declarative_statements.
+skipif config local-mixed-22.2-23.1
+statement ok
+SET CLUSTER SETTING sql.schema.force_declarative_statements='+ALTER TABLE';
+
+statement ok
+SET use_declarative_schema_changer = 'on';
+
+skipif config local-mixed-22.2-23.1
+statement ok
+EXPLAIN (DDL) ALTER TABLE t_108870 ALTER PRIMARY KEY USING COLUMNS (j), DROP COLUMN i;
+
+skipif config local-mixed-22.2-23.1
+statement ok
+EXPLAIN (DDL) ALTER TABLE t_108870 DROP COLUMN j, ADD COLUMN k INT DEFAULT 30;
+
+skipif config local-mixed-22.2-23.1
+statement ok
+EXPLAIN (DDL) ALTER TABLE t_108870 ADD COLUMN k INT DEFAULT 30, ALTER PRIMARY KEY USING COLUMNS (j);
+
+skipif config local-mixed-22.2-23.1
+statement ok
+EXPLAIN (DDL) ALTER TABLE t_108870 ALTER PRIMARY KEY USING COLUMNS (j), DROP COLUMN i, ADD COLUMN k INT DEFAULT 30;
+
+# Ensure we can also enable it in declarative schema changer with session variable
+# use_declarative_schema_changer.
+skipif config local-mixed-22.2-23.1
+statement ok
+SET CLUSTER SETTING sql.schema.force_declarative_statements='';
+
+statement ok
+SET use_declarative_schema_changer = 'unsafe_always';
+
+skipif config local-mixed-22.2-23.1
+statement ok
+EXPLAIN (DDL) ALTER TABLE t_108870 ALTER PRIMARY KEY USING COLUMNS (j), DROP COLUMN i;
+
+skipif config local-mixed-22.2-23.1
+statement ok
+EXPLAIN (DDL) ALTER TABLE t_108870 DROP COLUMN j, ADD COLUMN k INT DEFAULT 30;
+
+skipif config local-mixed-22.2-23.1
+statement ok
+EXPLAIN (DDL) ALTER TABLE t_108870 ADD COLUMN k INT DEFAULT 30, ALTER PRIMARY KEY USING COLUMNS (j);
+
+skipif config local-mixed-22.2-23.1
+statement ok
+EXPLAIN (DDL) ALTER TABLE t_108870 ALTER PRIMARY KEY USING COLUMNS (j), DROP COLUMN i, ADD COLUMN k INT DEFAULT 30;
+
+subtest end

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go

@@ -13,12 +13,15 @@ package scbuildstmt
 import (
 	"math"
 	"reflect"
+	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
+	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/screl"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
@@ -182,6 +185,52 @@ func AlterTable(b BuildCtx, n *tree.AlterTable) {
 	maybeDropRedundantPrimaryIndexes(b, tbl.TableID)
 	maybeRewriteTempIDsInPrimaryIndexes(b, tbl.TableID)
 	disallowDroppingPrimaryIndexReferencedInUDFOrView(b, tbl.TableID, n.String())
+	// TODO (xiang): Remove the following line for the next major release after v23.2,
+	// be it v24.1 or v23.3.
+	disableAlterTableMultipleCommandsOnV232(b, n, tbl.TableID)
+}
+
+// disableAlterTableMultipleCommandsOnV232 disables declarative schema changer
+// if processing this ALTER TABLE stmt requires building more than one new
+// primary indexes by default on v23.2, unless the mode is unsafe or ALTER TABLE
+// is forcefully turned on.
+func disableAlterTableMultipleCommandsOnV232(b BuildCtx, n *tree.AlterTable, tableID catid.DescID) {
+	chain := getPrimaryIndexChain(b, tableID)
+	chainTyp := chain.chainType()
+
+	// isAlterPKWithRowid returns true if the stmt is ALTER PK with rowid.
+	isAlterPKWithRowid := func() bool {
+		if chainTyp == twoNewPrimaryIndexesWithAlteredPKType {
+			_, _, inter2StoredCols := getSortedColumnIDsInIndexByKind(b, tableID, chain.inter2Spec.primary.IndexID)
+			_, _, finalStoredCols := getSortedColumnIDsInIndexByKind(b, tableID, chain.finalSpec.primary.IndexID)
+			inter2StoredColsAsSet := catalog.MakeTableColSet(inter2StoredCols...)
+			finalStoredColsAsSet := catalog.MakeTableColSet(finalStoredCols...)
+			diffSet := inter2StoredColsAsSet.Difference(finalStoredColsAsSet)
+			if diffSet.Len() == 1 {
+				colName := mustRetrieveColumnNameElem(b, tableID, diffSet.Ordered()[0]).Name
+				if strings.HasPrefix(colName, "rowid") {
+					return true
+				}
+			}
+		}
+		return false
+	}
+
+	if chainTyp == twoNewPrimaryIndexesWithAlteredPKType ||
+		chainTyp == twoNewPrimaryIndexesWithAddAndDropColumnsType ||
+		chainTyp == threeNewPrimaryIndexesType {
+		if isAlterPKWithRowid() {
+			// This is the only exception that involves building more than one new
+			// primary indexes but we would enable by default, because it was already
+			// supported in v23.1.
+			return
+		}
+		newSchemaChangerMode := getDeclarativeSchemaChangerModeForStmt(b, n)
+		if newSchemaChangerMode != sessiondatapb.UseNewSchemaChangerUnsafe &&
+			newSchemaChangerMode != sessiondatapb.UseNewSchemaChangerUnsafeAlways {
+			panic(scerrors.NotImplementedErrorf(n, "statement has been disabled on v23.2"))
+		}
+	}
 }
 
 // disallowDroppingPrimaryIndexReferencedInUDFOrView prevents dropping old (current)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go

@@ -1506,7 +1506,7 @@ const (
 // A set of five pre-defined acceptable types for primary index chains:
 // 1). noNewPrimaryIndex: "old, nil, nil, nil" (e.g. no add/drop column nor alter PK)
 // 2). oneNewPrimaryIndex: "old, nil, nil, final" (e.g. add column(s), or drop columns(s), or alter PK without rowid)
-// 3). twoNewPrimaryIndexesWithAlteredPK: "old, nil, inter2, final" (e.g. alter PK with rowid)
+// 3). twoNewPrimaryIndexesWithAlteredPK: "old, nil, inter2, final" (e.g. alter PK with rowid, or alter PK + drop column(s))
 // 4). twoNewPrimaryIndexesWithAddAndDropColumns: "old, inter1, nil, final" (e.g. add & drop column(s))
 // 5). threeNewPrimaryIndexes: "old, inter1, inter2, final" (e.g. add/drop column + alter PK)
 type chainType int

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go

@@ -166,13 +166,7 @@ func isFullySupportedWithFalsePositiveInternal(
 // Process dispatches on the statement type to populate the BuilderState
 // embedded in the BuildCtx. Any error will be panicked.
 func Process(b BuildCtx, n tree.Statement) {
-	newSchemaChangerMode := b.EvalCtx().SessionData().NewSchemaChangerMode
-	// Check if the feature is either forced disabled or enabled,
-	// using a cluster setting.
-	disabledStatements := getSchemaChangerStatementControl(&b.ClusterSettings().SV)
-	if forcedEnabled := disabledStatements.CheckStatementControl(n); forcedEnabled {
-		newSchemaChangerMode = sessiondatapb.UseNewSchemaChangerUnsafe
-	}
+	newSchemaChangerMode := getDeclarativeSchemaChangerModeForStmt(b, n)
 	// Run a few "quick checks" to see if the statement is not supported.
 	if !IsFullySupportedWithFalsePositive(n, b.EvalCtx().Settings.Version.ActiveVersion(b),
 		newSchemaChangerMode) {
@@ -191,6 +185,24 @@ func Process(b BuildCtx, n tree.Statement) {
 	fn.Call(in)
 }
 
+// getDeclarativeSchemaChangerModeForStmt returns the mode specific for `n`.
+// It almost always returns value of session variable
+// `use_declarative_schema_changer`, unless `n` is forcefully enabled (or
+// disabled) via cluster setting `sql.schema.force_declarative_statements`, in
+// which case it returns `unsafe` (or `off`).
+func getDeclarativeSchemaChangerModeForStmt(
+	b BuildCtx, n tree.Statement,
+) sessiondatapb.NewSchemaChangerMode {
+	ret := b.EvalCtx().SessionData().NewSchemaChangerMode
+	// Check if the feature is either forcefully enabled or disabled, via a
+	// cluster setting.
+	stmtsForceControl := getStatementsForceControl(&b.ClusterSettings().SV)
+	if forcedEnabled := stmtsForceControl.CheckControl(n); forcedEnabled {
+		ret = sessiondatapb.UseNewSchemaChangerUnsafe
+	}
+	return ret
+}
+
 var isV221Active = func(_ tree.NodeFormatter, _ sessiondatapb.NewSchemaChangerMode, activeVersion clusterversion.ClusterVersion) bool {
 	return activeVersion.IsActive(clusterversion.TODODelete_V22_1)
 }

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process_test.go

@@ -22,7 +22,7 @@ import (
 func TestSupportedStatements(t *testing.T) {
 	sv := &settings.Values{}
 	// Non-existent tags should error out.
-	require.Error(t, schemaChangerDisabledStatements.Validate(sv, "FAKE STATEMENT"))
+	require.Error(t, forceDeclarativeStatements.Validate(sv, "FAKE STATEMENT"))
 	// Generate the full set of statements
 	allTags := strings.Builder{}
 	noTags := strings.Builder{}
@@ -35,8 +35,8 @@ func TestSupportedStatements(t *testing.T) {
 		ret := typTag.Func.Call([]reflect.Value{reflect.New(typ.Elem())})
 		require.Equal(t, ret[0].String(), stmt.statementTag, "statement tag is different in AST")
 		// Validate all tags are supported.
-		require.NoError(t, schemaChangerDisabledStatements.Validate(sv, "+"+stmt.statementTag))
-		require.NoError(t, schemaChangerDisabledStatements.Validate(sv, "!"+stmt.statementTag))
+		require.NoError(t, forceDeclarativeStatements.Validate(sv, "+"+stmt.statementTag))
+		require.NoError(t, forceDeclarativeStatements.Validate(sv, "!"+stmt.statementTag))
 		// Validate all of them can be specified at once.
 		if !first {
 			allTags.WriteString(",")
@@ -48,6 +48,6 @@ func TestSupportedStatements(t *testing.T) {
 		noTags.WriteString("!")
 		noTags.WriteString(stmt.statementTag)
 	}
-	require.NoError(t, schemaChangerDisabledStatements.Validate(sv, allTags.String()))
-	require.NoError(t, schemaChangerDisabledStatements.Validate(sv, noTags.String()))
+	require.NoError(t, forceDeclarativeStatements.Validate(sv, allTags.String()))
+	require.NoError(t, forceDeclarativeStatements.Validate(sv, noTags.String()))
 }

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/statement_control.go

@@ -19,18 +19,21 @@ import (
 	"github.com/cockroachdb/errors"
 )
 
-// schemaStatementControl track if a statement tag is enabled or disabled
-// forcefully by the user.
-type schemaStatementControl map[string]bool
+// statementsForceControl track if a statement tag is enabled or disabled
+// forcefully by the user to use declarative schema changer.
+type statementsForceControl map[string]bool
 
-// schemaChangerDisabledStatements statements which are disabled
-// for the declarative schema changer. Users can specify statement
-// tags for each statement and a "!" symbol in front can have the opposite
-// effect to force enable fully unimplemented features.
-var schemaChangerDisabledStatements = settings.RegisterStringSetting(
+// forceDeclarativeStatements outlines statements which are forcefully enabled
+// and/or disabled with declarative schema changer, separated by comma.
+// Forcefully enabled statements are prefixed with "+";
+// Forcefully disabled statements are prefixed with "!";
+// E.g. `SET CLUSTER SETTING sql.schema.force_declarative_statements = "+ALTER TABLE,!CREATE SEQUENCE";`
+//
+// Note: We can only control statements implemented in declarative schema changer.
+var forceDeclarativeStatements = settings.RegisterStringSetting(
 	settings.TenantWritable,
 	"sql.schema.force_declarative_statements",
-	"allows force enabling / disabling declarative schema changer for specific statements",
+	"forcefully enable or disable declarative schema changer for specific statements",
 	"",
 	settings.WithValidateString(func(values *settings.Values, s string) error {
 		if s == "" {
@@ -52,10 +55,10 @@ var schemaChangerDisabledStatements = settings.RegisterStringSetting(
 		return nil
 	}))
 
-// CheckStatementControl if a statement is forced to disabled or enabled. If a
-// statement is disabled then an not implemented error will be panicked. Otherwise,
-// a flag is returned indicating if this statement has been *forced* to be enabled.
-func (c schemaStatementControl) CheckStatementControl(n tree.Statement) (forceEnabled bool) {
+// CheckControl checks if a statement is forced to be enabled or disabled. If
+// `n` is forcefully disabled, then a "NotImplemented" error will be panicked.
+// Otherwise, return whether `n` is forcefully enabled.
+func (c statementsForceControl) CheckControl(n tree.Statement) (forceEnabled bool) {
 	// This map is only created *if* any force flags are set.
 	if c == nil {
 		return false
@@ -74,9 +77,9 @@ func (c schemaStatementControl) CheckStatementControl(n tree.Statement) (forceEn
 // GetSchemaChangerStatementControl returns a map of statements that
 // are explicitly disabled by administrators for the declarative schema
 // changer.
-func getSchemaChangerStatementControl(sv *settings.Values) schemaStatementControl {
-	statements := schemaChangerDisabledStatements.Get(sv)
-	var statementMap schemaStatementControl
+func getStatementsForceControl(sv *settings.Values) statementsForceControl {
+	statements := forceDeclarativeStatements.Get(sv)
+	var statementMap statementsForceControl
 	for _, tag := range strings.Split(statements, ",") {
 		tag = strings.ToUpper(strings.TrimSpace(tag))
 		if len(tag) == 0 {
@@ -90,7 +93,7 @@ func getSchemaChangerStatementControl(sv *settings.Values) schemaStatementContro
 			enabledOrDisabled = false
 		}
 		if statementMap == nil {
-			statementMap = make(schemaStatementControl)
+			statementMap = make(statementsForceControl)
 		}
 		statementMap[tag] = enabledOrDisabled
 	}

pkg/sql/schemachanger/sctest/end_to_end.go

@@ -107,23 +107,20 @@ func EndToEndSideEffects(t *testing.T, relTestCaseDir string, factory TestServer
 				// for end-to-end side-effect testing, so we ignore them.
 				break
 			case "test":
-				stmts, execStmts := parseStmts()
+				stmts, _ := parseStmts()
 				require.Lessf(t, numTestStatementsObserved, 1, "only one test per-file.")
 				numTestStatementsObserved++
 				stmtSqls := make([]string, 0, len(stmts))
 				for _, stmt := range stmts {
 					stmtSqls = append(stmtSqls, stmt.SQL)
 				}
-				// Keep test cluster in sync.
-				defer execStmts()
 
-				// Wait for any jobs due to previous schema changes to finish.
-				sctestdeps.WaitForNoRunningSchemaChanges(t, tdb)
-				var deps *sctestdeps.TestState
 				// Create test dependencies and execute the schema changer.
 				// The schema changer test dependencies do not hold any reference to the
 				// test cluster, here the SQLRunner is only used to populate the mocked
 				// catalog state.
+				// It is declared here because it's used in its initialization below.
+				var deps *sctestdeps.TestState
 				// Set up a reference provider factory for the purpose of proper
 				// dependency resolution.
 				execCfg := s.ExecutorConfig().(sql.ExecutorConfig)