From 20bef2c3110af295501919bac463b87ac58876de Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Fri, 14 Sep 2018 13:41:20 -0300 Subject: [PATCH] Fix ALTER/TYPE on columns referenced by FKs in partitioned tables When ALTER TABLE ... SET DATA TYPE affects a column referenced by constraints and indexes, it drop those constraints and indexes and recreates them afterwards, so that the definitions match the new data type. The original code did this by dropping one object at a time (commit 077db40fa1f3 of May 2004), which worked fine because the dependencies between the objects were pretty straightforward, and ordering the objects in a specific way was enough to make this work. However, when there are foreign key constraints in partitioned tables, the dependencies are no longer so straightforward, and we were getting errors when attempted: ERROR: cache lookup failed for constraint 16398 This can be fixed by doing all the drops in one pass instead, using performMultipleDeletions (introduced by df18c51f2955 of Aug 2006). With this change we can also remove the code to carefully order the list of objects to be deleted. Reported-by: Rajkumar Raghuwanshi Reviewed-by: Tom Lane Discussion: https://postgr.es/m/CAKcux6nWS_m+s=1Udk_U9B+QY7pA-Ac58qR5BdUfOyrwnWHDew@mail.gmail.com --- src/backend/commands/tablecmds.c | 71 ++++++++--------------- src/test/regress/expected/foreign_key.out | 12 ++++ src/test/regress/sql/foreign_key.sql | 11 ++++ 3 files changed, 47 insertions(+), 47 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index e96512e051..03c0b739b3 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -9527,33 +9527,12 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, { char *defstring = pg_get_constraintdef_command(foundObject.objectId); - /* - * Put NORMAL dependencies at the front of the list and - * AUTO dependencies at the back. This makes sure that - * foreign-key constraints depending on this column will - * be dropped before unique or primary-key constraints of - * the column; which we must have because the FK - * constraints depend on the indexes belonging to the - * unique constraints. - */ - if (foundDep->deptype == DEPENDENCY_NORMAL) - { - tab->changedConstraintOids = - lcons_oid(foundObject.objectId, - tab->changedConstraintOids); - tab->changedConstraintDefs = - lcons(defstring, - tab->changedConstraintDefs); - } - else - { - tab->changedConstraintOids = - lappend_oid(tab->changedConstraintOids, - foundObject.objectId); - tab->changedConstraintDefs = - lappend(tab->changedConstraintDefs, - defstring); - } + tab->changedConstraintOids = + lappend_oid(tab->changedConstraintOids, + foundObject.objectId); + tab->changedConstraintDefs = + lappend(tab->changedConstraintDefs, + defstring); } break; @@ -9893,9 +9872,17 @@ static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) { ObjectAddress obj; + ObjectAddresses *objects; ListCell *def_item; ListCell *oid_item; + /* + * Collect all the constraints and indexes to drop so we can process them + * in a single call. That way we don't have to worry about dependencies + * among them. + */ + objects = new_object_addresses(); + /* * Re-parse the index and constraint definitions, and attach them to the * appropriate work queue entries. We do this before dropping because in @@ -9937,6 +9924,9 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) conislocal = con->conislocal; ReleaseSysCache(tup); + ObjectAddressSet(obj, ConstraintRelationId, lfirst_oid(oid_item)); + add_exact_object_address(&obj, objects); + /* * If the constraint is inherited (only), we don't want to inject a * new definition here; it'll get recreated when ATAddCheckConstraint @@ -9960,31 +9950,18 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) ATPostAlterTypeParse(oldId, relid, InvalidOid, (char *) lfirst(def_item), wqueue, lockmode, tab->rewrite); + + ObjectAddressSet(obj, RelationRelationId, lfirst_oid(oid_item)); + add_exact_object_address(&obj, objects); } /* - * Now we can drop the existing constraints and indexes --- constraints - * first, since some of them might depend on the indexes. In fact, we - * have to delete FOREIGN KEY constraints before UNIQUE constraints, but - * we already ordered the constraint list to ensure that would happen. It - * should be okay to use DROP_RESTRICT here, since nothing else should be - * depending on these objects. + * It should be okay to use DROP_RESTRICT here, since nothing else should + * be depending on these objects. */ - foreach(oid_item, tab->changedConstraintOids) - { - obj.classId = ConstraintRelationId; - obj.objectId = lfirst_oid(oid_item); - obj.objectSubId = 0; - performDeletion(&obj, DROP_RESTRICT, PERFORM_DELETION_INTERNAL); - } + performMultipleDeletions(objects, DROP_RESTRICT, PERFORM_DELETION_INTERNAL); - foreach(oid_item, tab->changedIndexOids) - { - obj.classId = RelationRelationId; - obj.objectId = lfirst_oid(oid_item); - obj.objectSubId = 0; - performDeletion(&obj, DROP_RESTRICT, PERFORM_DELETION_INTERNAL); - } + free_object_addresses(objects); /* * The objects will get recreated during subsequent passes over the work diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index b90c4926e2..fc3bbe4deb 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -1518,6 +1518,18 @@ DETAIL: Key (a, b)=(2500, 2502) is still referenced from table "fk_partitioned_ ALTER TABLE fk_partitioned_fk DROP CONSTRAINT fk_partitioned_fk_a_fkey; -- done. DROP TABLE fk_notpartitioned_pk, fk_partitioned_fk; +-- Altering a type referenced by a foreign key needs to drop/recreate the FK. +-- Ensure that works. +CREATE TABLE fk_notpartitioned_pk (a INT, PRIMARY KEY(a), CHECK (a > 0)); +CREATE TABLE fk_partitioned_fk (a INT REFERENCES fk_notpartitioned_pk(a) PRIMARY KEY) PARTITION BY RANGE(a); +CREATE TABLE fk_partitioned_fk_1 PARTITION OF fk_partitioned_fk FOR VALUES FROM (MINVALUE) TO (MAXVALUE); +INSERT INTO fk_notpartitioned_pk VALUES (1); +INSERT INTO fk_partitioned_fk VALUES (1); +ALTER TABLE fk_notpartitioned_pk ALTER COLUMN a TYPE bigint; +DELETE FROM fk_notpartitioned_pk WHERE a = 1; +ERROR: update or delete on table "fk_notpartitioned_pk" violates foreign key constraint "fk_partitioned_fk_a_fkey" on table "fk_partitioned_fk" +DETAIL: Key (a)=(1) is still referenced from table "fk_partitioned_fk". +DROP TABLE fk_notpartitioned_pk, fk_partitioned_fk; -- Test some other exotic foreign key features: MATCH SIMPLE, ON UPDATE/DELETE -- actions CREATE TABLE fk_notpartitioned_pk (a int, b int, primary key (a, b)); diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index f3e7058583..d2cecdf4eb 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -1139,6 +1139,17 @@ ALTER TABLE fk_partitioned_fk DROP CONSTRAINT fk_partitioned_fk_a_fkey; -- done. DROP TABLE fk_notpartitioned_pk, fk_partitioned_fk; +-- Altering a type referenced by a foreign key needs to drop/recreate the FK. +-- Ensure that works. +CREATE TABLE fk_notpartitioned_pk (a INT, PRIMARY KEY(a), CHECK (a > 0)); +CREATE TABLE fk_partitioned_fk (a INT REFERENCES fk_notpartitioned_pk(a) PRIMARY KEY) PARTITION BY RANGE(a); +CREATE TABLE fk_partitioned_fk_1 PARTITION OF fk_partitioned_fk FOR VALUES FROM (MINVALUE) TO (MAXVALUE); +INSERT INTO fk_notpartitioned_pk VALUES (1); +INSERT INTO fk_partitioned_fk VALUES (1); +ALTER TABLE fk_notpartitioned_pk ALTER COLUMN a TYPE bigint; +DELETE FROM fk_notpartitioned_pk WHERE a = 1; +DROP TABLE fk_notpartitioned_pk, fk_partitioned_fk; + -- Test some other exotic foreign key features: MATCH SIMPLE, ON UPDATE/DELETE -- actions CREATE TABLE fk_notpartitioned_pk (a int, b int, primary key (a, b));