From a8d2f077cd38b9ad983c0ba78d75057678c3bdd6 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Tue, 5 May 2026 11:03:03 -0400 Subject: [PATCH] schemachanger: allow dropping column referenced in partial index predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The declarative schema changer previously rejected ALTER TABLE ... DROP COLUMN if the column was referenced in a partial index predicate, with a hint to drop the index first. The restriction was added in #97372 as a temporary safeguard for #96924, where the optimizer could need to read a column being dropped to evaluate the predicate when both the column and its dependent partial index were transitioning simultaneously. The dep rule "secondary index partial no longer public before referenced column" (added later) sequences the partial index into DELETE_ONLY before the column reaches WRITE_ONLY. In DELETE_ONLY the index issues blind DELs and does not need to evaluate its predicate, so the original hazard no longer applies for the declarative path. This commit lifts the restriction in the declarative DROP COLUMN path only. The legacy schema changer keeps the restriction (it lacks the dep-rule machinery), and partial unique-without-index predicates remain blocked in both schema changers — those will be addressed separately. ALTER PRIMARY KEY and ALTER COLUMN TYPE also keep the restriction, because their column-drop traversal can encounter element types that the existing walker does not yet handle. The restriction toggle on walkColumnDependencies changes from a bool to a small predicateRefPolicy enum (allow all / disallow UWI only / disallow all) so each caller can pick the policy that matches its actual constraints. The DML-injection test case "drop column with partial index" was re-enabled (previously skipped under #97813). Resolves: #97813 Epic: none Release note (sql change): ALTER TABLE ... DROP COLUMN no longer rejects columns referenced in a partial index predicate. The dependent partial index is dropped automatically as part of the same statement. The previous behavior — requiring the user to drop the index first — is still in effect when the legacy schema changer is in use, and when the column is referenced in a partial UNIQUE WITHOUT INDEX predicate. Co-Authored-By: roachdev-claude --- .../testdata/logic_test/partial_index | 44 ++++++++--- pkg/sql/schemachanger/dml_injection_test.go | 1 - .../alter_table_alter_column_type.go | 6 +- .../alter_table_alter_primary_key.go | 2 +- .../scbuildstmt/alter_table_drop_column.go | 77 +++++++++++++++++-- .../scbuildstmt/alter_table_rename_column.go | 2 +- 6 files changed, 108 insertions(+), 24 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/partial_index b/pkg/sql/logictest/testdata/logic_test/partial_index index 6094b90fcf70..8f638c6bf998 100644 --- a/pkg/sql/logictest/testdata/logic_test/partial_index +++ b/pkg/sql/logictest/testdata/logic_test/partial_index @@ -273,7 +273,9 @@ t7 CREATE TABLE public.t7 ( INDEX t6i3 (b DESC) WHERE b > 8:::INT8 ) WITH (schema_locked = true); -# Dropping a column referenced in the predicate drops the index. +# Dropping a column referenced in the predicate drops the index. The +# declarative schema changer handles this directly; the legacy schema +# changer still requires explicitly dropping the index first (see #97813). statement ok CREATE TABLE t8 ( @@ -285,10 +287,17 @@ CREATE TABLE t8 ( FAMILY (a, b, c) ) -# TODO(mgartner): Lift this restriction. See #96924. +onlyif config local-legacy-schema-changer statement error cannot drop column "c" because it is referenced by partial index "t8_a_idx1"\nHINT: drop the partial index first, then drop the column ALTER TABLE t8 DROP COLUMN c +onlyif config local-legacy-schema-changer +statement ok +DROP INDEX t8_a_idx1 + +statement ok +ALTER TABLE t8 DROP COLUMN c + onlyif config schema-locked-disabled query TT SHOW CREATE TABLE t8 @@ -296,12 +305,10 @@ SHOW CREATE TABLE t8 t8 CREATE TABLE public.t8 ( a INT8 NULL, b INT8 NULL, - c STRING NULL, rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(), CONSTRAINT t8_pkey PRIMARY KEY (rowid ASC), INDEX t8_a_idx (a ASC) WHERE b > 0:::INT8, - INDEX t8_a_idx1 (a ASC) WHERE c = 'foo':::STRING, - FAMILY fam_0_a_b_c_rowid (a, b, c, rowid) + FAMILY fam_0_a_b_c_rowid (a, b, rowid) ); skipif config schema-locked-disabled @@ -311,12 +318,10 @@ SHOW CREATE TABLE t8 t8 CREATE TABLE public.t8 ( a INT8 NULL, b INT8 NULL, - c STRING NULL, rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(), CONSTRAINT t8_pkey PRIMARY KEY (rowid ASC), INDEX t8_a_idx (a ASC) WHERE b > 0:::INT8, - INDEX t8_a_idx1 (a ASC) WHERE c = 'foo':::STRING, - FAMILY fam_0_a_b_c_rowid (a, b, c, rowid) + FAMILY fam_0_a_b_c_rowid (a, b, rowid) ) WITH (schema_locked = true); # CREATE TABLE LIKE ... INCLUDING INDEXES copies partial index predicate @@ -2368,24 +2373,34 @@ CREATE TABLE t96924 ( statement ok ALTER TABLE t96924 DROP COLUMN a +# Under the declarative schema changer, dropping a column referenced in a +# partial index predicate drops the dependent index in the same statement. +# The legacy schema changer still requires the user to drop the index first. + +onlyif config local-legacy-schema-changer statement error pq: cannot drop column "b" because it is referenced by partial index "t96924_b_key"\nHINT: drop the partial index first, then drop the column ALTER TABLE t96924 DROP COLUMN b +onlyif config local-legacy-schema-changer statement ok DROP INDEX t96924_b_key CASCADE statement ok ALTER TABLE t96924 DROP COLUMN b +onlyif config local-legacy-schema-changer statement error pq: cannot drop column "c" because it is referenced by partial index "t96924_c_key"\nHINT: drop the partial index first, then drop the column ALTER TABLE t96924 DROP COLUMN c +onlyif config local-legacy-schema-changer statement ok DROP INDEX t96924_c_key CASCADE statement ok ALTER TABLE t96924 DROP COLUMN c +# Partial unique-without-index predicates still block the column drop in +# both schema changers (this restriction will be lifted separately). statement error pq: cannot drop column "d" because it is referenced by partial unique constraint "unique_d"\nHINT: drop the unique constraint first, then drop the column ALTER TABLE t96924 DROP COLUMN d @@ -2395,9 +2410,11 @@ ALTER TABLE t96924 DROP CONSTRAINT unique_d statement ok ALTER TABLE t96924 DROP COLUMN d +onlyif config local-legacy-schema-changer statement error pq: cannot drop column "e" because it is referenced by partial index "t96924_e_f_idx"\nHINT: drop the partial index first, then drop the column ALTER TABLE t96924 DROP COLUMN e +onlyif config local-legacy-schema-changer statement ok DROP INDEX t96924_e_f_idx CASCADE @@ -2409,19 +2426,22 @@ ALTER TABLE t96924 DROP COLUMN e statement ok ALTER TABLE t96924 DROP COLUMN f -# Column `h` is used in the predicate of another index that does not key on `h`, -# we will prevent dropping column `h` as well. +# Column `h` is referenced only in the predicate of another index that does +# not key on `h`. The declarative schema changer drops both atomically; the +# legacy schema changer requires dropping the index first. +onlyif config local-legacy-schema-changer statement error pq: cannot drop column "h" because it is referenced by partial index "t96924_g_idx"\nHINT: drop the partial index first, then drop the column ALTER TABLE t96924 DROP COLUMN h +onlyif config local-legacy-schema-changer statement ok DROP INDEX t96924_g_idx statement ok ALTER TABLE t96924 DROP COLUMN h -# Similarly, column `i` cannot be dropped since it's used in the predicate of -# of a UWI constraint. +# Column `i` is used in the predicate of a UWI constraint, which still +# blocks the drop in both schema changers. statement error pq: cannot drop column "i" because it is referenced by partial unique constraint "unique_g"\nHINT: drop the unique constraint first, then drop the column ALTER TABLE t96924 DROP COLUMN i diff --git a/pkg/sql/schemachanger/dml_injection_test.go b/pkg/sql/schemachanger/dml_injection_test.go index d534fc33ee22..72ef506d1395 100644 --- a/pkg/sql/schemachanger/dml_injection_test.go +++ b/pkg/sql/schemachanger/dml_injection_test.go @@ -543,7 +543,6 @@ func TestAlterTableDMLInjection(t *testing.T) { "CREATE INDEX idx ON tbl (val) WHERE i > 1", }, schemaChange: "ALTER TABLE tbl DROP COLUMN i", - skipIssue: 97813, }, { desc: "create expression index", diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_type.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_type.go index be87fe35feb3..eb36e297bba1 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_type.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_type.go @@ -89,7 +89,7 @@ func alterTableAlterColumnType( case *scpb.PolicyUsingExpr, *scpb.PolicyWithCheckExpr: panic(sqlerrors.NewAlterDependsOnPolicyExprError(op, objType, t.Column.String())) } - }, false /* allowPartialIdxPredicateRef */) + }, disallowAllPredicateRefs) var err error newColType.Type, err = schemachange.ValidateAlterColumnTypeChecks( @@ -304,7 +304,7 @@ func handleGeneralColumnConversion( case *scpb.SecondaryIndex: panic(sqlerrors.NewAlterColumnTypeColInIndexNotSupportedErr()) } - }, false /* allowPartialIdxPredicateRef */) + }, disallowAllPredicateRefs) // This code path should never be reached for virtual columns, as their values // are always computed dynamically on access and are never stored on disk. @@ -479,7 +479,7 @@ func maybeWriteNoticeForFKColTypeMismatch(b BuildCtx, col *scpb.Column, colType case *scpb.ForeignKeyConstraintUnvalidated: writeNoticeHelper(e.ColumnIDs, e.ReferencedColumnIDs, e.ReferencedTableID) } - }, false /* allowPartialIdxPredicateRef */) + }, disallowAllPredicateRefs) } func getComputeExpressionForBackfill( diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go index 063bd9f6bfae..482a441c4188 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go @@ -1376,7 +1376,7 @@ func checkIfColumnCanBeDropped(b BuildCtx, columnToDrop *scpb.Column) bool { canBeDropped = false } } - }, false /* allowPartialIdxPredicateRef */) + }, disallowAllPredicateRefs) return canBeDropped } diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go index 811877b86711..7d24788b1dc3 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go @@ -309,7 +309,7 @@ func dropColumn( default: b.Drop(e) } - }, false /* allowPartialIdxPredicateRef */) + }, disallowUWIPredicateRefs) // TODO(ajwerner): Track the undropped backrefs to populate a detail // message like postgres does. For example: // SET serial_normalization = sql_sequence; @@ -333,13 +333,35 @@ func dropColumn( assertAllColumnElementsAreDropped(colElts) } +// predicateRefPolicy controls how walkColumnDependencies handles columns +// that are referenced in a partial-index predicate or partial +// unique-without-index predicate. +type predicateRefPolicy int + +const ( + // disallowAllPredicateRefs panics if the column is referenced in either + // a partial-index predicate or a partial UWI predicate. Used by code + // paths (ALTER PRIMARY KEY, ALTER COLUMN TYPE) where the dependency + // rules don't yet sequence the partial-index drop safely. + disallowAllPredicateRefs predicateRefPolicy = iota + // disallowUWIPredicateRefs panics only for partial UWI references. + // Partial-index references are allowed because the declarative schema + // changer's dep rule sequences the partial index into DELETE_ONLY + // before the column reaches WRITE_ONLY, allowing the index entries to + // be removed via blind DELs without evaluating the predicate. + disallowUWIPredicateRefs + // allowAllPredicateRefs skips the check entirely. Used by ALTER + // COLUMN ... RENAME, which doesn't change column existence. + allowAllPredicateRefs +) + func walkColumnDependencies( b BuildCtx, col *scpb.Column, op string, objType string, fn func(e scpb.Element, op string, objType string), - allowPartialIdxPredicateRef bool, + predicateRefs predicateRefPolicy, ) { var sequenceDeps catalog.DescriptorIDSet var indexDeps catid.IndexSet @@ -347,11 +369,13 @@ func walkColumnDependencies( var constraintDeps catid.ConstraintSet tblElts := b.QueryByID(col.TableID).Filter(orFilter(publicTargetFilter, transientTargetFilter)) - // Panic if `col` is referenced in a predicate of an index or - // unique without index constraint. - // TODO (xiang): Remove this restriction when #97813 is fixed. - if !allowPartialIdxPredicateRef { + switch predicateRefs { + case disallowAllPredicateRefs: panicIfColReferencedInPredicate(b, col, tblElts, op, objType) + case disallowUWIPredicateRefs: + panicIfColReferencedInUWIPredicate(b, col, tblElts, op, objType) + case allowAllPredicateRefs: + // No-op. } tblElts. @@ -525,6 +549,47 @@ func panicIfColReferencedInPredicate( } } +// panicIfColReferencedInUWIPredicate disallows dropping a column that is +// referenced in the predicate of a partial unique-without-index constraint. +// Unlike partial indexes, UWI predicate evaluation during the column-drop +// transition is not yet sequenced safely by the declarative schema changer's +// dependency rules. +func panicIfColReferencedInUWIPredicate( + b BuildCtx, col *scpb.Column, tblElts ElementResultSet, op, objType string, +) { + contains := func(container []catid.ColumnID, target catid.ColumnID) bool { + for _, elem := range container { + if elem == target { + return true + } + } + return false + } + + var violatingUWI catid.ConstraintID + tblElts.ForEach(func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) { + if violatingUWI != 0 { + return + } + switch elt := e.(type) { + case *scpb.UniqueWithoutIndexConstraint: + if elt.Predicate != nil && contains(elt.Predicate.ReferencedColumnIDs, col.ColumnID) { + violatingUWI = elt.ConstraintID + } + case *scpb.UniqueWithoutIndexConstraintUnvalidated: + if elt.Predicate != nil && contains(elt.Predicate.ReferencedColumnIDs, col.ColumnID) { + violatingUWI = elt.ConstraintID + } + } + }) + if violatingUWI != 0 { + colNameElem := mustRetrieveColumnNameElem(b, col.TableID, col.ColumnID) + uwiNameElem := mustRetrieveConstraintWithoutIndexNameElem(b, col.TableID, violatingUWI) + panic(sqlerrors.ColumnReferencedByPartialUniqueWithoutIndexConstraint( + op, objType, colNameElem.Name, uwiNameElem.Name)) + } +} + func handleDropColumnPrimaryIndexes(b BuildCtx, tbl *scpb.Table, col *scpb.Column) { inflatedChain := getInflatedPrimaryIndexChain(b, tbl.TableID) diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_rename_column.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_rename_column.go index 5292c224a7d7..dbe24202f7f0 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_rename_column.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_rename_column.go @@ -95,7 +95,7 @@ func renameColumnChecks(b BuildCtx, col *scpb.Column, oldName, newName tree.Name tree.ErrString(&oldName), fnName.Name), ) } - }, true /* allowPartialIdxPredicateRef */) + }, allowAllPredicateRefs) if col.IsInaccessible { panic(pgerror.Newf(