On 2019/01/18 7:54, Alvaro Herrera wrote: > On 2019-Jan-09, Amit Langote wrote: > >> 1. Foreign keys of partitions stop working correctly after being detached >> from the parent table > >> This happens because the action triggers defined on the PK relation (pk) >> refers to p as the referencing relation. On detaching p1 from p, p1's >> data is no longer accessible to that trigger. > > Ouch. > >> To fix this problem, we need create action triggers on PK relation >> that refer to p1 when it's detached (unless such triggers already >> exist which might be true in some cases). Attached patch 0001 shows >> this approach. > > Hmm, okay. I'm not in love with the idea that such triggers might > already exist -- seems unclean. We should remove redundant action > triggers when we attach a table as a partition, no?
OK, I agree. I have updated the patch to make things work that way. With the patch: create table pk (a int primary key); create table p (a int references pk) partition by list (a); -- this query shows the action triggers on the referenced rel ('pk'), name -- of the constraint that the trigger is part of and the foreign key rel -- ('p', etc.) select tgrelid::regclass as pkrel, c.conname as fkconname, tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where tgrelid = 'pk'::regclass and tgconstraint = c.oid; pkrel │ fkconname │ fkrel ───────┼───────────┼─────── pk │ p_a_fkey │ p pk │ p_a_fkey │ p (2 rows) create table p1 ( a int references pk, foreign key (a) references pk (a) ON UPDATE CASCADE ON DELETE CASCADE DEFERRABLE, foreign key (a) references pk (a) MATCH FULL ON UPDATE CASCADE ON DELETE CASCADE ) partition by list (a); -- p1_a_fkey on 'p1' is equivalent to p_a_fkey on 'p', but they're not -- attached yet select tgrelid::regclass as pkrel, conname as fkconname, tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where tgrelid = 'pk'::regclass and tgconstraint = c.oid; pkrel │ fkconname │ fkrel ───────┼────────────┼─────── pk │ p_a_fkey │ p pk │ p_a_fkey │ p pk │ p1_a_fkey │ p1 pk │ p1_a_fkey │ p1 pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey2 │ p1 pk │ p1_a_fkey2 │ p1 (8 rows) create table p11 (like p1, foreign key (a) references pk); -- again, p11_a_fkey, p1_a_fkey, and p_a_fkey are equivalent select tgrelid::regclass as pkrel, conname as fkconname, tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where tgrelid = 'pk'::regclass and tgconstraint = c.oid; pkrel │ fkconname │ fkrel ───────┼────────────┼─────── pk │ p_a_fkey │ p pk │ p_a_fkey │ p pk │ p1_a_fkey │ p1 pk │ p1_a_fkey │ p1 pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey2 │ p1 pk │ p1_a_fkey2 │ p1 pk │ p11_a_fkey │ p11 pk │ p11_a_fkey │ p11 (10 rows) alter table p1 attach partition p11 for values in (1); -- p1_a_fkey and p11_a_fkey merged, so triggers for the latter dropped select tgrelid::regclass as pkrel, conname as fkconname, tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where tgrelid = 'pk'::regclass and tgconstraint = c.oid; pkrel │ fkconname │ fkrel ───────┼────────────┼─────── pk │ p_a_fkey │ p pk │ p_a_fkey │ p pk │ p1_a_fkey │ p1 pk │ p1_a_fkey │ p1 pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey2 │ p1 pk │ p1_a_fkey2 │ p1 (8 rows) -- p_a_fkey and p1_a_fkey merged, so triggers for the latter dropped alter table p attach partition p1 for values in (1); select tgrelid::regclass as pkrel, conname as fkconname, tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where tgrelid = 'pk'::regclass and tgconstraint = c.oid; pkrel │ fkconname │ fkrel ───────┼────────────┼─────── pk │ p_a_fkey │ p pk │ p_a_fkey │ p pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey2 │ p1 pk │ p1_a_fkey2 │ p1 (6 rows) alter table p detach partition p1; -- p1_a_fkey needs its own triggers again select tgrelid::regclass as pkrel, conname as fkconname, tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where tgrelid = 'pk'::regclass and tgconstraint = c.oid; pkrel │ fkconname │ fkrel ───────┼────────────┼─────── pk │ p_a_fkey │ p pk │ p_a_fkey │ p pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey2 │ p1 pk │ p1_a_fkey2 │ p1 pk │ p1_a_fkey │ p1 pk │ p1_a_fkey │ p1 (8 rows) alter table p1 detach partition p11; -- p11_a_fkey needs its own triggers again select tgrelid::regclass as pkrel, conname as fkconname, tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where tgrelid = 'pk'::regclass and tgconstraint = c.oid; pkrel │ fkconname │ fkrel ───────┼────────────┼─────── pk │ p_a_fkey │ p pk │ p_a_fkey │ p pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey2 │ p1 pk │ p1_a_fkey2 │ p1 pk │ p1_a_fkey │ p1 pk │ p1_a_fkey │ p1 pk │ p11_a_fkey │ p11 pk │ p11_a_fkey │ p11 pk │ p1_a_fkey1 │ p11 pk │ p1_a_fkey1 │ p11 pk │ p1_a_fkey2 │ p11 pk │ p1_a_fkey2 │ p11 (14 rows) -- try again alter table p1 attach partition p11 for values in (1); select tgrelid::regclass as pkrel, conname as fkconname, tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where tgrelid = 'pk'::regclass and tgconstraint = c.oid; pkrel │ fkconname │ fkrel ───────┼────────────┼─────── pk │ p_a_fkey │ p pk │ p_a_fkey │ p pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey2 │ p1 pk │ p1_a_fkey2 │ p1 pk │ p1_a_fkey │ p1 pk │ p1_a_fkey │ p1 (8 rows) alter table p attach partition p1 for values in (1); select tgrelid::regclass as pkrel, conname as fkconname, tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where tgrelid = 'pk'::regclass and tgconstraint = c.oid; pkrel │ fkconname │ fkrel ───────┼────────────┼─────── pk │ p_a_fkey │ p pk │ p_a_fkey │ p pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey1 │ p1 pk │ p1_a_fkey2 │ p1 pk │ p1_a_fkey2 │ p1 (6 rows) By the way, I also noticed that there's duplicated code in clone_fk_constraints() which 0001 gets rid of: datum = fastgetattr(tuple, Anum_pg_constraint_conpfeqop, tupdesc, &isnull); if (isnull) elog(ERROR, "null conpfeqop"); arr = DatumGetArrayTypeP(datum); nelem = ARR_DIMS(arr)[0]; if (ARR_NDIM(arr) != 1 || nelem < 1 || nelem > INDEX_MAX_KEYS || ARR_HASNULL(arr) || ARR_ELEMTYPE(arr) != OIDOID) elog(ERROR, "conpfeqop is not a 1-D OID array"); memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid)); - datum = fastgetattr(tuple, Anum_pg_constraint_conpfeqop, - tupdesc, &isnull); - if (isnull) - elog(ERROR, "null conpfeqop"); - arr = DatumGetArrayTypeP(datum); - nelem = ARR_DIMS(arr)[0]; - if (ARR_NDIM(arr) != 1 || - nelem < 1 || - nelem > INDEX_MAX_KEYS || - ARR_HASNULL(arr) || - ARR_ELEMTYPE(arr) != OIDOID) - elog(ERROR, "conpfeqop is not a 1-D OID array"); - memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid)); - I know you're working on a bug fix in the thread on pgsql-bugs which is related to the patch 0002 here, but attaching it here anyway, because it proposes to get rid of the needless dependencies which I didn't see mentioned on the other thread. Also, updated 0001 needed it to be rebased. Like the last time, I've also attached the patches that can be applied PG11 branch. Thanks, Amit
From 9c877b0cb1b8dbe7f6957b5b1256686f3690dee5 Mon Sep 17 00:00:00 2001 From: amit <amitlangot...@gmail.com> Date: Fri, 18 Jan 2019 14:13:11 +0900 Subject: [PATCH v2 1/2] Ensure PK-side action triggers for partitions after being detached Detaching a partition from the parent whose foreign key(s) would've been duplicated in the partition makes the foreign key(s) stop working corretly. That's because PK-side action trigger for the foreign key would refer to the parent as the referencing relation and after the partition is detached it's data is no longer accessible via parent. So while the check triggers that remain even after being detached takes care of the one side of enforcing the foreign key of the detached partition, lack of corresponding PK-side action trigger to check detached partition's data means the other side doesn't work. To fix, add the action triggers on the PK relation that point to the detached partition when detaching. --- src/backend/catalog/pg_constraint.c | 74 +++++++++++++++++++++++++++++-------- src/backend/commands/tablecmds.c | 42 +++++++++++++++++++-- src/include/commands/tablecmds.h | 3 +- 3 files changed, 98 insertions(+), 21 deletions(-) diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index f4057a9f15..9df6540ac3 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -26,6 +26,7 @@ #include "catalog/partition.h" #include "catalog/pg_constraint.h" #include "catalog/pg_operator.h" +#include "catalog/pg_trigger.h" #include "catalog/pg_type.h" #include "commands/defrem.h" #include "commands/tablecmds.h" @@ -551,20 +552,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, elog(ERROR, "conpfeqop is not a 1-D OID array"); memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid)); - datum = fastgetattr(tuple, Anum_pg_constraint_conpfeqop, - tupdesc, &isnull); - if (isnull) - elog(ERROR, "null conpfeqop"); - arr = DatumGetArrayTypeP(datum); - nelem = ARR_DIMS(arr)[0]; - if (ARR_NDIM(arr) != 1 || - nelem < 1 || - nelem > INDEX_MAX_KEYS || - ARR_HASNULL(arr) || - ARR_ELEMTYPE(arr) != OIDOID) - elog(ERROR, "conpfeqop is not a 1-D OID array"); - memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid)); - datum = fastgetattr(tuple, Anum_pg_constraint_conppeqop, tupdesc, &isnull); if (isnull) @@ -607,6 +594,10 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, ForeignKeyCacheInfo *fk = lfirst_node(ForeignKeyCacheInfo, cell); Form_pg_constraint partConstr; HeapTuple partcontup; + Relation trigrel; + HeapTuple trigtup; + HeapScanDesc scan; + ScanKeyData key[3]; attach_it = true; @@ -658,7 +649,57 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, ReleaseSysCache(partcontup); - /* looks good! Attach this constraint */ + /* + * looks good! Attach this constraint. Drop the action triggers + * on the PK side that refer to this partition rel as part of + * this constraint, because they will be wasteful after attaching + * this constraint to the parent constraint. After being attached, + * action triggers corresponding to the parent constraint can + * indirectly refer to this partition's data by referencing the + * parent relation. + */ + trigrel = heap_open(TriggerRelationId, AccessShareLock); + ScanKeyInit(&key[0], + Anum_pg_trigger_tgrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(fk->confrelid)); + ScanKeyInit(&key[1], + Anum_pg_trigger_tgconstrrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(fk->conrelid)); + ScanKeyInit(&key[2], + Anum_pg_trigger_tgconstraint, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(fk->conoid)); + scan = heap_beginscan_catalog(trigrel, 3, key); + + /* + * There should be two tuples to be deleted corresponding to + * the two action triggers that createForeignKeyActionTriggers + * would have created. + */ + trigtup = heap_getnext(scan, ForwardScanDirection); + Assert(trigtup != NULL); + deleteDependencyRecordsForClass(TriggerRelationId, + HeapTupleGetOid(trigtup), + ConstraintRelationId, + DEPENDENCY_INTERNAL); + CatalogTupleDelete(trigrel, &trigtup->t_self); + + trigtup = heap_getnext(scan, ForwardScanDirection); + Assert(trigtup != NULL); + deleteDependencyRecordsForClass(TriggerRelationId, + HeapTupleGetOid(trigtup), + ConstraintRelationId, + DEPENDENCY_INTERNAL); + CatalogTupleDelete(trigrel, &trigtup->t_self); + + trigtup = heap_getnext(scan, ForwardScanDirection); + Assert(trigtup == NULL); + + heap_endscan(scan); + heap_close(trigrel, AccessShareLock); + ConstraintSetParentConstraint(fk->conoid, HeapTupleGetOid(tuple)); CommandCounterIncrement(); @@ -720,7 +761,8 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, fkconstraint->initdeferred = constrForm->condeferred; createForeignKeyTriggers(partRel, constrForm->confrelid, fkconstraint, - constrOid, constrForm->conindid, false); + constrOid, constrForm->conindid, false, + true); if (cloned) { diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 71b2e3f134..d668a57ac2 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7718,7 +7718,7 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * though the constraints also exist below. */ createForeignKeyTriggers(rel, RelationGetRelid(pkrel), fkconstraint, - constrOid, indexOid, !recursing); + constrOid, indexOid, !recursing, true); /* * Tell Phase 3 to check that the constraint is satisfied by existing @@ -8827,7 +8827,8 @@ createForeignKeyCheckTriggers(Oid myRelOid, Oid refRelOid, */ void createForeignKeyTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint, - Oid constraintOid, Oid indexOid, bool create_action) + Oid constraintOid, Oid indexOid, bool create_action, + bool create_check) { /* * For the referenced side, create action triggers, if requested. (If the @@ -8843,7 +8844,7 @@ createForeignKeyTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint, * For the referencing side, create the check triggers. We only need * these on the partitions. */ - if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE && create_check) createForeignKeyCheckTriggers(RelationGetRelid(rel), refRelOid, fkconstraint, constraintOid, indexOid); @@ -14887,19 +14888,52 @@ ATExecDetachPartition(Relation rel, RangeVar *name) } heap_close(classRel, RowExclusiveLock); - /* Detach foreign keys */ + /* Detach any foreign keys that are inherited */ fks = copyObject(RelationGetFKeyList(partRel)); foreach(cell, fks) { ForeignKeyCacheInfo *fk = lfirst(cell); HeapTuple contup; + Form_pg_constraint conform; + Constraint *fkconstraint; contup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid)); if (!contup) elog(ERROR, "cache lookup failed for constraint %u", fk->conoid); + conform = (Form_pg_constraint) GETSTRUCT(contup); + /* consider only the inherited foreign keys */ + if (conform->contype != CONSTRAINT_FOREIGN || + !OidIsValid(conform->conparentid)) + { + ReleaseSysCache(contup); + continue; + } + + /* unset conparentid and adjust conislocal, coninhcount, etc. */ ConstraintSetParentConstraint(fk->conoid, InvalidOid); + /* + * We'll need to make the action triggers on primary key relation that + * point to this relation as the FK relation. We need to do this, + * because no PK-side triggers exist for an inherited FK constraint. + * Now that we've detached it from the parent, we'd our own. + */ + fkconstraint = makeNode(Constraint); + fkconstraint->conname = pstrdup(NameStr(conform->conname)); + fkconstraint->fk_upd_action = conform->confupdtype; + fkconstraint->fk_del_action = conform->confdeltype; + fkconstraint->deferrable = conform->condeferrable; + fkconstraint->initdeferred = conform->condeferred; + + /* + * As we already got the check triggers, no need to recreate them, + * so pass false for create_check. + */ + createForeignKeyTriggers(partRel, conform->confrelid, fkconstraint, + fk->conoid, conform->conindid, + true, false); + ReleaseSysCache(contup); } list_free_deep(fks); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 138de84e83..dd985a80b6 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -77,7 +77,8 @@ extern void check_of_type(HeapTuple typetuple); extern void createForeignKeyTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint, Oid constraintOid, - Oid indexOid, bool create_action); + Oid indexOid, bool create_action, + bool create_check); extern void register_on_commit_action(Oid relid, OnCommitAction action); extern void remove_on_commit_action(Oid relid); -- 2.11.0
From 61c03e3435eb10304361ca093be7687f56463b9f Mon Sep 17 00:00:00 2001 From: amit <amitlangot...@gmail.com> Date: Fri, 18 Jan 2019 14:20:13 +0900 Subject: [PATCH v2 2/2] Do not track foreign key inheritance by dependencies Inheritance information maintained in pg_constraint is enough to prevent a child constraint to be dropped and for them to be dropped when the parent constraint is dropped. So, do not create dependencies between the parent foreign key constraint and its children. Also, fix ATAddForeignKeyConstraint to set inheritance information correctly. --- src/backend/catalog/pg_constraint.c | 33 ++++++++---------------- src/backend/commands/indexcmds.c | 17 +++++++++++++ src/backend/commands/tablecmds.c | 50 ++++++++++++++++++++++++++----------- 3 files changed, 64 insertions(+), 36 deletions(-) diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 9df6540ac3..0048b3a436 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -483,8 +483,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, Constraint *fkconstraint; bool attach_it; Oid constrOid; - ObjectAddress parentAddr, - childAddr; int nelem; ListCell *cell; int i; @@ -505,8 +503,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, continue; } - ObjectAddressSet(parentAddr, ConstraintRelationId, parentConstrOid); - datum = fastgetattr(tuple, Anum_pg_constraint_conkey, tupdesc, &isnull); if (isnull) @@ -749,9 +745,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, 1, false, true); subclone = lappend_oid(subclone, constrOid); - ObjectAddressSet(childAddr, ConstraintRelationId, constrOid); - recordDependencyOn(&childAddr, &parentAddr, DEPENDENCY_INTERNAL_AUTO); - fkconstraint = makeNode(Constraint); /* for now this is all we need */ fkconstraint->conname = pstrdup(NameStr(constrForm->conname)); @@ -1203,8 +1196,8 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId, * ConstraintSetParentConstraint * Set a partition's constraint as child of its parent table's * - * This updates the constraint's pg_constraint row to show it as inherited, and - * add a dependency to the parent so that it cannot be removed on its own. + * This updates the constraint's pg_constraint row to change its + * inheritance properties. */ void ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId) @@ -1213,8 +1206,6 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId) Form_pg_constraint constrForm; HeapTuple tuple, newtup; - ObjectAddress depender; - ObjectAddress referenced; constrRel = heap_open(ConstraintRelationId, RowExclusiveLock); tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(childConstrId)); @@ -1226,25 +1217,23 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId) { constrForm->conislocal = false; constrForm->coninhcount++; + /* + * An inherited foreign key constraint can never have more than one + * parent, because inheriting foreign keys is only allowed for + * partitioning where multiple inheritance is disallowed. + */ + Assert(constrForm->coninhcount == 1); constrForm->conparentid = parentConstrId; CatalogTupleUpdate(constrRel, &tuple->t_self, newtup); - - ObjectAddressSet(referenced, ConstraintRelationId, parentConstrId); - ObjectAddressSet(depender, ConstraintRelationId, childConstrId); - - recordDependencyOn(&depender, &referenced, DEPENDENCY_INTERNAL_AUTO); } else { constrForm->coninhcount--; - if (constrForm->coninhcount <= 0) - constrForm->conislocal = true; + /* See the above comment. */ + Assert(constrForm->coninhcount == 0); + constrForm->conislocal = true; constrForm->conparentid = InvalidOid; - - deleteDependencyRecordsForClass(ConstraintRelationId, childConstrId, - ConstraintRelationId, - DEPENDENCY_INTERNAL_AUTO); CatalogTupleUpdate(constrRel, &tuple->t_self, newtup); } diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index fec5bc5dd6..1b497b5bd8 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -974,8 +974,25 @@ DefineIndex(Oid relationId, /* Attach index to parent and we're done. */ IndexSetParentIndex(cldidx, indexRelationId); if (createdConstraintId != InvalidOid) + { + ObjectAddress depender; + ObjectAddress referenced; + ConstraintSetParentConstraint(cldConstrOid, createdConstraintId); + /* + * Need to set an explicit dependency in this + * case unlike other types of constraints where + * the child constraint gets dropped due to + * inheritance recursion. + */ + ObjectAddressSet(referenced, ConstraintRelationId, + createdConstraintId); + ObjectAddressSet(depender, ConstraintRelationId, + cldConstrOid); + recordDependencyOn(&depender, &referenced, + DEPENDENCY_INTERNAL_AUTO); + } if (!IndexIsValid(cldidx->rd_index)) invalidate_parent = true; diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d668a57ac2..d045b4eddb 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7342,6 +7342,9 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, bool old_check_ok; ObjectAddress address; ListCell *old_pfeqop_item = list_head(fkconstraint->old_conpfeqop); + bool conislocal = true; + int coninhcount = 0; + bool connoinherit = true; /* * Grab ShareRowExclusiveLock on the pk table, so that someone doesn't @@ -7680,6 +7683,26 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, /* * Record the FK constraint in pg_constraint. */ + + /* + * Set the inheritance related properties correctly if it's being + * recursively added for a partition. + */ + if (OidIsValid(parentConstr)) + { + /* Foreign keys are inherited only for partitioning. */ + Assert(rel->rd_rel->relispartition); + conislocal = false; + /* Partitions can have only one parent. */ + coninhcount = 1; + /* Make sure that the constraint can be further inherited. */ + connoinherit = false; + } + + /* If partitioned table, constraint must be marked as inheritable. */ + if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + connoinherit = false; + constrOid = CreateConstraintEntry(fkconstraint->conname, RelationGetNamespace(rel), CONSTRAINT_FOREIGN, @@ -7706,9 +7729,9 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, NULL, /* no check constraint */ NULL, NULL, - true, /* islocal */ - 0, /* inhcount */ - true, /* isnoinherit */ + conislocal, + coninhcount, + connoinherit, false); /* is_internal */ ObjectAddressSet(address, ConstraintRelationId, constrOid); @@ -7757,20 +7780,15 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, Oid partitionId = partdesc->oids[i]; Relation partition = heap_open(partitionId, lockmode); AlteredTableInfo *childtab; - ObjectAddress childAddr; CheckTableNotInUse(partition, "ALTER TABLE"); /* Find or create work queue entry for this table */ childtab = ATGetQueueEntry(wqueue, partition); - childAddr = - ATAddForeignKeyConstraint(wqueue, childtab, partition, - fkconstraint, constrOid, - recurse, true, lockmode); - - /* Record this constraint as dependent on the parent one */ - recordDependencyOn(&childAddr, &address, DEPENDENCY_INTERNAL_AUTO); + ATAddForeignKeyConstraint(wqueue, childtab, partition, + fkconstraint, constrOid, + recurse, true, lockmode); heap_close(partition, NoLock); } @@ -9024,9 +9042,13 @@ ATExecDropConstraint(Relation rel, const char *constrName, con = (Form_pg_constraint) GETSTRUCT(copy_tuple); - /* Right now only CHECK constraints can be inherited */ - if (con->contype != CONSTRAINT_CHECK) - elog(ERROR, "inherited constraint is not a CHECK constraint"); + /* + * Right now only CHECK constraints and foreign key constraints can + * be inherited. + */ + if (con->contype != CONSTRAINT_CHECK && + con->contype != CONSTRAINT_FOREIGN) + elog(ERROR, "inherited constraint is not a CHECK constraint or a foreign key constraint"); if (con->coninhcount <= 0) /* shouldn't happen */ elog(ERROR, "relation %u has non-inherited constraint \"%s\"", -- 2.11.0
From d168bfa988bb8923fb138ceb4026ac005a7f580a Mon Sep 17 00:00:00 2001 From: amit <amitlangot...@gmail.com> Date: Wed, 9 Jan 2019 10:00:11 +0900 Subject: [PATCH v2 1/2] Ensure PK-side action triggers for partitions after being detached Detaching a partition from the parent whose foreign key(s) would've been duplicated in the partition makes the foreign key(s) stop working corretly. That's because PK-side action trigger for the foreign key would refer to the parent as the referencing relation and after the partition is detached it's data is no longer accessible via parent. So while the check triggers that remain even after being detached takes care of the one side of enforcing the foreign key of the detached partition, lack of corresponding PK-side action trigger to check detached partition's data means the other side doesn't work. To fix, add the action triggers on the PK relation that point to the detached partition when detaching. --- src/backend/catalog/pg_constraint.c | 74 +++++++++++++++++++++++++++++-------- src/backend/commands/tablecmds.c | 42 +++++++++++++++++++-- src/include/commands/tablecmds.h | 3 +- 3 files changed, 98 insertions(+), 21 deletions(-) diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 3c960c9423..3cac851447 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -27,6 +27,7 @@ #include "catalog/partition.h" #include "catalog/pg_constraint.h" #include "catalog/pg_operator.h" +#include "catalog/pg_trigger.h" #include "catalog/pg_type.h" #include "commands/defrem.h" #include "commands/tablecmds.h" @@ -547,20 +548,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, elog(ERROR, "conpfeqop is not a 1-D OID array"); memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid)); - datum = fastgetattr(tuple, Anum_pg_constraint_conpfeqop, - tupdesc, &isnull); - if (isnull) - elog(ERROR, "null conpfeqop"); - arr = DatumGetArrayTypeP(datum); - nelem = ARR_DIMS(arr)[0]; - if (ARR_NDIM(arr) != 1 || - nelem < 1 || - nelem > INDEX_MAX_KEYS || - ARR_HASNULL(arr) || - ARR_ELEMTYPE(arr) != OIDOID) - elog(ERROR, "conpfeqop is not a 1-D OID array"); - memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid)); - datum = fastgetattr(tuple, Anum_pg_constraint_conppeqop, tupdesc, &isnull); if (isnull) @@ -603,6 +590,11 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, ForeignKeyCacheInfo *fk = lfirst_node(ForeignKeyCacheInfo, cell); Form_pg_constraint partConstr; HeapTuple partcontup; + Relation trigrel; + HeapTuple trigtup; + Form_pg_trigger trigform; + HeapScanDesc scan; + ScanKeyData key[3]; attach_it = true; @@ -654,7 +646,56 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, ReleaseSysCache(partcontup); - /* looks good! Attach this constraint */ + /* + * looks good! Attach this constraint. Drop the action triggers + * on the PK side that refer to this partition rel as part of + * this constraint, because they will be wasteful after attaching + * this constraint to the parent constraint. After being attached, + * action triggers corresponding to the parent constraint can + * indirectly refer to this partition's data by referencing the + * parent relation. + */ + trigrel = heap_open(TriggerRelationId, AccessShareLock); + ScanKeyInit(&key[0], + Anum_pg_trigger_tgrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(fk->confrelid)); + ScanKeyInit(&key[1], + Anum_pg_trigger_tgconstrrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(fk->conrelid)); + ScanKeyInit(&key[2], + Anum_pg_trigger_tgconstraint, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(fk->conoid)); + scan = heap_beginscan_catalog(trigrel, 3, key); + + /* + * There should be two tuples to be deleted corresponding to + * the two action triggers that createForeignKeyActionTriggers + * would have created. + */ + trigtup = heap_getnext(scan, ForwardScanDirection); + Assert(trigtup != NULL); + trigform = (Form_pg_trigger) GETSTRUCT(trigtup); + deleteDependencyRecordsForClass(TriggerRelationId, trigform->oid, + ConstraintRelationId, + DEPENDENCY_INTERNAL); + CatalogTupleDelete(trigrel, &trigtup->t_self); + + trigtup = heap_getnext(scan, ForwardScanDirection); + Assert(trigtup != NULL); + trigform = (Form_pg_trigger) GETSTRUCT(trigtup); + deleteDependencyRecordsForClass(TriggerRelationId, trigform->oid, + ConstraintRelationId, + DEPENDENCY_INTERNAL); + CatalogTupleDelete(trigrel, &trigtup->t_self); + + trigtup = heap_getnext(scan, ForwardScanDirection); + Assert(trigtup == NULL); + heap_endscan(scan); + heap_close(trigrel, AccessShareLock); + ConstraintSetParentConstraint(fk->conoid, constrForm->oid); CommandCounterIncrement(); attach_it = true; @@ -714,7 +755,8 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, fkconstraint->initdeferred = constrForm->condeferred; createForeignKeyTriggers(partRel, constrForm->confrelid, fkconstraint, - constrOid, constrForm->conindid, false); + constrOid, constrForm->conindid, false, + true); if (cloned) { diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d2781cbf19..80b46d3139 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7623,7 +7623,7 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * though the constraints also exist below. */ createForeignKeyTriggers(rel, RelationGetRelid(pkrel), fkconstraint, - constrOid, indexOid, !recursing); + constrOid, indexOid, !recursing, true); /* * Tell Phase 3 to check that the constraint is satisfied by existing @@ -8728,7 +8728,8 @@ createForeignKeyCheckTriggers(Oid myRelOid, Oid refRelOid, */ void createForeignKeyTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint, - Oid constraintOid, Oid indexOid, bool create_action) + Oid constraintOid, Oid indexOid, bool create_action, + bool create_check) { /* * For the referenced side, create action triggers, if requested. (If the @@ -8744,7 +8745,7 @@ createForeignKeyTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint, * For the referencing side, create the check triggers. We only need * these on the partitions. */ - if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE && create_check) createForeignKeyCheckTriggers(RelationGetRelid(rel), refRelOid, fkconstraint, constraintOid, indexOid); @@ -14749,19 +14750,52 @@ ATExecDetachPartition(Relation rel, RangeVar *name) } heap_close(classRel, RowExclusiveLock); - /* Detach foreign keys */ + /* Detach any foreign keys that are inherited */ fks = copyObject(RelationGetFKeyList(partRel)); foreach(cell, fks) { ForeignKeyCacheInfo *fk = lfirst(cell); HeapTuple contup; + Form_pg_constraint conform; + Constraint *fkconstraint; contup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid)); if (!contup) elog(ERROR, "cache lookup failed for constraint %u", fk->conoid); + conform = (Form_pg_constraint) GETSTRUCT(contup); + /* consider only the inherited foreign keys */ + if (conform->contype != CONSTRAINT_FOREIGN || + !OidIsValid(conform->conparentid)) + { + ReleaseSysCache(contup); + continue; + } + + /* unset conparentid and adjust conislocal, coninhcount, etc. */ ConstraintSetParentConstraint(fk->conoid, InvalidOid); + /* + * We'll need to make the action triggers on primary key relation that + * point to this relation as the FK relation. We need to do this, + * because no PK-side triggers exist for an inherited FK constraint. + * Now that we've detached it from the parent, we'd our own. + */ + fkconstraint = makeNode(Constraint); + fkconstraint->conname = pstrdup(NameStr(conform->conname)); + fkconstraint->fk_upd_action = conform->confupdtype; + fkconstraint->fk_del_action = conform->confdeltype; + fkconstraint->deferrable = conform->condeferrable; + fkconstraint->initdeferred = conform->condeferred; + + /* + * As we already got the check triggers, no need to recreate them, + * so pass false for create_check. + */ + createForeignKeyTriggers(partRel, conform->confrelid, fkconstraint, + fk->conoid, conform->conindid, + true, false); + ReleaseSysCache(contup); } list_free_deep(fks); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index ec3bb90b01..6bebc68425 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -78,7 +78,8 @@ extern void check_of_type(HeapTuple typetuple); extern void createForeignKeyTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint, Oid constraintOid, - Oid indexOid, bool create_action); + Oid indexOid, bool create_action, + bool create_check); extern void register_on_commit_action(Oid relid, OnCommitAction action); extern void remove_on_commit_action(Oid relid); -- 2.11.0
From 3c2ec8fb45df5b807cb00fa9d2c742451372bda2 Mon Sep 17 00:00:00 2001 From: amit <amitlangot...@gmail.com> Date: Wed, 9 Jan 2019 14:01:47 +0900 Subject: [PATCH v2 2/2] Do not track foreign key inheritance by dependencies Inheritance information maintained in pg_constraint is enough to prevent a child constraint to be dropped and for them to be dropped when the parent constraint is dropped. So, do not create dependencies between the parent foreign key constraint and its children. Also, fix ATAddForeignKeyConstraint to set inheritance information correctly. --- src/backend/catalog/pg_constraint.c | 33 ++++++++---------------- src/backend/commands/indexcmds.c | 18 +++++++++++++ src/backend/commands/tablecmds.c | 50 ++++++++++++++++++++++++++----------- 3 files changed, 65 insertions(+), 36 deletions(-) diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 3cac851447..3571416c3d 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -479,8 +479,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, Constraint *fkconstraint; bool attach_it; Oid constrOid; - ObjectAddress parentAddr, - childAddr; int nelem; ListCell *cell; int i; @@ -501,8 +499,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, continue; } - ObjectAddressSet(parentAddr, ConstraintRelationId, parentConstrOid); - datum = fastgetattr(tuple, Anum_pg_constraint_conkey, tupdesc, &isnull); if (isnull) @@ -743,9 +739,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel, 1, false, true); subclone = lappend_oid(subclone, constrOid); - ObjectAddressSet(childAddr, ConstraintRelationId, constrOid); - recordDependencyOn(&childAddr, &parentAddr, DEPENDENCY_INTERNAL_AUTO); - fkconstraint = makeNode(Constraint); /* for now this is all we need */ fkconstraint->conname = pstrdup(NameStr(constrForm->conname)); @@ -1197,8 +1190,8 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId, * ConstraintSetParentConstraint * Set a partition's constraint as child of its parent table's * - * This updates the constraint's pg_constraint row to show it as inherited, and - * add a dependency to the parent so that it cannot be removed on its own. + * This updates the constraint's pg_constraint row to change its + * inheritance properties. */ void ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId) @@ -1207,8 +1200,6 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId) Form_pg_constraint constrForm; HeapTuple tuple, newtup; - ObjectAddress depender; - ObjectAddress referenced; constrRel = heap_open(ConstraintRelationId, RowExclusiveLock); tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(childConstrId)); @@ -1220,25 +1211,23 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId) { constrForm->conislocal = false; constrForm->coninhcount++; + /* + * An inherited foreign key constraint can never have more than one + * parent, because inheriting foreign keys is only allowed for + * partitioning where multiple inheritance is disallowed. + */ + Assert(constrForm->coninhcount == 1); constrForm->conparentid = parentConstrId; CatalogTupleUpdate(constrRel, &tuple->t_self, newtup); - - ObjectAddressSet(referenced, ConstraintRelationId, parentConstrId); - ObjectAddressSet(depender, ConstraintRelationId, childConstrId); - - recordDependencyOn(&depender, &referenced, DEPENDENCY_INTERNAL_AUTO); } else { constrForm->coninhcount--; - if (constrForm->coninhcount <= 0) - constrForm->conislocal = true; + /* See the above comment. */ + Assert(constrForm->coninhcount == 0); + constrForm->conislocal = true; constrForm->conparentid = InvalidOid; - - deleteDependencyRecordsForClass(ConstraintRelationId, childConstrId, - ConstraintRelationId, - DEPENDENCY_INTERNAL_AUTO); CatalogTupleUpdate(constrRel, &tuple->t_self, newtup); } diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 1959e8a82e..91e0b968ac 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -973,9 +973,27 @@ DefineIndex(Oid relationId, /* Attach index to parent and we're done. */ IndexSetParentIndex(cldidx, indexRelationId); if (createdConstraintId != InvalidOid) + { + ObjectAddress depender; + ObjectAddress referenced; + ConstraintSetParentConstraint(cldConstrOid, createdConstraintId); + /* + * Need to set an explicit dependency in this + * case unlike other types of constraints where + * the child constraint gets dropped due to + * inheritance recursion. + */ + ObjectAddressSet(referenced, ConstraintRelationId, + createdConstraintId); + ObjectAddressSet(depender, ConstraintRelationId, + cldConstrOid); + recordDependencyOn(&depender, &referenced, + DEPENDENCY_INTERNAL_AUTO); + } + if (!cldidx->rd_index->indisvalid) invalidate_parent = true; diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 80b46d3139..96aef0699c 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7248,6 +7248,9 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, bool old_check_ok; ObjectAddress address; ListCell *old_pfeqop_item = list_head(fkconstraint->old_conpfeqop); + bool conislocal = true; + int coninhcount = 0; + bool connoinherit = true; /* * Grab ShareRowExclusiveLock on the pk table, so that someone doesn't @@ -7586,6 +7589,26 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, /* * Record the FK constraint in pg_constraint. */ + + /* + * Set the inheritance related properties correctly if it's being + * recursively added for a partition. + */ + if (OidIsValid(parentConstr)) + { + /* Foreign keys are inherited only for partitioning. */ + Assert(rel->rd_rel->relispartition); + conislocal = false; + /* Partitions can have only one parent. */ + coninhcount = 1; + /* Make sure that the constraint can be further inherited. */ + connoinherit = false; + } + + /* If partitioned table, constraint must be marked as inheritable. */ + if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + connoinherit = false; + constrOid = CreateConstraintEntry(fkconstraint->conname, RelationGetNamespace(rel), CONSTRAINT_FOREIGN, @@ -7611,9 +7634,9 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, NULL, /* no exclusion constraint */ NULL, /* no check constraint */ NULL, - true, /* islocal */ - 0, /* inhcount */ - true, /* isnoinherit */ + conislocal, + coninhcount, + connoinherit, false); /* is_internal */ ObjectAddressSet(address, ConstraintRelationId, constrOid); @@ -7662,20 +7685,15 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, Oid partitionId = partdesc->oids[i]; Relation partition = heap_open(partitionId, lockmode); AlteredTableInfo *childtab; - ObjectAddress childAddr; CheckTableNotInUse(partition, "ALTER TABLE"); /* Find or create work queue entry for this table */ childtab = ATGetQueueEntry(wqueue, partition); - childAddr = - ATAddForeignKeyConstraint(wqueue, childtab, partition, - fkconstraint, constrOid, - recurse, true, lockmode); - - /* Record this constraint as dependent on the parent one */ - recordDependencyOn(&childAddr, &address, DEPENDENCY_INTERNAL_AUTO); + ATAddForeignKeyConstraint(wqueue, childtab, partition, + fkconstraint, constrOid, + recurse, true, lockmode); heap_close(partition, NoLock); } @@ -8925,9 +8943,13 @@ ATExecDropConstraint(Relation rel, const char *constrName, con = (Form_pg_constraint) GETSTRUCT(copy_tuple); - /* Right now only CHECK constraints can be inherited */ - if (con->contype != CONSTRAINT_CHECK) - elog(ERROR, "inherited constraint is not a CHECK constraint"); + /* + * Right now only CHECK constraints and foreign key constraints can + * be inherited. + */ + if (con->contype != CONSTRAINT_CHECK && + con->contype != CONSTRAINT_FOREIGN) + elog(ERROR, "inherited constraint is not a CHECK constraint or a foreign key constraint"); if (con->coninhcount <= 0) /* shouldn't happen */ elog(ERROR, "relation %u has non-inherited constraint \"%s\"", -- 2.11.0