From 923c13520f1b01a5563ec272de5a0a59b343f892 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Wed, 5 May 2021 12:14:21 -0400 Subject: [PATCH] Have ALTER CONSTRAINT recurse on partitioned tables When ALTER TABLE .. ALTER CONSTRAINT changes deferrability properties changed in a partitioned table, we failed to propagate those changes correctly to partitions and to triggers. Repair by adding a recursion mechanism to affect all derived constraints and all derived triggers. (In particular, recurse to partitions even if their respective parents are already in the desired state: it is possible for the partitions to have been altered individually.) Because foreign keys involve tables in two sides, we cannot use the standard ALTER TABLE recursion mechanism, so we invent our own by following pg_constraint.conparentid down. When ALTER TABLE .. ALTER CONSTRAINT is invoked on the derived pg_constraint object that's automaticaly created in a partition as a result of a constraint added to its parent, raise an error instead of pretending to work and then failing to modify all the affected triggers. Before this commit such a command would be allowed but failed to affect all triggers, so it would silently misbehave. (Restoring dumps of existing databases is not affected, because pg_dump does not produce anything for such a derived constraint anyway.) Add some tests for the case. Backpatch to 11, where foreign key support was added to partitioned tables by commit 3de241dba86f. (A related change is commit f56f8f8da6af in pg12 which added support for FKs *referencing* partitioned tables; this is what forces us to use an ad-hoc recursion mechanism for this.) Diagnosed by Tom Lane from bug report from Ron L Johnson. As of this writing, no reviews were offered. Discussion: https://postgr.es/m/75fe0761-a291-86a9-c8d8-4906da077469@gmail.com Discussion: https://postgr.es/m/3144850.1607369633@sss.pgh.pa.us --- src/backend/commands/tablecmds.c | 196 +++++++++++++++++----- src/test/regress/expected/foreign_key.out | 78 +++++++++ src/test/regress/sql/foreign_key.sql | 75 +++++++++ 3 files changed, 311 insertions(+), 38 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 022d5e9b2d..84e6e9dbc5 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -328,6 +328,9 @@ static void AlterSeqNamespaces(Relation classRel, Relation rel, LOCKMODE lockmode); static ObjectAddress ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd, bool recurse, bool recursing, LOCKMODE lockmode); +static bool ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel, + Relation rel, HeapTuple contuple, List **otherrelids, + LOCKMODE lockmode); static ObjectAddress ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, bool recurse, bool recursing, LOCKMODE lockmode); @@ -4292,6 +4295,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, break; case AT_AlterConstraint: /* ALTER CONSTRAINT */ ATSimplePermissions(rel, ATT_TABLE); + /* Recursion occurs during execution phase */ pass = AT_PASS_MISC; break; case AT_ValidateConstraint: /* VALIDATE CONSTRAINT */ @@ -9718,28 +9722,29 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, * Update the attributes of a constraint. * * Currently only works for Foreign Key constraints. - * Foreign keys do not inherit, so we purposely ignore the - * recursion bit here, but we keep the API the same for when - * other constraint types are supported. * * If the constraint is modified, returns its address; otherwise, return * InvalidObjectAddress. */ static ObjectAddress -ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd, - bool recurse, bool recursing, LOCKMODE lockmode) +ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd, bool recurse, + bool recursing, LOCKMODE lockmode) { Constraint *cmdcon; Relation conrel; + Relation tgrel; SysScanDesc scan; ScanKeyData skey[3]; HeapTuple contuple; Form_pg_constraint currcon; ObjectAddress address; + List *otherrelids = NIL; + ListCell *lc; cmdcon = castNode(Constraint, cmd->def); conrel = table_open(ConstraintRelationId, RowExclusiveLock); + tgrel = table_open(TriggerRelationId, RowExclusiveLock); /* * Find and check the target constraint @@ -9773,21 +9778,120 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd, errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key constraint", cmdcon->conname, RelationGetRelationName(rel)))); + /* + * If it's not the topmost constraint, raise an error. + * + * Altering a non-topmost constraint leaves some triggers untouched, since + * they are not directly connected to this constraint; also, pg_dump would + * ignore the deferrability status of the individual constraint, since it + * only dumps topmost constraints. Avoid these problems by refusing this + * operation and telling the user to alter the parent constraint instead. + */ + if (OidIsValid(currcon->conparentid)) + { + HeapTuple tp; + Oid parent = currcon->conparentid; + char *ancestorname = NULL; + char *ancestortable = NULL; + + /* Loop to find the topmost constraint */ + while (HeapTupleIsValid(tp = SearchSysCache1(CONSTROID, ObjectIdGetDatum(parent)))) + { + Form_pg_constraint contup = (Form_pg_constraint) GETSTRUCT(tp); + + /* If no parent, this is the constraint we want */ + if (!OidIsValid(contup->conparentid)) + { + ancestorname = pstrdup(NameStr(contup->conname)); + ancestortable = get_rel_name(contup->conrelid); + ReleaseSysCache(tp); + break; + } + + parent = contup->conparentid; + ReleaseSysCache(tp); + } + + ereport(ERROR, + (errmsg("cannot alter constraint \"%s\" on relation \"%s\"", + cmdcon->conname, RelationGetRelationName(rel)), + ancestorname && ancestortable ? + errdetail("Constraint \"%s\" is derived from constraint \"%s\" of relation \"%s\".", + cmdcon->conname, ancestorname, ancestortable) : 0, + errhint("You may alter the constraint it derives from, instead."))); + } + + /* + * Do the actual catalog work. We can skip changing if already in the + * desired state, but not if a partitioned table: partitions need to be + * processed regardless, in case they had the constraint locally changed. + */ + address = InvalidObjectAddress; + if (currcon->condeferrable != cmdcon->deferrable || + currcon->condeferred != cmdcon->initdeferred || + rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + if (ATExecAlterConstrRecurse(cmdcon, conrel, tgrel, rel, contuple, + &otherrelids, lockmode)) + ObjectAddressSet(address, ConstraintRelationId, currcon->oid); + } + + /* + * ATExecConstrRecurse already invalidated relcache for the relations + * having the constraint itself; here we also invalidate for relations + * that have any triggers that are part of the constraint. + */ + foreach(lc, otherrelids) + CacheInvalidateRelcacheByRelid(lfirst_oid(lc)); + + systable_endscan(scan); + + table_close(tgrel, RowExclusiveLock); + table_close(conrel, RowExclusiveLock); + + return address; +} + +/* + * Recursive subroutine of ATExecAlterConstraint. Returns true if the + * constraint is altered. + * + * *otherrelids is appended OIDs of relations containing affected triggers. + * + * Note that we must recurse even when the values are correct, in case + * indirect descendants have had their constraints altered locally. + * (This could be avoided if we forbade altering constraints in partitions + * but existing releases don't do that.) + */ +static bool +ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel, + Relation rel, HeapTuple contuple, List **otherrelids, + LOCKMODE lockmode) +{ + Form_pg_constraint currcon; + Oid conoid; + Oid refrelid; + bool changed = false; + + currcon = (Form_pg_constraint) GETSTRUCT(contuple); + conoid = currcon->oid; + refrelid = currcon->confrelid; + + /* + * Update pg_constraint with the flags from cmdcon. + * + * If called to modify a constraint that's already in the desired state, + * silently do nothing. + */ if (currcon->condeferrable != cmdcon->deferrable || currcon->condeferred != cmdcon->initdeferred) { HeapTuple copyTuple; - HeapTuple tgtuple; Form_pg_constraint copy_con; - List *otherrelids = NIL; + HeapTuple tgtuple; ScanKeyData tgkey; SysScanDesc tgscan; - Relation tgrel; - ListCell *lc; - /* - * Now update the catalog, while we have the door open. - */ copyTuple = heap_copytuple(contuple); copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple); copy_con->condeferrable = cmdcon->deferrable; @@ -9795,28 +9899,29 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd, CatalogTupleUpdate(conrel, ©Tuple->t_self, copyTuple); InvokeObjectPostAlterHook(ConstraintRelationId, - currcon->oid, 0); + conoid, 0); heap_freetuple(copyTuple); + changed = true; + + /* Make new constraint flags visible to others */ + CacheInvalidateRelcache(rel); /* * Now we need to update the multiple entries in pg_trigger that * implement the constraint. */ - tgrel = table_open(TriggerRelationId, RowExclusiveLock); - ScanKeyInit(&tgkey, Anum_pg_trigger_tgconstraint, BTEqualStrategyNumber, F_OIDEQ, - ObjectIdGetDatum(currcon->oid)); - + ObjectIdGetDatum(conoid)); tgscan = systable_beginscan(tgrel, TriggerConstraintIndexId, true, NULL, 1, &tgkey); - while (HeapTupleIsValid(tgtuple = systable_getnext(tgscan))) { Form_pg_trigger tgform = (Form_pg_trigger) GETSTRUCT(tgtuple); Form_pg_trigger copy_tg; + HeapTuple copyTuple; /* * Remember OIDs of other relation(s) involved in FK constraint. @@ -9825,8 +9930,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd, * change, but let's be conservative.) */ if (tgform->tgrelid != RelationGetRelid(rel)) - otherrelids = list_append_unique_oid(otherrelids, - tgform->tgrelid); + *otherrelids = list_append_unique_oid(*otherrelids, + tgform->tgrelid); /* * Update deferrability of RI_FKey_noaction_del, @@ -9853,31 +9958,46 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd, } systable_endscan(tgscan); + } - table_close(tgrel, RowExclusiveLock); + /* + * If the table at either end of the constraint is partitioned, we need to + * recurse and handle every constraint that is a child of this one. + * + * (This assumes that the recurse flag is forcibly set for partitioned + * tables, and not set for legacy inheritance, though we don't check for + * that here.) + */ + if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE || + get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE) + { + ScanKeyData pkey; + SysScanDesc pscan; + HeapTuple childtup; - /* - * Invalidate relcache so that others see the new attributes. We must - * inval both the named rel and any others having relevant triggers. - * (At present there should always be exactly one other rel, but - * there's no need to hard-wire such an assumption here.) - */ - CacheInvalidateRelcache(rel); - foreach(lc, otherrelids) + ScanKeyInit(&pkey, + Anum_pg_constraint_conparentid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(conoid)); + + pscan = systable_beginscan(conrel, ConstraintParentIndexId, + true, NULL, 1, &pkey); + + while (HeapTupleIsValid(childtup = systable_getnext(pscan))) { - CacheInvalidateRelcacheByRelid(lfirst_oid(lc)); + Form_pg_constraint childcon = (Form_pg_constraint) GETSTRUCT(childtup); + Relation childrel; + + childrel = table_open(childcon->conrelid, lockmode); + ATExecAlterConstrRecurse(cmdcon, conrel, tgrel, childrel, childtup, + otherrelids, lockmode); + table_close(childrel, NoLock); } - ObjectAddressSet(address, ConstraintRelationId, currcon->oid); + systable_endscan(pscan); } - else - address = InvalidObjectAddress; - systable_endscan(scan); - - table_close(conrel, RowExclusiveLock); - - return address; + return changed; } /* diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 07bd5b6434..16698b7a07 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -2313,6 +2313,84 @@ SET CONSTRAINTS fk_a_fkey DEFERRED; DELETE FROM pk WHERE a = 1; DELETE FROM fk WHERE a = 1; COMMIT; -- OK +-- Verify constraint deferrability when changed by ALTER +-- Partitioned table at referencing end +CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2)); +CREATE TABLE ref(f1 int, f2 int, f3 int) + PARTITION BY list(f1); +CREATE TABLE ref1 PARTITION OF ref FOR VALUES IN (1); +CREATE TABLE ref2 PARTITION OF ref FOR VALUES in (2); +ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt; +ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey + DEFERRABLE INITIALLY DEFERRED; +INSERT INTO pt VALUES(1,2,3); +INSERT INTO ref VALUES(1,2,3); +BEGIN; +DELETE FROM pt; +DELETE FROM ref; +ABORT; +DROP TABLE pt, ref; +-- Multi-level partitioning at referencing end +CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2)); +CREATE TABLE ref(f1 int, f2 int, f3 int) + PARTITION BY list(f1); +CREATE TABLE ref1_2 PARTITION OF ref FOR VALUES IN (1, 2) PARTITION BY list (f2); +CREATE TABLE ref1 PARTITION OF ref1_2 FOR VALUES IN (1); +CREATE TABLE ref2 PARTITION OF ref1_2 FOR VALUES IN (2) PARTITION BY list (f2); +CREATE TABLE ref22 PARTITION OF ref2 FOR VALUES IN (2); +ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt; +INSERT INTO pt VALUES(1,2,3); +INSERT INTO ref VALUES(1,2,3); +ALTER TABLE ref22 ALTER CONSTRAINT ref_f1_f2_fkey + DEFERRABLE INITIALLY IMMEDIATE; -- fails +ERROR: cannot alter constraint "ref_f1_f2_fkey" on relation "ref22" +DETAIL: Constraint "ref_f1_f2_fkey" is derived from constraint "ref_f1_f2_fkey" of relation "ref". +HINT: You may alter the constraint it derives from, instead. +ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey + DEFERRABLE INITIALLY DEFERRED; +BEGIN; +DELETE FROM pt; +DELETE FROM ref; +ABORT; +DROP TABLE pt, ref; +-- Partitioned table at referenced end +CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2)) + PARTITION BY LIST(f1); +CREATE TABLE pt1 PARTITION OF pt FOR VALUES IN (1); +CREATE TABLE pt2 PARTITION OF pt FOR VALUES IN (2); +CREATE TABLE ref(f1 int, f2 int, f3 int); +ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt; +ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey + DEFERRABLE INITIALLY DEFERRED; +INSERT INTO pt VALUES(1,2,3); +INSERT INTO ref VALUES(1,2,3); +BEGIN; +DELETE FROM pt; +DELETE FROM ref; +ABORT; +DROP TABLE pt, ref; +-- Multi-level partitioning at at referenced end +CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2)) + PARTITION BY LIST(f1); +CREATE TABLE pt1_2 PARTITION OF pt FOR VALUES IN (1, 2) PARTITION BY LIST (f1); +CREATE TABLE pt1 PARTITION OF pt1_2 FOR VALUES IN (1); +CREATE TABLE pt2 PARTITION OF pt1_2 FOR VALUES IN (2); +CREATE TABLE ref(f1 int, f2 int, f3 int); +ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt; +ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey1 + DEFERRABLE INITIALLY DEFERRED; -- fails +ERROR: cannot alter constraint "ref_f1_f2_fkey1" on relation "ref" +DETAIL: Constraint "ref_f1_f2_fkey1" is derived from constraint "ref_f1_f2_fkey" of relation "ref". +HINT: You may alter the constraint it derives from, instead. +ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey + DEFERRABLE INITIALLY DEFERRED; +INSERT INTO pt VALUES(1,2,3); +INSERT INTO ref VALUES(1,2,3); +BEGIN; +DELETE FROM pt; +DELETE FROM ref; +ABORT; +DROP TABLE pt, ref; DROP SCHEMA fkpart9 CASCADE; NOTICE: drop cascades to 2 other objects DETAIL: drop cascades to table pk diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index c5c9011afc..c52672e03a 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -1636,6 +1636,81 @@ SET CONSTRAINTS fk_a_fkey DEFERRED; DELETE FROM pk WHERE a = 1; DELETE FROM fk WHERE a = 1; COMMIT; -- OK + +-- Verify constraint deferrability when changed by ALTER +-- Partitioned table at referencing end +CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2)); +CREATE TABLE ref(f1 int, f2 int, f3 int) + PARTITION BY list(f1); +CREATE TABLE ref1 PARTITION OF ref FOR VALUES IN (1); +CREATE TABLE ref2 PARTITION OF ref FOR VALUES in (2); +ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt; +ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey + DEFERRABLE INITIALLY DEFERRED; +INSERT INTO pt VALUES(1,2,3); +INSERT INTO ref VALUES(1,2,3); +BEGIN; +DELETE FROM pt; +DELETE FROM ref; +ABORT; +DROP TABLE pt, ref; +-- Multi-level partitioning at referencing end +CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2)); +CREATE TABLE ref(f1 int, f2 int, f3 int) + PARTITION BY list(f1); +CREATE TABLE ref1_2 PARTITION OF ref FOR VALUES IN (1, 2) PARTITION BY list (f2); +CREATE TABLE ref1 PARTITION OF ref1_2 FOR VALUES IN (1); +CREATE TABLE ref2 PARTITION OF ref1_2 FOR VALUES IN (2) PARTITION BY list (f2); +CREATE TABLE ref22 PARTITION OF ref2 FOR VALUES IN (2); +ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt; +INSERT INTO pt VALUES(1,2,3); +INSERT INTO ref VALUES(1,2,3); +ALTER TABLE ref22 ALTER CONSTRAINT ref_f1_f2_fkey + DEFERRABLE INITIALLY IMMEDIATE; -- fails +ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey + DEFERRABLE INITIALLY DEFERRED; +BEGIN; +DELETE FROM pt; +DELETE FROM ref; +ABORT; +DROP TABLE pt, ref; + +-- Partitioned table at referenced end +CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2)) + PARTITION BY LIST(f1); +CREATE TABLE pt1 PARTITION OF pt FOR VALUES IN (1); +CREATE TABLE pt2 PARTITION OF pt FOR VALUES IN (2); +CREATE TABLE ref(f1 int, f2 int, f3 int); +ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt; +ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey + DEFERRABLE INITIALLY DEFERRED; +INSERT INTO pt VALUES(1,2,3); +INSERT INTO ref VALUES(1,2,3); +BEGIN; +DELETE FROM pt; +DELETE FROM ref; +ABORT; +DROP TABLE pt, ref; +-- Multi-level partitioning at at referenced end +CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2)) + PARTITION BY LIST(f1); +CREATE TABLE pt1_2 PARTITION OF pt FOR VALUES IN (1, 2) PARTITION BY LIST (f1); +CREATE TABLE pt1 PARTITION OF pt1_2 FOR VALUES IN (1); +CREATE TABLE pt2 PARTITION OF pt1_2 FOR VALUES IN (2); +CREATE TABLE ref(f1 int, f2 int, f3 int); +ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt; +ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey1 + DEFERRABLE INITIALLY DEFERRED; -- fails +ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey + DEFERRABLE INITIALLY DEFERRED; +INSERT INTO pt VALUES(1,2,3); +INSERT INTO ref VALUES(1,2,3); +BEGIN; +DELETE FROM pt; +DELETE FROM ref; +ABORT; +DROP TABLE pt, ref; + DROP SCHEMA fkpart9 CASCADE; -- Verify ON UPDATE/DELETE behavior