Skip to content

Commit 5dcf331

Browse files
rafissrafiss
committed
sql/schemachanger: support ALTER TABLE DROP CONSTRAINT for UNIQUE constraints
Previously, attempting to drop a UNIQUE constraint using `ALTER TABLE ... DROP CONSTRAINT` would fail with an error directing users to use `DROP INDEX CASCADE` instead. This was inconvenient for users and broke compatibility with ORMs like Liquibase and Laravel that use the standard `ALTER TABLE DROP CONSTRAINT` syntax. This change implements support for dropping index-backed UNIQUE constraints via `ALTER TABLE DROP CONSTRAINT` in the declarative schema changer. The implementation reuses the existing `dropSecondaryIndex` logic which handles all the complex dependency management including: - Dependent views and functions - Dependent FK constraints (with CASCADE support) - Sharded and expression index cleanup - Trigger dependencies - Regional-by-row table validation The legacy schema changer still does not support this operation and will continue to direct users to use `DROP INDEX CASCADE`. Fixes #42840 Epic: CRDB-31281 Release note (sql change): `ALTER TABLE ... DROP CONSTRAINT` can now be used to drop UNIQUE constraints. The backing UNIQUE index will also be dropped, as CockroachDB treats the constraint and index as the same thing.
1 parent 52845e0 commit 5dcf331

6 files changed

Lines changed: 104 additions & 29 deletions

File tree

‎pkg/ccl/multiregionccl/regional_by_row_test.go‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -716,63 +716,81 @@ func TestRegionChangeRacingRegionalByRowChange(t *testing.T) {
716716
skip.UnderRace(t, "too slow under race (>10min)")
717717

718718
regionalByRowChanges := []struct {
719+
desc string
719720
setup string
720721
cmd string
721722
errorOnAddOrDropRegionSandwich string
722723
errorOnTableChangeSandwich string
723724
}{
724725
{
726+
desc: "set locality rbr",
725727
setup: `CREATE TABLE t.test (k INT NOT NULL, v INT) LOCALITY GLOBAL`,
726728
cmd: `ALTER TABLE t.test SET LOCALITY REGIONAL BY ROW`,
727729
errorOnAddOrDropRegionSandwich: "pq: cannot perform database region changes while a REGIONAL BY ROW transition is underway",
728730
errorOnTableChangeSandwich: "pq: cannot perform this locality change while a region is being added or dropped on the database",
729731
},
730732
{
733+
desc: "set locality global",
731734
setup: `CREATE TABLE t.test (k INT NOT NULL, v INT) LOCALITY REGIONAL BY ROW`,
732735
cmd: `ALTER TABLE t.test SET LOCALITY GLOBAL`,
733736
errorOnAddOrDropRegionSandwich: "pq: cannot perform database region changes while a REGIONAL BY ROW transition is underway",
734737
errorOnTableChangeSandwich: "pq: cannot perform this locality change while a region is being added or dropped on the database",
735738
},
736739
{
740+
desc: "create index",
737741
setup: `CREATE TABLE t.test (k INT NOT NULL, v INT) LOCALITY REGIONAL BY ROW`,
738742
cmd: `CREATE INDEX v_idx ON t.test(v)`,
739743
errorOnAddOrDropRegionSandwich: "pq: cannot perform database region changes while an index is being created or dropped on a REGIONAL BY ROW table",
740744
errorOnTableChangeSandwich: "pq: cannot CREATE INDEX on a REGIONAL BY ROW table while a region is being added or dropped on the database",
741745
},
742746
{
747+
desc: "drop index",
743748
setup: `CREATE TABLE t.test (k INT NOT NULL, v INT, INDEX v_idx (v)) LOCALITY REGIONAL BY ROW`,
744749
cmd: `DROP INDEX t.test@v_idx`,
745750
errorOnAddOrDropRegionSandwich: "pq: cannot perform database region changes while an index is being created or dropped on a REGIONAL BY ROW table",
746751
errorOnTableChangeSandwich: "pq: cannot DROP INDEX on a REGIONAL BY ROW table while a region is being added or dropped on the database",
747752
},
748753
{
754+
desc: "add unique constraint",
749755
setup: `CREATE TABLE t.test (k INT NOT NULL, v INT) LOCALITY REGIONAL BY ROW`,
750756
cmd: `ALTER TABLE t.test ADD CONSTRAINT v_uniq UNIQUE (v)`,
751757
errorOnAddOrDropRegionSandwich: "pq: cannot perform database region changes while an index is being created or dropped on a REGIONAL BY ROW table",
752758
errorOnTableChangeSandwich: "pq: cannot CREATE INDEX on a REGIONAL BY ROW table while a region is being added or dropped on the database",
753759
},
754760
{
761+
desc: "add unique column",
755762
setup: `CREATE TABLE t.test (k INT NOT NULL, v INT) LOCALITY REGIONAL BY ROW`,
756763
cmd: `ALTER TABLE t.test ADD COLUMN z INT UNIQUE`,
757764
errorOnAddOrDropRegionSandwich: "pq: cannot perform database region changes while an index is being created or dropped on a REGIONAL BY ROW table",
758765
errorOnTableChangeSandwich: "pq: cannot add a UNIQUE COLUMN on a REGIONAL BY ROW table while a region is being added or dropped on the database",
759766
},
760767
{
768+
desc: "alter primary key",
761769
setup: `CREATE TABLE t.test (k INT NOT NULL, v INT NOT NULL) LOCALITY REGIONAL BY ROW`,
762770
cmd: `ALTER TABLE t.test ALTER PRIMARY KEY USING COLUMNS (v)`,
763771
errorOnAddOrDropRegionSandwich: "pq: cannot perform database region changes while a ALTER PRIMARY KEY is underway",
764772
errorOnTableChangeSandwich: "pq: cannot ALTER PRIMARY KEY on a REGIONAL BY ROW table while a region is being added or dropped on the database",
765773
},
774+
{
775+
desc: "drop unique constraint",
776+
setup: `CREATE TABLE t.test (k INT NOT NULL, v INT, CONSTRAINT v_uniq UNIQUE (v)) LOCALITY REGIONAL BY ROW`,
777+
cmd: `ALTER TABLE t.test DROP CONSTRAINT v_uniq`,
778+
errorOnAddOrDropRegionSandwich: "pq: cannot perform database region changes while an index is being created or dropped on a REGIONAL BY ROW table",
779+
errorOnTableChangeSandwich: "pq: cannot ALTER TABLE DROP CONSTRAINT on a REGIONAL BY ROW table while a region is being added or dropped on the database",
780+
},
766781
}
767782

768783
regionChanges := []struct {
769-
cmd string
784+
desc string
785+
cmd string
770786
}{
771787
{
772-
cmd: `ALTER DATABASE t ADD REGION "us-east3"`,
788+
desc: "add region",
789+
cmd: `ALTER DATABASE t ADD REGION "us-east3"`,
773790
},
774791
{
775-
cmd: `ALTER DATABASE t DROP REGION "us-east2"`,
792+
desc: "drop region",
793+
cmd: `ALTER DATABASE t DROP REGION "us-east2"`,
776794
},
777795
}
778796

@@ -796,7 +814,7 @@ USE t;
796814
// Tests ADD/DROP REGION during a REGIONAL BY ROW index-related change.
797815
for _, rbrChange := range regionalByRowChanges {
798816
for _, regionChange := range regionChanges {
799-
t.Run(fmt.Sprintf("setup %s executing %s with racing %s", rbrChange.setup, rbrChange.cmd, regionChange.cmd), func(t *testing.T) {
817+
t.Run(fmt.Sprintf("%s racing %s", rbrChange.desc, regionChange.desc), func(t *testing.T) {
800818
defer log.Scope(t).Close(t)
801819
interruptStartCh := make(chan struct{})
802820
interruptEndCh := make(chan struct{})
@@ -862,7 +880,7 @@ USE t;
862880
// Tests REGIONAL BY ROW during a ADD/DROP REGION index-related change.
863881
for _, regionChange := range regionChanges {
864882
for _, rbrChange := range regionalByRowChanges {
865-
t.Run(fmt.Sprintf("setup %s executing %s with racing %s", rbrChange.setup, regionChange.cmd, rbrChange.cmd), func(t *testing.T) {
883+
t.Run(fmt.Sprintf("%s racing %s", regionChange.desc, rbrChange.desc), func(t *testing.T) {
866884
defer log.Scope(t).Close(t)
867885
interruptStartCh := make(chan struct{})
868886
interruptEndCh := make(chan struct{})

‎pkg/sql/catalog/tabledesc/structured.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1372,7 +1372,15 @@ func (desc *Mutable) DropConstraint(
13721372
desc.SetPrimaryIndex(primaryIndex)
13731373
return nil
13741374
}
1375-
return unimplemented.NewWithIssueDetailf(42840, "drop-constraint-unique",
1375+
// The declarative schema changer supports this already. Supporting it in
1376+
// the legacy schema changer is more complex because dropping an index
1377+
// requires managing the mutation lifecycle, handling FK back-references,
1378+
// cleaning up dependent views/functions/triggers, and managing sharded or
1379+
// expression index cleanup. This logic exists in dropIndexByName but is
1380+
// tightly coupled to DROP INDEX statement processing. Given the legacy
1381+
// schema changer is being deprecated, we direct users to DROP INDEX CASCADE
1382+
// rather than handling unique constraints here.
1383+
return unimplemented.Newf("drop-constraint-unique",
13761384
"cannot drop UNIQUE constraint %q using ALTER TABLE DROP CONSTRAINT, use DROP INDEX CASCADE instead",
13771385
tree.ErrNameString(u.GetName()))
13781386
}

‎pkg/sql/logictest/testdata/logic_test/alter_table‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -214,9 +214,13 @@ ALTER TABLE t DROP CONSTRAINT bar
214214
statement ok
215215
ALTER TABLE t DROP CONSTRAINT IF EXISTS bar
216216

217+
# Legacy schema changer does not support dropping unique constraints via
218+
# ALTER TABLE DROP CONSTRAINT.
219+
onlyif config local-legacy-schema-changer
217220
statement error cannot drop UNIQUE constraint \"foo\" using ALTER TABLE DROP CONSTRAINT, use DROP INDEX CASCADE instead
218221
ALTER TABLE t DROP CONSTRAINT foo
219222

223+
onlyif config local-legacy-schema-changer
220224
statement ok
221225
DROP INDEX foo CASCADE
222226

@@ -231,6 +235,11 @@ LIMIT 2
231235
SCHEMA CHANGE GC GC for DROP INDEX test.public.t@foo CASCADE root running 0 ·
232236
SCHEMA CHANGE DROP INDEX test.public.t@foo CASCADE root succeeded 1 ·
233237

238+
# Declarative schema changer supports dropping unique constraints via
239+
# ALTER TABLE DROP CONSTRAINT.
240+
skipif config local-legacy-schema-changer
241+
statement ok
242+
ALTER TABLE t DROP CONSTRAINT foo
234243

235244
skipif config local-legacy-schema-changer
236245
query TTTTRT retry
@@ -240,9 +249,8 @@ WHERE job_type = 'NEW SCHEMA CHANGE' OR job_type = 'SCHEMA CHANGE GC'
240249
ORDER BY created DESC
241250
LIMIT 2
242251
----
243-
SCHEMA CHANGE GC GC for DROP INDEX test.public.t@foo CASCADE node running 0 ·
244-
NEW SCHEMA CHANGE DROP INDEX test.public.t@foo CASCADE root succeeded 1 ·
245-
252+
SCHEMA CHANGE GC GC for ALTER TABLE test.public.t DROP CONSTRAINT foo node running 0 ·
253+
NEW SCHEMA CHANGE ALTER TABLE test.public.t DROP CONSTRAINT foo root succeeded 1 ·
246254

247255

248256
query TTBITTTBBBF colnames,rowsort
@@ -1882,9 +1890,21 @@ unique_without_index unique_d_e UNIQUE UNIQUE WITHOUT IN
18821890
unique_without_index unique_without_index_c_key UNIQUE UNIQUE (c ASC) true
18831891
unique_without_index unique_without_index_pkey PRIMARY KEY PRIMARY KEY (rowid ASC) true
18841892

1893+
# Drop an index-backed unique constraint using ALTER TABLE DROP CONSTRAINT.
1894+
skipif config local-legacy-schema-changer
1895+
statement ok
1896+
ALTER TABLE unique_without_index DROP CONSTRAINT unique_without_index_c_key
1897+
1898+
# Dropping the constraint only works with the declarative schema changer.
1899+
# For the legacy config, drop the index itself.
1900+
onlyif config local-legacy-schema-changer
18851901
statement error pgcode 0A000 cannot drop UNIQUE constraint \"unique_without_index_c_key\"
18861902
ALTER TABLE unique_without_index DROP CONSTRAINT unique_without_index_c_key
18871903

1904+
onlyif config local-legacy-schema-changer
1905+
statement ok
1906+
DROP INDEX unique_without_index_c_key CASCADE
1907+
18881908
statement error pgcode 42704 constraint \"unique_b\" of relation \"unique_without_index\" does not exist
18891909
ALTER TABLE unique_without_index DROP CONSTRAINT unique_b
18901910

@@ -1907,7 +1927,6 @@ unique_without_index unique_b_1 UNIQUE UNIQUE WITHOUT IN
19071927
unique_without_index unique_c UNIQUE UNIQUE WITHOUT INDEX (c) true
19081928
unique_without_index unique_d UNIQUE UNIQUE WITHOUT INDEX (d) true
19091929
unique_without_index unique_d_e UNIQUE UNIQUE WITHOUT INDEX (d, e) true
1910-
unique_without_index unique_without_index_c_key UNIQUE UNIQUE (c ASC) true
19111930
unique_without_index unique_without_index_pkey PRIMARY KEY PRIMARY KEY (rowid ASC) true
19121931

19131932
# Dropping a column in a unique constraint drops the constraint.
@@ -1923,7 +1942,6 @@ unique_without_index my_unique_f UNIQUE UNIQUE WITHOUT IN
19231942
unique_without_index unique_c UNIQUE UNIQUE WITHOUT INDEX (c) true
19241943
unique_without_index unique_d UNIQUE UNIQUE WITHOUT INDEX (d) true
19251944
unique_without_index unique_d_e UNIQUE UNIQUE WITHOUT INDEX (d, e) true
1926-
unique_without_index unique_without_index_c_key UNIQUE UNIQUE (c ASC) true
19271945
unique_without_index unique_without_index_pkey PRIMARY KEY PRIMARY KEY (rowid ASC) true
19281946

19291947
query TTTTB nosort
@@ -1949,7 +1967,6 @@ unique_without_index my_partial_unique_f UNIQUE UNIQUE WITHOUT IN
19491967
unique_without_index my_unique_e UNIQUE UNIQUE WITHOUT INDEX (e) true
19501968
unique_without_index my_unique_f UNIQUE UNIQUE WITHOUT INDEX (f) true
19511969
unique_without_index unique_c UNIQUE UNIQUE WITHOUT INDEX (c) true
1952-
unique_without_index unique_without_index_c_key UNIQUE UNIQUE (c ASC) true
19531970
unique_without_index unique_without_index_pkey PRIMARY KEY PRIMARY KEY (rowid ASC) true
19541971

19551972
query TTTTB

‎pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_constraint.go‎

Lines changed: 44 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import (
1616
"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
1717
"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
1818
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
19-
"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
2019
"github.com/cockroachdb/errors"
2120
)
2221

@@ -45,8 +44,10 @@ func alterTableDropConstraint(
4544
if dropPrimaryKeyConstraint(b, constraintElems, stmt, t) {
4645
return
4746
}
48-
// Dropping UNIQUE constraint: error out as not implemented.
49-
droppingUniqueConstraintNotImplemented(constraintElems, t)
47+
// Dropping UNIQUE constraint backed by an index.
48+
if dropUniqueIndexBackedConstraint(b, tn, tbl, constraintElems, stmt, t) {
49+
return
50+
}
5051

5152
_, _, constraintNameElem := scpb.FindConstraintWithoutIndexName(constraintElems)
5253
constraintID := constraintNameElem.ConstraintID
@@ -133,20 +134,48 @@ func dropPrimaryKeyConstraint(
133134
panic(scerrors.NotImplementedError(t))
134135
}
135136

136-
func droppingUniqueConstraintNotImplemented(
137-
constraintElems ElementResultSet, t *tree.AlterTableDropConstraint,
138-
) {
137+
// dropUniqueIndexBackedConstraint handles dropping a unique constraint that is
138+
// backed by a secondary index. Returns true if the constraint was handled,
139+
// false if not a unique index-backed constraint.
140+
func dropUniqueIndexBackedConstraint(
141+
b BuildCtx,
142+
tn *tree.TableName,
143+
tbl *scpb.Table,
144+
constraintElems ElementResultSet,
145+
stmt tree.Statement,
146+
t *tree.AlterTableDropConstraint,
147+
) bool {
139148
_, _, sie := scpb.FindSecondaryIndex(constraintElems)
140-
if sie != nil {
141-
if sie.IsUnique {
142-
panic(unimplemented.NewWithIssueDetailf(42840, "drop-constraint-unique",
143-
"cannot drop UNIQUE constraint %q using ALTER TABLE DROP CONSTRAINT, use DROP INDEX CASCADE instead",
144-
tree.ErrNameString(string(t.Constraint))))
145-
} else {
146-
panic(errors.AssertionFailedf("dropping an index-backed constraint but the " +
147-
"index is not unique"))
148-
}
149+
if sie == nil {
150+
return false
151+
}
152+
if !sie.IsUnique {
153+
panic(errors.AssertionFailedf("dropping an index-backed constraint but the index is not unique"))
149154
}
155+
156+
// Since this will drop the index as well, guard this behind the
157+
// sql_safe_updates flag.
158+
failIfSafeUpdates(b, t)
159+
160+
panicIfRegionChangeUnderwayOnRBRTable(b, "ALTER TABLE DROP CONSTRAINT", sie.TableID)
161+
162+
// Build index name for error messages and dependency handling.
163+
_, _, indexNameElem := scpb.FindIndexName(constraintElems)
164+
indexName := &tree.TableIndexName{
165+
Table: *tn,
166+
Index: tree.UnrestrictedName(indexNameElem.Name),
167+
}
168+
169+
// Reuse the DROP INDEX logic which handles all dependencies:
170+
// - Dependent views
171+
// - Dependent functions
172+
// - Dependent FK constraints
173+
// - Sharded index cleanup
174+
// - Expression index cleanup
175+
// - Dependent triggers
176+
dropSecondaryIndex(b, indexName, t.DropBehavior, sie, stmt)
177+
178+
return true
150179
}
151180

152181
func checkRegionalByRowConstraintConflict(

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2209,6 +2209,8 @@ func failIfSafeUpdates(b BuildCtx, n tree.NodeFormatter) {
22092209
"for certain type conversions or when applying a USING clause")
22102210
case *tree.DropIndex:
22112211
errorWithMessage = errors.New("DROP INDEX")
2212+
case *tree.AlterTableDropConstraint:
2213+
errorWithMessage = errors.New("ALTER TABLE DROP CONSTRAINT")
22122214
default:
22132215
panic(errors.AssertionFailedf("programming error: unexpected node type %T", n))
22142216
}

‎pkg/workload/schemachange/operation_generator.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2001,12 +2001,13 @@ func (og *operationGenerator) dropConstraint(ctx context.Context, tx pgx.Tx) (*o
20012001
}
20022002

20032003
// DROP INDEX CASCADE is preferred for dropping unique constraints, and
2004-
// dropping the constraint with ALTER TABLE ... DROP CONSTRAINT is unsupported.
2004+
// dropping the constraint with ALTER TABLE ... DROP CONSTRAINT is not
2005+
// supported by the legacy schema changer.
20052006
constraintIsUnique, err := og.constraintIsUnique(ctx, tx, tableName, constraintName)
20062007
if err != nil {
20072008
return nil, err
20082009
}
2009-
if constraintIsUnique {
2010+
if constraintIsUnique && !og.useDeclarativeSchemaChanger {
20102011
stmt.expectedExecErrors.add(pgcode.FeatureNotSupported)
20112012
}
20122013

0 commit comments

Comments
 (0)