From 3651cbf884f5a459c099fb743cd478803610b53a Mon Sep 17 00:00:00 2001 From: Amul Sul Date: Fri, 17 Jan 2025 17:03:59 +0530 Subject: [PATCH v10 7/7] Merge the parent and child constraints with differing enforcibility. If an enforced parent constraint is attached to a non-enforced child constraint, the child constraint will be made enforced, with validation applied if the parent constraint is validated as well. Otherwise, a new enforced constraint (with validation, if the parent constraint is validated) would need to be created on the child table, which would be unnecessary if a similar constraint already exists and can be attached. On the other hand, having a not-enforced parent constraint with an enforced child constraint does not cause any issues, and no changes are required. ---- NOTE: This patch is intended to reduce the diff noise from the main patch and is not meant to be committed separately. It should be squashed with the main patch that adds ENFORCED/NOT ENFORCED. ---- --- src/backend/commands/tablecmds.c | 161 ++++++++++++++++++++-- src/test/regress/expected/foreign_key.out | 36 ++++- src/test/regress/sql/foreign_key.sql | 14 ++ 3 files changed, 195 insertions(+), 16 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 5c1b44c387d..d6bf11ae53a 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -11443,7 +11443,6 @@ tryAttachPartitionForeignKey(List **wqueue, if (OidIsValid(partConstr->conparentid) || partConstr->condeferrable != parentConstr->condeferrable || partConstr->condeferred != parentConstr->condeferred || - partConstr->conenforced != parentConstr->conenforced || partConstr->confupdtype != parentConstr->confupdtype || partConstr->confdeltype != parentConstr->confdeltype || partConstr->confmatchtype != parentConstr->confmatchtype) @@ -11488,7 +11487,9 @@ AttachPartitionForeignKey(List **wqueue, Oid partConstrRelid; bool parentConstrIsEnforced; bool parentConstrIsValid; + bool partConstrIsEnforced; bool partConstrIsValid; + bool partConstrParentIsSet; Oid insertTriggerOid, updateTriggerOid; @@ -11507,10 +11508,80 @@ AttachPartitionForeignKey(List **wqueue, if (!HeapTupleIsValid(partcontup)) elog(ERROR, "cache lookup failed for constraint %u", partConstrOid); partConstr = (Form_pg_constraint) GETSTRUCT(partcontup); + partConstrIsEnforced = partConstr->conenforced; partConstrIsValid = partConstr->convalidated; partConstrFrelid = partConstr->confrelid; partConstrRelid = partConstr->conrelid; + /* + * When the enforcibility of both constraints is the same, no special + * action is taken, and the flow remains unchanged. However, there are two + * scenarios to consider when it differs: + * + * 1. Parent constraint is not-enforced and child constraint is enforced: + * + * This is allowed because the not-enforced parent constraint does not + * have triggers, so no redundancy issues arise with the child constraint, + * even if it is enforced. In this case, we allow the child constraint to + * remain enforced and keep its trigger intact. This ensures that + * referential integrity checks for the child continue as before, even + * with the parent constraint not enforced. The relationship between the + * two constraints is maintained by setting the parent constraint, which + * enables us to locate the child constraint. This is important if the + * parent constraint is later changed to enforced, at which point the + * necessary trigger will be created for the parent. Any potential + * redundancy from these triggers will be addressed accordingly. + * + * 2. Parent constraint is enforced and child constraint is not-enforced: + * + * This is not allowed, as it would violate referential integrity. In such + * cases, the child constraint will first be made enforced before merging + * it with the enforced parent constraint. Then, action triggers and + * constraint redundancy will be handled in the usual manner, similar to + * how two enforced constraints are merged. + */ + if (!parentConstrIsEnforced && partConstrIsEnforced) + { + ReleaseSysCache(partcontup); + ReleaseSysCache(parentConstrTup); + + ConstraintSetParentConstraint(partConstrOid, parentConstrOid, + RelationGetRelid(partition)); + CommandCounterIncrement(); + + return; + } + else if (parentConstrIsEnforced && !partConstrIsEnforced) + { + Constraint *cmdcon = makeNode(Constraint); + Relation conrel; + List *otherrelids = NIL; + + cmdcon->contype = partConstr->contype; + cmdcon->conname = NameStr(partConstr->conname); + cmdcon->deferrable = partConstr->condeferrable; + cmdcon->initdeferred = partConstr->condeferred; + cmdcon->is_enforced = true; + + conrel = table_open(ConstraintRelationId, RowExclusiveLock); + + ATExecAlterConstrRecurse(cmdcon, conrel, trigrel, partConstr->conrelid, + partConstr->confrelid, partcontup, &otherrelids, + AccessExclusiveLock, InvalidOid, InvalidOid, + InvalidOid, InvalidOid); + + table_close(conrel, RowExclusiveLock); + + CommandCounterIncrement(); + } + + /* + * The constraint parent shouldn't be set beforehand, or if it's already + * set, it should be the specified parent. + */ + partConstrParentIsSet = OidIsValid(partConstr->conparentid); + Assert(!partConstrParentIsSet || partConstr->conparentid == parentConstrOid); + ReleaseSysCache(partcontup); ReleaseSysCache(parentConstrTup); @@ -11523,8 +11594,10 @@ AttachPartitionForeignKey(List **wqueue, DropForeignKeyConstraintTriggers(trigrel, partConstrOid, partConstrFrelid, partConstrRelid); - ConstraintSetParentConstraint(partConstrOid, parentConstrOid, - RelationGetRelid(partition)); + /* Skip if the parent is already set */ + if (!partConstrParentIsSet) + ConstraintSetParentConstraint(partConstrOid, parentConstrOid, + RelationGetRelid(partition)); /* * Like the constraint, attach partition's "check" triggers to the @@ -11569,7 +11642,7 @@ AttachPartitionForeignKey(List **wqueue, * validate the child data. Conversely, having an invalid parent * constraint while the child constraint is valid doesn't cause any harm. */ - if (parentConstrIsValid && !partConstrIsValid); + if (wqueue && parentConstrIsValid && !partConstrIsValid) { Relation conrel; @@ -12156,6 +12229,17 @@ ATExecAlterConstrEnforceability(Constraint *cmdcon, Relation conrel, /* Drop all the triggers */ DropForeignKeyConstraintTriggers(tgrel, conoid, InvalidOid, InvalidOid); + + /* + * If the referenced table is partitioned, the child constraint we're + * changing to not-enforced may have additional pg_constraint rows and + * action triggers that remain untouched while this child constraint + * is attached to the not-enforced parent. These must now be removed. + * For more details, see AttachPartitionForeignKey(). + */ + if (OidIsValid(currcon->conparentid) && + get_rel_relkind(currcon->confrelid) == RELKIND_PARTITIONED_TABLE) + RemoveInheritedConstraint(conrel, tgrel, currcon->conrelid, conoid); } else /* Create triggers */ { @@ -12214,13 +12298,46 @@ ATExecAlterConstrEnforceability(Constraint *cmdcon, Relation conrel, true, NULL, 1, &pkey); while (HeapTupleIsValid(childtup = systable_getnext(pscan))) - ATExecAlterConstrRecurse(cmdcon, conrel, tgrel, fkrelid, pkrelid, - childtup, otherrelids, lockmode, - ReferencedDelTriggerOid, - ReferencedUpdTriggerOid, - ReferencingInsTriggerOid, - ReferencingUpdTriggerOid); + { + Form_pg_constraint childcon; + childcon = (Form_pg_constraint) GETSTRUCT(childtup); + + if (childcon->conenforced) + { + + /* + * The child constraint is attached to the parent + * constraint, which is already enforced. Now, as the + * parent constraint is being modified to be enforced, some + * constraints and action triggers on the child table may + * become redundant and need to be removed. + */ + if (currcon->confrelid == pkrelid) + { + /* + * The lock on the relation will be held by upper + * callers, so it's fine to open it with no lock here. + */ + Relation rel = table_open(currcon->conrelid, NoLock); + + AttachPartitionForeignKey(NULL, rel, childcon->oid, + conoid, + ReferencingInsTriggerOid, + ReferencingUpdTriggerOid, + tgrel); + + table_close(rel, NoLock); + } + } + else + ATExecAlterConstrRecurse(cmdcon, conrel, tgrel, fkrelid, + pkrelid, childtup, otherrelids, + lockmode, ReferencedDelTriggerOid, + ReferencedUpdTriggerOid, + ReferencingInsTriggerOid, + ReferencingUpdTriggerOid); + } systable_endscan(pscan); } } @@ -20347,7 +20464,9 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, { ForeignKeyCacheInfo *fk = lfirst(cell); HeapTuple contup; + HeapTuple parentContup; Form_pg_constraint conform; + Oid parentConstrIsEnforced; contup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid)); if (!HeapTupleIsValid(contup)) @@ -20362,12 +20481,34 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, continue; } + /* Get the enforcibility of the parent constraint */ + parentContup = SearchSysCache1(CONSTROID, + ObjectIdGetDatum(conform->conparentid)); + if (!HeapTupleIsValid(parentContup)) + elog(ERROR, "cache lookup failed for constraint %u", + conform->conparentid); + parentConstrIsEnforced = + ((Form_pg_constraint) GETSTRUCT(parentContup))->conenforced; + ReleaseSysCache(parentContup); + /* * The constraint on this table must be marked no longer a child of * the parent's constraint, as do its check triggers. */ ConstraintSetParentConstraint(fk->conoid, InvalidOid, InvalidOid); + /* + * Unsetting the parent is sufficient when the parent constraint is + * not enforced and the child constraint is enforced, as we link them + * by setting the constraint parent, while leaving the rest unchanged. + * For more details, see AttachPartitionForeignKey(). + */ + if (!parentConstrIsEnforced && fk->conenforced) + { + ReleaseSysCache(contup); + continue; + } + /* * Also, look up the partition's "check" triggers corresponding to the * enforced constraint being detached and detach them from the parent diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 9f6f22dd988..d894beaf1c0 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -1713,8 +1713,33 @@ ALTER TABLE fk_partitioned_fk_3 DROP COLUMN fdrop1, DROP COLUMN fdrop2, DROP COLUMN fdrop3, DROP COLUMN fdrop4; CREATE TABLE fk_partitioned_fk_3_0 PARTITION OF fk_partitioned_fk_3 FOR VALUES WITH (MODULUS 5, REMAINDER 0); CREATE TABLE fk_partitioned_fk_3_1 PARTITION OF fk_partitioned_fk_3 FOR VALUES WITH (MODULUS 5, REMAINDER 1); +-- Merge the not-enforced parent constraint with the enforced and not-enforced child constraints. +ALTER TABLE fk_partitioned_fk_3_0 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk ENFORCED; +ALTER TABLE fk_partitioned_fk_3_1 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT ENFORCED; +ALTER TABLE fk_partitioned_fk_3 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT ENFORCED; +SELECT conname, conenforced, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk_3%' ORDER BY oid; + conname | conenforced | convalidated | conrelid +--------------------------------+-------------+--------------+----------------------- + fk_partitioned_fk_3_0_a_b_fkey | t | t | fk_partitioned_fk_3_0 + fk_partitioned_fk_3_1_a_b_fkey | f | f | fk_partitioned_fk_3_1 + fk_partitioned_fk_3_a_b_fkey | f | f | fk_partitioned_fk_3 +(3 rows) + +-- Merging an enforced parent constraint (validated) with a not-enforced child +-- constraint will implicitly change the child constraint to enforced and apply +-- the validation as well. ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_3 FOR VALUES FROM (2000,2000) TO (3000,3000); +SELECT conname, conenforced, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk_3%' ORDER BY oid; + conname | conenforced | convalidated | conrelid +--------------------------------+-------------+--------------+----------------------- + fk_partitioned_fk_3_0_a_b_fkey | t | t | fk_partitioned_fk_3_0 + fk_partitioned_fk_3_1_a_b_fkey | t | t | fk_partitioned_fk_3_1 + fk_partitioned_fk_3_a_b_fkey | t | t | fk_partitioned_fk_3 +(3 rows) + -- Creating a foreign key with ONLY on a partitioned table referencing -- a non-partitioned table fails. ALTER TABLE ONLY fk_partitioned_fk ADD FOREIGN KEY (a, b) @@ -1735,16 +1760,16 @@ INSERT INTO fk_partitioned_fk_2 (a,b) VALUES (1500, 1501); ERROR: insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_2_a_b_fkey" DETAIL: Key (a, b)=(1500, 1501) is not present in table "fk_notpartitioned_pk". INSERT INTO fk_partitioned_fk (a,b) VALUES (2500, 2502); -ERROR: insert or update on table "fk_partitioned_fk_3_1" violates foreign key constraint "fk_partitioned_fk_a_b_fkey" +ERROR: insert or update on table "fk_partitioned_fk_3_1" violates foreign key constraint "fk_partitioned_fk_3_1_a_b_fkey" DETAIL: Key (a, b)=(2500, 2502) is not present in table "fk_notpartitioned_pk". INSERT INTO fk_partitioned_fk_3 (a,b) VALUES (2500, 2502); -ERROR: insert or update on table "fk_partitioned_fk_3_1" violates foreign key constraint "fk_partitioned_fk_a_b_fkey" +ERROR: insert or update on table "fk_partitioned_fk_3_1" violates foreign key constraint "fk_partitioned_fk_3_1_a_b_fkey" DETAIL: Key (a, b)=(2500, 2502) is not present in table "fk_notpartitioned_pk". INSERT INTO fk_partitioned_fk (a,b) VALUES (2501, 2503); -ERROR: insert or update on table "fk_partitioned_fk_3_0" violates foreign key constraint "fk_partitioned_fk_a_b_fkey" +ERROR: insert or update on table "fk_partitioned_fk_3_0" violates foreign key constraint "fk_partitioned_fk_3_0_a_b_fkey" DETAIL: Key (a, b)=(2501, 2503) is not present in table "fk_notpartitioned_pk". INSERT INTO fk_partitioned_fk_3 (a,b) VALUES (2501, 2503); -ERROR: insert or update on table "fk_partitioned_fk_3_0" violates foreign key constraint "fk_partitioned_fk_a_b_fkey" +ERROR: insert or update on table "fk_partitioned_fk_3_0" violates foreign key constraint "fk_partitioned_fk_3_0_a_b_fkey" DETAIL: Key (a, b)=(2501, 2503) is not present in table "fk_notpartitioned_pk". -- but if we insert the values that make them valid, then they work INSERT INTO fk_notpartitioned_pk VALUES (500, 501), (1500, 1501), @@ -1755,7 +1780,7 @@ INSERT INTO fk_partitioned_fk (a,b) VALUES (2500, 2502); INSERT INTO fk_partitioned_fk (a,b) VALUES (2501, 2503); -- this update fails because there is no referenced row UPDATE fk_partitioned_fk SET a = a + 1 WHERE a = 2501; -ERROR: insert or update on table "fk_partitioned_fk_3_1" violates foreign key constraint "fk_partitioned_fk_a_b_fkey" +ERROR: insert or update on table "fk_partitioned_fk_3_1" violates foreign key constraint "fk_partitioned_fk_3_1_a_b_fkey" DETAIL: Key (a, b)=(2502, 2503) is not present in table "fk_notpartitioned_pk". -- but we can fix it thusly: INSERT INTO fk_notpartitioned_pk (a,b) VALUES (2502, 2503); @@ -2030,7 +2055,6 @@ ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES IN a | integer | | | Partition of: fk_partitioned_fk FOR VALUES IN (1500, 1502) Foreign-key constraints: - "fk_partitioned_fk_2_a_b_fkey" FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk(a, b) ON UPDATE CASCADE ON DELETE CASCADE NOT ENFORCED TABLE "fk_partitioned_fk" CONSTRAINT "fk_partitioned_fk_a_b_fkey" FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk(a, b) ON UPDATE CASCADE ON DELETE CASCADE DROP TABLE fk_partitioned_fk_2; diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 98f47873d41..8dda6b6bbae 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -1295,9 +1295,23 @@ ALTER TABLE fk_partitioned_fk_3 DROP COLUMN fdrop1, DROP COLUMN fdrop2, DROP COLUMN fdrop3, DROP COLUMN fdrop4; CREATE TABLE fk_partitioned_fk_3_0 PARTITION OF fk_partitioned_fk_3 FOR VALUES WITH (MODULUS 5, REMAINDER 0); CREATE TABLE fk_partitioned_fk_3_1 PARTITION OF fk_partitioned_fk_3 FOR VALUES WITH (MODULUS 5, REMAINDER 1); +-- Merge the not-enforced parent constraint with the enforced and not-enforced child constraints. +ALTER TABLE fk_partitioned_fk_3_0 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk ENFORCED; +ALTER TABLE fk_partitioned_fk_3_1 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT ENFORCED; +ALTER TABLE fk_partitioned_fk_3 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT ENFORCED; + +SELECT conname, conenforced, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk_3%' ORDER BY oid; + +-- Merging an enforced parent constraint (validated) with a not-enforced child +-- constraint will implicitly change the child constraint to enforced and apply +-- the validation as well. ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_3 FOR VALUES FROM (2000,2000) TO (3000,3000); +SELECT conname, conenforced, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk_3%' ORDER BY oid; + -- Creating a foreign key with ONLY on a partitioned table referencing -- a non-partitioned table fails. ALTER TABLE ONLY fk_partitioned_fk ADD FOREIGN KEY (a, b) -- 2.43.5