schemachanger: Use legacy schema changer for ALTER TABLE with multipl…

…e commands by default

This commits disables ALTER TABLE with multiple ADD COLUMN, DROP COLUMN,
and/or ALTER PK commands with declarative schema changer by default.
Customers can turn it on by setting the mode to be "unsafe" or "unsafe_always",
and/or forcefully turn on declarative schema changer for ALTER TABLE
stmt. We do this because there may not be sufficient testing so we are
at a risk of introducing regression. We'd like to turn it back on again
by default in the next major release.

Release note: None

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 forcefully enabled or disabled, via a
-	// cluster setting.
-	stmtsForceControl := getStatementsForceControl(&b.ClusterSettings().SV)
-	if forcedEnabled := stmtsForceControl.CheckControl(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)
 }