Skip to content

Commit e12994b

Browse files
craig[bot]spilchen
craig[bot]
and
spilchen
committed
Merge #159180
159180: sql/schemachanger: clean up constraint comments on column drop r=spilchen a=spilchen Previously, when dropping a column that implicitly removed a constraint with an associated comment, the comment was not cleaned up. This commit ensures that such dangling comments are now removed for both the legacy and declarative schema changers. Additionally, this commit addresses several issues in the declarative schema changer: - Foreign key constraints were not dropped when their referenced columns were removed. This is now fixed. - scpb.ConstraintColumn entries were inconsistently populated in decomp.go, which has been corrected. Closes #158804 Release note (bug fix): Fixes a bug where comments associated with constraints were left behind after the column and constraint were dropped. Co-authored-by: Matt Spilchen <[email protected]>
2 parents a747b75 + a9a83d7 commit e12994b

File tree

6 files changed

+320
-12
lines changed

6 files changed

+320
-12
lines changed

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

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5040,6 +5040,129 @@ CREATE SEQUENCE public.t1_serial_columns_z_seq1 MINVALUE 1 MAXVALUE 922337203685
50405040
statement ok
50415041
RESET create_table_with_schema_locked
50425042

5043+
# Regression for #158804: dropping constraints with comments should not leave
5044+
# dangling constraint comments in system.comments table.
5045+
subtest drop_constraint_removes_comments
5046+
5047+
# Setup: Create parent table for foreign key constraints.
5048+
statement ok
5049+
CREATE TABLE t_parent (
5050+
id INT PRIMARY KEY
5051+
);
5052+
5053+
# Test table with various constraint types.
5054+
statement ok
5055+
CREATE TABLE t_constraints (
5056+
a INT,
5057+
b INT NOT NULL,
5058+
c INT UNIQUE,
5059+
d INT,
5060+
e INT,
5061+
CONSTRAINT pkey_a PRIMARY KEY (a),
5062+
CONSTRAINT check_b CHECK (b > 0),
5063+
CONSTRAINT unique_d UNIQUE WITHOUT INDEX (d),
5064+
CONSTRAINT fk_e FOREIGN KEY (e) REFERENCES t_parent(id),
5065+
FAMILY f1 (a, b, c, d, e)
5066+
) WITH (schema_locked = false);
5067+
5068+
# Add comments on all constraints.
5069+
statement ok
5070+
COMMENT ON CONSTRAINT pkey_a ON t_constraints IS 'primary key comment';
5071+
5072+
statement ok
5073+
COMMENT ON CONSTRAINT check_b ON t_constraints IS 'check constraint comment';
5074+
5075+
statement ok
5076+
COMMENT ON CONSTRAINT t_constraints_c_key ON t_constraints IS 'unique with index comment';
5077+
5078+
statement ok
5079+
COMMENT ON CONSTRAINT unique_d ON t_constraints IS 'unique without index comment';
5080+
5081+
statement ok
5082+
COMMENT ON CONSTRAINT fk_e ON t_constraints IS 'foreign key comment';
5083+
5084+
# Verify all comments exist (5 constraint comments).
5085+
query I
5086+
SELECT count(*) FROM system.comments WHERE type = 5 AND object_id = (
5087+
SELECT table_id FROM crdb_internal.tables
5088+
WHERE name = 't_constraints' AND schema_name = 'public' AND database_name = current_database()
5089+
);
5090+
----
5091+
5
5092+
5093+
# Drop check constraint directly - should remove its comment.
5094+
statement ok
5095+
ALTER TABLE t_constraints DROP CONSTRAINT check_b;
5096+
5097+
query I
5098+
SELECT count(*) FROM system.comments WHERE type = 5 AND object_id = (
5099+
SELECT table_id FROM crdb_internal.tables
5100+
WHERE name = 't_constraints' AND schema_name = 'public' AND database_name = current_database()
5101+
);
5102+
----
5103+
4
5104+
5105+
# Drop foreign key constraint via column drop - should remove its comment.
5106+
statement ok
5107+
ALTER TABLE t_constraints DROP COLUMN e;
5108+
5109+
query I
5110+
SELECT count(*) FROM system.comments WHERE type = 5 AND object_id = (
5111+
SELECT table_id FROM crdb_internal.tables
5112+
WHERE name = 't_constraints' AND schema_name = 'public' AND database_name = current_database()
5113+
);
5114+
----
5115+
3
5116+
5117+
# Drop unique without index constraint via column drop - should remove its comment.
5118+
statement ok
5119+
ALTER TABLE t_constraints DROP COLUMN d;
5120+
5121+
query I
5122+
SELECT count(*) FROM system.comments WHERE type = 5 AND object_id = (
5123+
SELECT table_id FROM crdb_internal.tables
5124+
WHERE name = 't_constraints' AND schema_name = 'public' AND database_name = current_database()
5125+
);
5126+
----
5127+
2
5128+
5129+
# Drop unique constraint with index via column drop - should remove its comment.
5130+
statement ok
5131+
ALTER TABLE t_constraints DROP COLUMN c;
5132+
5133+
query I
5134+
SELECT count(*) FROM system.comments WHERE type = 5 AND object_id = (
5135+
SELECT table_id FROM crdb_internal.tables
5136+
WHERE name = 't_constraints' AND schema_name = 'public' AND database_name = current_database()
5137+
);
5138+
----
5139+
1
5140+
5141+
# Change primary key - should retain old primary key comment and move it to the new PK.
5142+
statement ok
5143+
ALTER TABLE t_constraints ALTER PRIMARY KEY USING COLUMNS (b);
5144+
5145+
query I
5146+
SELECT count(*) FROM system.comments WHERE type = 5 AND object_id = (
5147+
SELECT table_id FROM crdb_internal.tables
5148+
WHERE name = 't_constraints' AND schema_name = 'public' AND database_name = current_database()
5149+
);
5150+
----
5151+
1
5152+
5153+
query T
5154+
SELECT obj_description(oid) FROM pg_constraint WHERE conrelid = 't_constraints'::regclass AND contype = 'p';
5155+
----
5156+
primary key comment
5157+
5158+
statement ok
5159+
DROP TABLE t_constraints;
5160+
5161+
statement ok
5162+
DROP TABLE t_parent;
5163+
5164+
subtest end
5165+
50435166
# Regression tests for issue #137326. Add column with IF NOT EXIST.
50445167
subtest add_col_if_not_exists
50455168

‎pkg/sql/schema_changer.go‎

Lines changed: 58 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1558,6 +1558,19 @@ func WaitToUpdateLeases(
15581558
return desc, err
15591559
}
15601560

1561+
type commentToDelete struct {
1562+
id int64
1563+
subID int64
1564+
commentType catalogkeys.CommentType
1565+
}
1566+
1567+
type commentToSwap struct {
1568+
id int64
1569+
oldSubID int64
1570+
newSubID int64
1571+
commentType catalogkeys.CommentType
1572+
}
1573+
15611574
// done finalizes the mutations (adds new cols/indexes to the table).
15621575
// It ensures that all nodes are on the current (pre-update) version of
15631576
// sc.descID and that all nodes are on the new (post-update) version of
@@ -1566,18 +1579,6 @@ func WaitToUpdateLeases(
15661579
// It also kicks off GC jobs as needed.
15671580
func (sc *SchemaChanger) done(ctx context.Context) error {
15681581
// Gathers ant comments that need to be swapped/cleaned.
1569-
type commentToDelete struct {
1570-
id int64
1571-
subID int64
1572-
commentType catalogkeys.CommentType
1573-
}
1574-
type commentToSwap struct {
1575-
id int64
1576-
oldSubID int64
1577-
newSubID int64
1578-
commentType catalogkeys.CommentType
1579-
}
1580-
var commentsToDelete []commentToDelete
15811582
var commentsToSwap []commentToSwap
15821583
// Jobs (for GC, etc.) that need to be started immediately after the table
15831584
// descriptor updates are published.
@@ -1935,6 +1936,11 @@ func (sc *SchemaChanger) done(ctx context.Context) error {
19351936
// Trim the executed mutations from the descriptor.
19361937
scTable.Mutations = scTable.Mutations[i:]
19371938

1939+
commentsToDelete, err := sc.findCommentsToDelete(ctx, txn, scTable)
1940+
if err != nil {
1941+
return err
1942+
}
1943+
19381944
// Check any jobs that we need to depend on for the current
19391945
// job to be successful.
19401946
existingDepMutationJobs, err := sc.getDependentMutationsJobs(ctx, scTable, committedMutations)
@@ -2205,6 +2211,46 @@ func (sc *SchemaChanger) done(ctx context.Context) error {
22052211
return nil
22062212
}
22072213

2214+
// findCommentsToDelete will collect any constraint comments whose referenced
2215+
// constraint no longer exists on the table. This is a catch-all for constraints
2216+
// implicitly removed by other operations (e.g. column drops).
2217+
func (sc *SchemaChanger) findCommentsToDelete(
2218+
ctx context.Context, txn descs.Txn, tbl catalog.TableDescriptor,
2219+
) ([]commentToDelete, error) {
2220+
seen := make(map[commentToDelete]struct{})
2221+
constraintIDs := make(map[uint32]struct{})
2222+
for _, c := range tbl.AllConstraints() {
2223+
constraintIDs[uint32(c.GetConstraintID())] = struct{}{}
2224+
}
2225+
ckPrefix := catalogkeys.MakeObjectCommentsMetadataPrefix(
2226+
sc.execCfg.Codec, catalogkeys.ConstraintCommentType, tbl.GetID())
2227+
kvs, err := txn.KV().Scan(ctx, ckPrefix, ckPrefix.PrefixEnd(), 0 /* maxRows */)
2228+
if err != nil {
2229+
return nil, err
2230+
}
2231+
var comments []commentToDelete
2232+
for _, kv := range kvs {
2233+
_, key, err := catalogkeys.DecodeCommentMetadataID(sc.execCfg.Codec, kv.Key)
2234+
if err != nil {
2235+
return nil, err
2236+
}
2237+
if _, ok := constraintIDs[key.SubID]; ok {
2238+
continue
2239+
}
2240+
ctd := commentToDelete{
2241+
id: int64(tbl.GetID()),
2242+
subID: int64(key.SubID),
2243+
commentType: catalogkeys.ConstraintCommentType,
2244+
}
2245+
if _, dup := seen[ctd]; dup {
2246+
continue
2247+
}
2248+
comments = append(comments, ctd)
2249+
seen[ctd] = struct{}{}
2250+
}
2251+
return comments, nil
2252+
}
2253+
22082254
// maybeUpdateZoneConfigsForPKChange moves zone configs for any rewritten
22092255
// indexes from the old index over to the new index. Noop if run on behalf of a
22102256
// tenant. If forceSwap is set, we copy the zone configs for primary keys

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,22 @@ func dropColumn(
244244
"cannot drop column %q because trigger %q on table %q depends on it",
245245
cn.Name, triggerName.Name, tableName.Name,
246246
))
247+
case *scpb.CheckConstraint:
248+
constraintElems := b.QueryByID(e.TableID).Filter(hasConstraintIDAttrFilter(e.ConstraintID))
249+
_, _, constraintName := scpb.FindConstraintWithoutIndexName(constraintElems.Filter(publicTargetFilter))
250+
alterTableDropConstraint(b, tn, tbl, stmt, &tree.AlterTableDropConstraint{
251+
IfExists: false,
252+
Constraint: tree.Name(constraintName.Name),
253+
DropBehavior: behavior,
254+
})
255+
case *scpb.CheckConstraintUnvalidated:
256+
constraintElems := b.QueryByID(e.TableID).Filter(hasConstraintIDAttrFilter(e.ConstraintID))
257+
_, _, constraintName := scpb.FindConstraintWithoutIndexName(constraintElems.Filter(publicTargetFilter))
258+
alterTableDropConstraint(b, tn, tbl, stmt, &tree.AlterTableDropConstraint{
259+
IfExists: false,
260+
Constraint: tree.Name(constraintName.Name),
261+
DropBehavior: behavior,
262+
})
247263
case *scpb.UniqueWithoutIndexConstraint:
248264
constraintElems := b.QueryByID(e.TableID).Filter(hasConstraintIDAttrFilter(e.ConstraintID))
249265
_, _, constraintName := scpb.FindConstraintWithoutIndexName(constraintElems.Filter(publicTargetFilter))
@@ -260,6 +276,23 @@ func dropColumn(
260276
Constraint: tree.Name(constraintName.Name),
261277
DropBehavior: behavior,
262278
})
279+
case *scpb.ForeignKeyConstraint:
280+
constraintElems := b.QueryByID(e.TableID).Filter(hasConstraintIDAttrFilter(e.ConstraintID))
281+
_, _, constraintName := scpb.FindConstraintWithoutIndexName(constraintElems.Filter(publicTargetFilter))
282+
alterTableDropConstraint(b, tn, tbl, stmt, &tree.AlterTableDropConstraint{
283+
IfExists: false,
284+
Constraint: tree.Name(constraintName.Name),
285+
DropBehavior: behavior,
286+
})
287+
case *scpb.ForeignKeyConstraintUnvalidated:
288+
constraintElems := b.QueryByID(e.TableID).Filter(hasConstraintIDAttrFilter(e.ConstraintID))
289+
_, _, constraintName := scpb.FindConstraintWithoutIndexName(constraintElems.Filter(publicTargetFilter))
290+
alterTableDropConstraint(b, tn, tbl, stmt, &tree.AlterTableDropConstraint{
291+
IfExists: false,
292+
Constraint: tree.Name(constraintName.Name),
293+
DropBehavior: behavior,
294+
})
295+
263296
case *scpb.RowLevelTTL:
264297
// If a duration expression is set, the column level dependency is on the
265298
// internal ttl column, which we are attempting to drop.
@@ -308,6 +341,7 @@ func walkColumnDependencies(
308341
var sequenceDeps catalog.DescriptorIDSet
309342
var indexDeps catid.IndexSet
310343
var columnDeps catalog.TableColSet
344+
var constraintDeps catid.ConstraintSet
311345
tblElts := b.QueryByID(col.TableID).Filter(orFilter(publicTargetFilter, transientTargetFilter))
312346

313347
// Panic if `col` is referenced in a predicate of an index or
@@ -368,6 +402,7 @@ func walkColumnDependencies(
368402
}
369403
})
370404

405+
// First loop: Process columns and indexes, collecting constraint IDs.
371406
tblElts.ForEach(func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) {
372407
switch elt := e.(type) {
373408
case *scpb.Column:
@@ -376,14 +411,31 @@ func walkColumnDependencies(
376411
}
377412
case *scpb.PrimaryIndex:
378413
if indexDeps.Contains(elt.IndexID) {
414+
if elt.ConstraintID != 0 {
415+
constraintDeps.Add(elt.ConstraintID)
416+
}
379417
fn(e, op, objType)
380418
}
381419
case *scpb.SecondaryIndex:
382420
if indexDeps.Contains(elt.IndexID) {
421+
if elt.ConstraintID != 0 {
422+
constraintDeps.Add(elt.ConstraintID)
423+
}
383424
fn(e, op, objType)
384425
}
385426
}
386427
})
428+
429+
// Second loop: Process constraint comments using the fully-populated constraintDeps.
430+
if !constraintDeps.Empty() {
431+
tblElts.ForEach(func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) {
432+
if cc, ok := e.(*scpb.ConstraintComment); ok {
433+
if constraintDeps.Contains(cc.ConstraintID) {
434+
fn(e, op, objType)
435+
}
436+
}
437+
})
438+
}
387439
sequenceDeps.ForEach(func(id descpb.ID) {
388440
_, target, seq := scpb.FindSequence(b.QueryByID(id))
389441
if target == scpb.ToPublic && seq != nil {

‎pkg/sql/schemachanger/scbuild/testdata/zone_cfg‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,8 @@ ALTER TABLE defaultdb.foo_index_zone_cfg DROP COLUMN k
8989
{indexId: 3, tableId: 105}
9090
- [[CheckConstraint:{DescID: 105, IndexID: 0, ConstraintID: 4, ReferencedColumnIDs: [2]}, ABSENT], PUBLIC]
9191
{columnIds: [2], constraintId: 4, expr: 'k > 10:::INT8', referencedColumnIds: [2], tableId: 105}
92+
- [[ConstraintWithoutIndexName:{DescID: 105, Name: check_k, ConstraintID: 4}, ABSENT], PUBLIC]
93+
{constraintId: 4, name: check_k, tableId: 105}
9294
- [[IndexZoneConfig:{DescID: 105, IndexID: 3, SeqNum: 0}, ABSENT], PUBLIC]
9395
{indexId: 3, oldIdxRef: -1, subzone: {config: {gc: {ttlSeconds: 10}, inheritedConstraints: true, inheritedLeasePreferences: true}, indexId: 3}, subzoneSpans: [{key: iw==}], tableId: 105}
9496
- [[TableData:{DescID: 105, ReferencedDescID: 100}, PUBLIC], PUBLIC]

‎pkg/sql/sem/catid/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library")
33
go_library(
44
name = "catid",
55
srcs = [
6+
"constraint_id_set.go",
67
"ids.go",
78
"index_id_set.go",
89
],

0 commit comments

Comments
 (0)