Merge #143273

143273: sql/schemachanger: set schema_locked automatically for declarative r=fqazi a=fqazi

Previously, the schema_locked setting had to be manually toggled by the user before executing DDL on a table. This meant that setting / unsetting schema_locked was a manual process for users. The declarative schema changer makes it much easier to tie setting / unsetting the locked setting within the schema change path. This patch adds a new TRANSIENT_PUBLIC state that allows elements to be toggled. The builder is modified to emit this schema_locked element in a TRANSIENT_PUBLIC state.

Informs: #129694
Release note: None

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

Author: craig[bot]

pkg/ccl/schemachangerccl/backup_base_generated_test.go

@@ -45,7 +45,7 @@ func validateSchemaChangerState(d catalog.Descriptor, vea catalog.ValidationErro
 				i, got, d.GetID()))
 		}
 		switch t.TargetStatus {
-		case scpb.Status_PUBLIC, scpb.Status_ABSENT, scpb.Status_TRANSIENT_ABSENT:
+		case scpb.Status_PUBLIC, scpb.Status_ABSENT, scpb.Status_TRANSIENT_ABSENT, scpb.Status_TRANSIENT_PUBLIC:
 			// These are valid target status values.
 		default:
 			report(errors.Errorf("target %d is targeting an invalid status %s",

pkg/sql/catalog/internal/validate/schema_changer_state.go

@@ -83,34 +83,43 @@ CREATE TABLE t (i INT PRIMARY KEY, j INT, UNIQUE INDEX idx (j)) WITH (schema_loc
 statement ok
 INSERT INTO t SELECT i, i+1 FROM generate_series(1,10) AS tmp(i);
 
+onlyif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
 statement error pgcode 57000 schema changes are disallowed on table "t" because it is locked
 ALTER TABLE t ADD COLUMN k INT DEFAULT 30;
 
+onlyif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
 statement error pgcode 57000 schema changes are disallowed on table "t" because it is locked
 ALTER TABLE t DROP COLUMN j;
 
+onlyif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
 statement error pgcode 57000 schema changes are disallowed on table "t" because it is locked
 ALTER TABLE t RENAME TO t2;
 
+onlyif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
 statement error pgcode 57000 schema changes are disallowed on table "t" because it is locked
 ALTER TABLE t RENAME COLUMN j TO j2;
 
+onlyif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
 statement error pgcode 57000 schema changes are disallowed on table "t" because it is locked
 ALTER INDEX idx RENAME TO idx2;
 
+onlyif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
 statement error pgcode 57000 schema changes are disallowed on table "t" because it is locked
 ALTER INDEX idx INVISIBLE;
 
+onlyif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
 statement error pgcode 57000 schema changes are disallowed on table "t" because it is locked
 DROP INDEX idx;
 
+onlyif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
 statement error pgcode 57000 schema changes are disallowed on table "t" because it is locked
 CREATE INDEX idx2 ON t(j);
 
 statement ok
 CREATE TABLE ref (a INT PRIMARY KEY, b INT)
 
 # Locked tables cannot be referenced by foreign keys.
+onlyif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
 statement error pgcode 57000 schema changes are disallowed on table "t" because it is locked
 ALTER TABLE ref ADD CONSTRAINT fk FOREIGN KEY (b) REFERENCES t(j);
 
@@ -125,18 +134,52 @@ statement ok
 COMMENT ON TABLE t IS 't is a table';
 COMMENT ON INDEX t@idx IS 'idx is an index';
 COMMENT ON COLUMN t.i IS 'i is a column';
+subtest end
+
+# Declarative schema change can automatically unset and set schema locked.
+subtest declarative_allows_schema_changes
 
+skipif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
 statement ok
-ALTER TABLE t RESET (schema_locked)
+ALTER TABLE t ADD COLUMN k INT DEFAULT 30;
+
+skipif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
+statement ok
+CREATE INDEX idx2 ON t(j);
 
+skipif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
+statement ok
+DROP INDEX idx2;
+
+skipif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
+statement ok
+ALTER TABLE ref ADD CONSTRAINT fk FOREIGN KEY (b) REFERENCES t(j);
+
+skipif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
+statement ok
+ALTER TABLE t DROP COLUMN j CASCADE;
+
+# Confirm that the table definition still has schema_locked
+query I
+SELECT count(create_statement) FROM [SHOW CREATE TABLE t] WHERE create_statement LIKE '%schema_locked = true%'
+----
+1
+
+onlyif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
+statement ok
+ALTER TABLE t SET (schema_locked = false);
+
+# Finally drop the table.
 statement ok
 DROP TABLE t;
+subtest end
 
 subtest zone_config
 
 statement ok
 ALTER TABLE ref SET (schema_locked = true);
 
+onlyif config local-mixed-24.3 local-mixed-25.1 local-legacy-schema-changer
 statement error pgcode 57000 schema changes are disallowed on table "ref" because it is locked
 ALTER TABLE ref CONFIGURE ZONE USING num_replicas = 11;
 

pkg/sql/logictest/testdata/logic_test/schema_locked

@@ -341,7 +341,7 @@ func makeNameMappings(elementStates []elementState) (ret scpb.NameMappings) {
 		return &ret[idx], true /* isNew */
 	}
 	isNotDropping := func(ts scpb.TargetStatus) bool {
-		return ts != scpb.ToAbsent && ts != scpb.Transient
+		return ts != scpb.ToAbsent && ts != scpb.TransientAbsent
 	}
 	for _, es := range elementStates {
 		switch e := es.element.(type) {
@@ -447,7 +447,11 @@ func (b buildCtx) Add(element scpb.Element) {
 }
 
 func (b buildCtx) AddTransient(element scpb.Element) {
-	b.Ensure(element, scpb.Transient, b.TargetMetadata())
+	b.Ensure(element, scpb.TransientAbsent, b.TargetMetadata())
+}
+
+func (b buildCtx) DropTransient(element scpb.Element) {
+	b.Ensure(element, scpb.TransientPublic, b.TargetMetadata())
 }
 
 // Drop implements the scbuildstmt.BuildCtx interface.

pkg/sql/schemachanger/scbuild/build.go

@@ -62,7 +62,7 @@ func (b *builderState) QueryByID(id catid.DescID) scbuildstmt.ElementResultSet {
 func (b *builderState) Ensure(e scpb.Element, target scpb.TargetStatus, meta scpb.TargetMetadata) {
 	dst := b.getExistingElementState(e)
 	switch target {
-	case scpb.ToAbsent, scpb.ToPublic, scpb.Transient:
+	case scpb.ToAbsent, scpb.ToPublic, scpb.TransientAbsent, scpb.TransientPublic:
 		// Sanity check.
 	default:
 		panic(errors.AssertionFailedf("unsupported target %s", target.Status()))
@@ -108,7 +108,7 @@ func (b *builderState) Ensure(e scpb.Element, target scpb.TargetStatus, meta scp
 	// of it and investigate it further if needed.
 	if dst.current == scpb.Status_ABSENT &&
 		dst.target == scpb.ToAbsent &&
-		(target == scpb.ToPublic || target == scpb.Transient) &&
+		(target == scpb.ToPublic || target == scpb.TransientAbsent) &&
 		dst.metadata.IsLinkedToSchemaChange() {
 		panic(scerrors.NotImplementedErrorf(
 			nil,
@@ -156,7 +156,7 @@ func (b *builderState) Ensure(e scpb.Element, target scpb.TargetStatus, meta scp
 		// the transient path so it can be done but we must take care to migrate
 		// the current status to its transient equivalent if need be in order to
 		// avoid the possibility of a future transition to PUBLIC.
-		if target == scpb.Transient {
+		if target == scpb.TransientAbsent {
 			var ok bool
 			dst.current, ok = scpb.GetTransientEquivalent(dst.current)
 			if !ok {
@@ -167,7 +167,7 @@ func (b *builderState) Ensure(e scpb.Element, target scpb.TargetStatus, meta scp
 			}
 		}
 		return
-	case scpb.Transient:
+	case scpb.TransientAbsent:
 		// Here the new target is either to-absent, which effectively undoes the
 		// incumbent target, or it's to-public. In both cases if the current
 		// status is TRANSIENT_ we need to migrate it to its non-transient

pkg/sql/schemachanger/scbuild/builder_state.go

@@ -17,7 +17,7 @@ func AlterPolicy(b BuildCtx, n *tree.AlterPolicy) {
 	tableElems := b.ResolveTable(n.TableName, ResolveParams{
 		RequireOwnership: true,
 	})
-	panicIfSchemaChangeIsDisallowed(tableElems, n)
+	checkTableSchemaChangePrerequisites(b, tableElems, n)
 	tbl := tableElems.FilterTable().MustGetOneElement()
 
 	// Alter of a policy implies that it must exist.

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

@@ -123,7 +123,7 @@ func AlterTable(b BuildCtx, n *tree.AlterTable) {
 		panic(pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
 			"table %q is being dropped, try again later", n.Table.Object()))
 	}
-	panicIfSchemaChangeIsDisallowed(elts, n)
+	checkTableSchemaChangePrerequisites(b, elts, n)
 	tn.ObjectNamePrefix = b.NamePrefix(tbl)
 	b.SetUnresolvedNameAnnotation(n.Table, &tn)
 	b.IncrementSchemaChangeAlterCounter("table")

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

@@ -497,8 +497,8 @@ func addColumn(b BuildCtx, spec addColumnSpec, n tree.NodeFormatter) (backing *s
 			// recognize this and drop redundant primary indexes appropriately.
 			addStoredColumnToPrimaryIndexTargeting(b, spec.tbl.TableID, inflatedChain.oldSpec.primary, spec.col, scpb.ToPublic)
 		}
-		addStoredColumnToPrimaryIndexTargeting(b, spec.tbl.TableID, inflatedChain.inter1Spec.primary, spec.col, scpb.Transient)
-		addStoredColumnToPrimaryIndexTargeting(b, spec.tbl.TableID, inflatedChain.inter2Spec.primary, spec.col, scpb.Transient)
+		addStoredColumnToPrimaryIndexTargeting(b, spec.tbl.TableID, inflatedChain.inter1Spec.primary, spec.col, scpb.TransientAbsent)
+		addStoredColumnToPrimaryIndexTargeting(b, spec.tbl.TableID, inflatedChain.inter2Spec.primary, spec.col, scpb.TransientAbsent)
 		addStoredColumnToPrimaryIndexTargeting(b, spec.tbl.TableID, inflatedChain.finalSpec.primary, spec.col, scpb.ToPublic)
 		return inflatedChain.finalSpec.primary
 	}
@@ -521,8 +521,8 @@ func addColumn(b BuildCtx, spec addColumnSpec, n tree.NodeFormatter) (backing *s
 
 // addStoredColumnToPrimaryIndexTargeting adds a stored column `col` to primary
 // index `idx` and its associated temporary index.
-// The column in primary index is targeting `target` (either ToPublic or Transient),
-// and the column in its temporary index is always targeting Transient.
+// The column in primary index is targeting `target` (either ToPublic or TransientAbsent),
+// and the column in its temporary index is always targeting TransientAbsent.
 func addStoredColumnToPrimaryIndexTargeting(
 	b BuildCtx,
 	tableID catid.DescID,
@@ -531,7 +531,7 @@ func addStoredColumnToPrimaryIndexTargeting(
 	target scpb.TargetStatus,
 ) {
 	addIndexColumnToInternal(b, tableID, idx.IndexID, col.ColumnID, scpb.IndexColumn_STORED, target)
-	addIndexColumnToInternal(b, tableID, idx.TemporaryIndexID, col.ColumnID, scpb.IndexColumn_STORED, scpb.Transient)
+	addIndexColumnToInternal(b, tableID, idx.TemporaryIndexID, col.ColumnID, scpb.IndexColumn_STORED, scpb.TransientAbsent)
 }
 
 func addIndexColumnToInternal(
@@ -569,7 +569,7 @@ func addIndexColumnToInternal(
 	switch target {
 	case scpb.ToPublic:
 		b.Add(&indexCol)
-	case scpb.Transient:
+	case scpb.TransientAbsent:
 		b.AddTransient(&indexCol)
 	default:
 		panic(errors.AssertionFailedf("programming error: add index column element "+

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

@@ -281,7 +281,7 @@ func alterTableAddForeignKey(
 			"and is no longer supported."))
 	}
 	// Disallow schema change if the FK references a table whose schema is locked.
-	panicIfSchemaChangeIsDisallowed(b.QueryByID(referencedTableID), stmt)
+	checkTableSchemaChangePrerequisites(b, b.QueryByID(referencedTableID), stmt)
 
 	// 6. Check that temporary tables can only reference temporary tables, or,
 	// permanent tables can only reference permanent tables.

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

@@ -156,7 +156,7 @@ func astToZoneConfigObject(b BuildCtx, n *tree.SetZoneConfig) (zoneConfigObject,
 	}
 	tblName := zs.TableOrIndex.Table.ToUnresolvedObjectName()
 	elems := b.ResolvePhysicalTable(tblName, ResolveParams{})
-	panicIfSchemaChangeIsDisallowed(elems, n)
+	checkTableSchemaChangePrerequisites(b, elems, n)
 	var tableID catid.DescID
 	elems.ForEach(func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) {
 		switch e := e.(type) {