Skip to content

Commit

Permalink
scbuild: add missing name check for ALTER PRIMARY KEY
Browse files Browse the repository at this point in the history
Previously, this check was missing, leading the schema change to hang
due to a validation failure in a non-revertible post-commit transaction.

This patch maintains the existing behavior of the legacy schema changer
which doesn't distinguish between unique and non-unique indexes: all
name collisions will be reported as constraint name collisions despite
non-unique indexes not being constraints.

Fixes #88131.

Release justification: low-risk high-value bug fix
Release note: None
  • Loading branch information
Marius Posta committed Sep 19, 2022
1 parent 58589fd commit 7016ae3
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 0 deletions.
32 changes: 32 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/alter_primary_key
Original file line number Diff line number Diff line change
Expand Up @@ -1763,6 +1763,38 @@ SELECT column_name FROM [SHOW COLUMNS FROM t_rowid] ORDER BY column_name;
k
v

subtest check_constraint_name

statement ok
CREATE TABLE t_name_check (a INT NOT NULL, CONSTRAINT ctcheck CHECK (a > 0))

skipif config local-legacy-schema-changer
statement error pgcode 42710 constraint with name "ctcheck" already exists
ALTER TABLE t_name_check ADD CONSTRAINT ctcheck PRIMARY KEY (a)

statement ok
ALTER TABLE t_name_check ADD CONSTRAINT t_name_check_pkey PRIMARY KEY (a)

statement ok
DROP TABLE t_name_check

statement ok
CREATE TABLE t_name_check (a INT NOT NULL, CONSTRAINT ctuniq UNIQUE (a))

skipif config local-legacy-schema-changer
statement error pgcode 42710 constraint with name "ctuniq" already exists
ALTER TABLE t_name_check ADD CONSTRAINT ctuniq PRIMARY KEY (a)

statement ok
DROP TABLE t_name_check

statement ok
CREATE TABLE t_name_check (a INT NOT NULL, INDEX idx (a))

skipif config local-legacy-schema-changer
statement error pgcode 42710 constraint with name "idx" already exists
ALTER TABLE t_name_check ADD CONSTRAINT idx PRIMARY KEY (a)

# The following subtest tests the case when the new primary key
# intersects with the old primary key.
subtest regression_85877
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ func alterPrimaryKey(b BuildCtx, tn *tree.TableName, tbl *scpb.Table, t alterPri
}
}
out.apply(b.Drop)
checkIfConstraintNameAlreadyExists(b, tbl, t)
sharding := makeShardedDescriptor(b, t)
var sourcePrimaryIndexElem *scpb.PrimaryIndex
if rowidToDrop == nil {
Expand Down Expand Up @@ -499,6 +500,25 @@ func mustRetrieveKeyIndexColumns(
return indexColumns
}

func checkIfConstraintNameAlreadyExists(b BuildCtx, tbl *scpb.Table, t alterPrimaryKeySpec) {
if t.Name == "" {
return
}
// Check explicit constraint names.
publicTableElts := b.QueryByID(tbl.TableID).Filter(publicTargetFilter)
scpb.ForEachConstraintName(publicTableElts, func(_ scpb.Status, _ scpb.TargetStatus, e *scpb.ConstraintName) {
if e.Name == string(t.Name) {
panic(pgerror.Newf(pgcode.DuplicateObject, "constraint with name %q already exists", t.Name))
}
})
// Check index names.
scpb.ForEachIndexName(publicTableElts, func(_ scpb.Status, _ scpb.TargetStatus, n *scpb.IndexName) {
if n.Name == string(t.Name) {
panic(pgerror.Newf(pgcode.DuplicateObject, "constraint with name %q already exists", t.Name))
}
})
}

// makeShardedDescriptor construct a sharded descriptor for the new primary key.
// Return nil if the new primary key is not hash-sharded.
func makeShardedDescriptor(b BuildCtx, t alterPrimaryKeySpec) *catpb.ShardedDescriptor {
Expand Down

0 comments on commit 7016ae3

Please sign in to comment.