diff --git a/pkg/sql/logictest/testdata/logic_test/alter_table b/pkg/sql/logictest/testdata/logic_test/alter_table index af1a92dfdc92..0228047f5e2e 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_table +++ b/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 diff --git a/pkg/sql/logictest/testdata/logic_test/new_schema_changer b/pkg/sql/logictest/testdata/logic_test/new_schema_changer index c0a6653a51f4..4517d7e3c571 100644 --- a/pkg/sql/logictest/testdata/logic_test/new_schema_changer +++ b/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 diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go index c21c2bc25637..c9db64baf1b9 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go +++ b/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) diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go index a6558f087a45..fb6ec6a45278 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go +++ b/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 diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go index 2cbca600e22a..237bb967d30e 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go +++ b/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) }