Hi,
I noticed a couple of problems with foreign keys on partitioned tables.
1. Foreign keys of partitions stop working correctly after being detached
from the parent table
create table pk (a int primary key);
create table p (a int) partition by list (a);
create table p1 partition of p for values in (1) partition by list (a);
create table p11 partition of p1 for values in (1);
alter table p add foreign key (a) references pk (a);
-- these things work correctly
insert into p values (1);
ERROR: insert or update on table "p11" violates foreign key constraint
"p_a_fkey"
DETAIL: Key (a)=(1) is not present in table "pk".
insert into pk values (1);
insert into p values (1);
delete from pk where a = 1;
ERROR: update or delete on table "pk" violates foreign key constraint
"p_a_fkey" on table "p"
DETAIL: Key (a)=(1) is still referenced from table "p".
-- detach p1, which preserves the foreign key key
alter table p detach partition p1;
create table p12 partition of p1 for values in (2);
-- this part of the foreign key on p1 still works
insert into p1 values (2);
ERROR: insert or update on table "p12" violates foreign key constraint
"p_a_fkey"
DETAIL: Key (a)=(2) is not present in table "pk".
-- but this seems wrong
delete from pk where a = 1;
DELETE 1
-- because
select * from p1;
a
───
1
(1 row)
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. 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.
2. Foreign keys of a partition cannot be dropped in some cases after
detaching it from the parent.
create table p (a int references pk) partition by list (a);
create table p1 partition of p for values in (1) partition by list (a);
create table p11 partition of p1 for values in (1);
alter table p detach partition p1;
-- p1's foreign key is no longer inherited, so should be able to drop it
alter table p1 drop constraint p_a_fkey ;
ERROR: constraint "p_a_fkey" of relation "p11" does not exist
This happens because by the time ATExecDropConstraint tries to recursively
drop the p11's inherited foreign key constraint (which is what normally
happens for inherited constraints), the latter has already been dropped by
dependency management. I think the foreign key inheritance related code
doesn't need to add dependencies for something that inheritance recursion
can take of and I can't think of any other reason to have such
dependencies around. I thought maybe they're needed for pg_dump to work
correctly, but apparently not so.
Interestingly, the above problem doesn't occur if the constraint is added
to partitions by inheritance recursion.
create table p (a int) partition by list (a);
create table p1 partition of p for values in (1) partition by list (a);
create table p11 partition of p1 for values in (1);
alter table p add foreign key (a) references pk (a);
alter table p detach partition p1;
alter table p1 drop constraint p_a_fkey ;
ALTER TABLE
Looking into it, that happens to work *accidentally*.
ATExecDropInherit() doesn't try to recurse, which prevents the error in
this case, because it finds that the constraint on p1 is marked NO INHERIT
(non-inheritable), which is incorrect. The value of p1's constraint's
connoinherit (in fact, other inheritance related properties too) is
incorrect, because ATAddForeignKeyConstraint doesn't bother to set them
correctly. This is what the inheritance properties of various copies of
'p_a_fkey' looks like in the catalog in this case:
-- case 1: foreign key added to partitions recursively
create table p (a int) partition by list (a);
create table p1 partition of p for values in (1) partition by list (a);
create table p11 partition of p1 for values in (1);
alter table p add foreign key (a) references pk (a);
select conname, conrelid::regclass, conislocal, coninhcount, connoinherit
from pg_constraint where conname like 'p%fkey%';
conname │ conrelid │ conislocal │ coninhcount │ connoinherit
──────────┼──────────┼────────────┼─────────────┼──────────────
p_a_fkey │ p │ t │ 0 │ t
p_a_fkey │ p1 │ t │ 0 │ t
p_a_fkey │ p11 │ t │ 0 │ t
(3 rows)
In this case, after detaching p1 from p, p1's foreign key's coninhcount
turns to -1, which is not good.
alter table p detach partition p1;
select conname, conrelid::regclass, conislocal, coninhcount, connoinherit
from pg_constraint where conname like 'p%fkey%';
conname │ conrelid │ conislocal │ coninhcount │ connoinherit
──────────┼──────────┼────────────┼─────────────┼──────────────
p_a_fkey │ p │ t │ 0 │ t
p_a_fkey │ p11 │ t │ 0 │ t
p_a_fkey │ p1 │ t │ -1 │ t
(3 rows)
-- case 2: foreign keys cloned to partitions after adding partitions
create table p (a int references pk) partition by list (a);
create table p1 partition of p for values in (1) partition by list (a);
create table p11 partition of p1 for values in (1);
select conname, conrelid::regclass, conislocal, coninhcount, connoinherit
from pg_constraint where conname like 'p%fkey%';
conname │ conrelid │ conislocal │ coninhcount │ connoinherit
──────────┼──────────┼────────────┼─────────────┼──────────────
p_a_fkey │ p │ t │ 0 │ t
p_a_fkey │ p1 │ f │ 1 │ f
p_a_fkey │ p11 │ f │ 1 │ f
(3 rows)
Anyway, I propose we fix this by first getting rid of dependencies for
foreign key constraints and instead rely on inheritance recursion for
dropping the inherited constraints. Before we can do that, we'll need to
consistently set the inheritance properties of foreign key constraints
correctly, that is, teach ATAddForeignKeyConstraint what
clone_fk_constraints already does correctly. See the attached patch 0002
for that.
I'm also attaching versions of 0001 and 0002 that can be applied to PG 11.
Thanks,
Amit
From 86e18c2855ee9f8df19dd55dd024938ba2d41f78 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 9 Jan 2019 10:00:11 +0900
Subject: [PATCH v1 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 points to
the detached partition when detaching.
---
src/backend/catalog/pg_constraint.c | 3 +-
src/backend/commands/tablecmds.c | 57 +++++++++++++++++++++++++++++++++++--
src/include/commands/tablecmds.h | 3 +-
3 files changed, 58 insertions(+), 5 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index f4057a9f15..a379bec202 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -720,7 +720,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 04b1098320..ae83deaf51 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);
@@ -14813,13 +14814,63 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
{
ForeignKeyCacheInfo *fk = lfirst(cell);
HeapTuple contup;
+ Form_pg_constraint conform;
+ Constraint *fkconstraint;
+ Relation trigrel;
+ HeapTuple trigtup;
+ HeapScanDesc scan;
+ ScanKeyData key[3];
contup = SearchSysCache1(CONSTROID,
ObjectIdGetDatum(fk->conoid));
if (!contup)
elog(ERROR, "cache lookup failed for constraint %u",
fk->conoid);
+ conform = (Form_pg_constraint) GETSTRUCT(contup);
ConstraintSetParentConstraint(fk->conoid, InvalidOid);
+ /*
+ * We'll need to make the triggers on primary key relation to
point
+ * to this relation as the FK relation. Currently, it points to
+ * the parent from which this relation is being detached.
+ */
+ 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;
+
+ /* First check if such a trigger doesn't already exist. */
+ trigrel = heap_open(TriggerRelationId, AccessShareLock);
+ ScanKeyInit(&key[0],
+ Anum_pg_trigger_tgrelid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(conform->confrelid));
+ ScanKeyInit(&key[1],
+ Anum_pg_trigger_tgconstrrelid,
+ BTEqualStrategyNumber, F_OIDEQ,
+
ObjectIdGetDatum(RelationGetRelid(partRel)));
+ ScanKeyInit(&key[2],
+ Anum_pg_trigger_tgconstraint,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(fk->conoid));
+ scan = heap_beginscan_catalog(trigrel, 3, key);
+ trigtup = heap_getnext(scan, ForwardScanDirection);
+ if (trigtup == NULL)
+ {
+ /*
+ * Nope, we didn't find such a trigger, so create one.
+ *
+ * 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);
+ }
+ heap_endscan(scan);
+ heap_close(trigrel, AccessShareLock);
+
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 669c0621d7970e04663117c43ba877ed53a804df Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 9 Jan 2019 18:52:16 +0900
Subject: [PATCH v1 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 | 57 ++++++++++++++++++++++++++++---------
3 files changed, 72 insertions(+), 36 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index a379bec202..4f0d40473d 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -482,8 +482,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;
@@ -504,8 +502,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)
@@ -708,9 +704,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));
@@ -1162,8 +1155,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)
@@ -1172,8 +1165,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));
@@ -1185,25 +1176,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 965b9f0d23..9e227796bc 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -974,9 +974,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
(!IndexIsValid(cldidx->rd_index))
invalidate_parent =
true;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ae83deaf51..1a5da20bf3 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\"",
@@ -14826,6 +14848,13 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
elog(ERROR, "cache lookup failed for constraint %u",
fk->conoid);
conform = (Form_pg_constraint) GETSTRUCT(contup);
+ /* No need to touch the partition's own foreign keys. */
+ if (conform->conislocal)
+ {
+ ReleaseSysCache(contup);
+ continue;
+ }
+
ConstraintSetParentConstraint(fk->conoid, InvalidOid);
/*
--
2.11.0
From f757e49a93aa5a43b673b973f900bdbf91bef8e4 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 9 Jan 2019 10:00:11 +0900
Subject: [PATCH v1 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 points to
the detached partition when detaching.
---
src/backend/catalog/pg_constraint.c | 3 +-
src/backend/commands/tablecmds.c | 57 +++++++++++++++++++++++++++++++++++--
src/include/commands/tablecmds.h | 3 +-
3 files changed, 58 insertions(+), 5 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index 3c960c9423..c88e923327 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -714,7 +714,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 c75c5808ba..a3eb39ffdd 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);
@@ -14675,13 +14676,63 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
{
ForeignKeyCacheInfo *fk = lfirst(cell);
HeapTuple contup;
+ Form_pg_constraint conform;
+ Constraint *fkconstraint;
+ Relation trigrel;
+ HeapTuple trigtup;
+ HeapScanDesc scan;
+ ScanKeyData key[3];
contup = SearchSysCache1(CONSTROID,
ObjectIdGetDatum(fk->conoid));
if (!contup)
elog(ERROR, "cache lookup failed for constraint %u",
fk->conoid);
+ conform = (Form_pg_constraint) GETSTRUCT(contup);
ConstraintSetParentConstraint(fk->conoid, InvalidOid);
+ /*
+ * We'll need to make the triggers on primary key relation to
point
+ * to this relation as the FK relation. Currently, it points to
+ * the parent from which this relation is being detached.
+ */
+ 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;
+
+ /* First check if such a trigger doesn't already exist. */
+ trigrel = heap_open(TriggerRelationId, AccessShareLock);
+ ScanKeyInit(&key[0],
+ Anum_pg_trigger_tgrelid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(conform->confrelid));
+ ScanKeyInit(&key[1],
+ Anum_pg_trigger_tgconstrrelid,
+ BTEqualStrategyNumber, F_OIDEQ,
+
ObjectIdGetDatum(RelationGetRelid(partRel)));
+ ScanKeyInit(&key[2],
+ Anum_pg_trigger_tgconstraint,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(fk->conoid));
+ scan = heap_beginscan_catalog(trigrel, 3, key);
+ trigtup = heap_getnext(scan, ForwardScanDirection);
+ if (trigtup == NULL)
+ {
+ /*
+ * Nope, we didn't find such a trigger, so create one.
+ *
+ * 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);
+ }
+ heap_endscan(scan);
+ heap_close(trigrel, AccessShareLock);
+
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 92da65c2359db0bb6947a283a7996273d22a014a Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 9 Jan 2019 14:01:47 +0900
Subject: [PATCH v1 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 | 57 ++++++++++++++++++++++++++++---------
3 files changed, 72 insertions(+), 36 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index c88e923327..60e4ff5594 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -478,8 +478,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;
@@ -500,8 +498,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)
@@ -702,9 +698,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));
@@ -1156,8 +1149,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)
@@ -1166,8 +1159,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));
@@ -1179,25 +1170,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 d263903622..d8430f6fe5 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -972,9 +972,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 a3eb39ffdd..8e2fb9b189 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\"",
@@ -14688,6 +14710,13 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
elog(ERROR, "cache lookup failed for constraint %u",
fk->conoid);
conform = (Form_pg_constraint) GETSTRUCT(contup);
+ /* No need to touch the partition's own foreign keys. */
+ if (conform->conislocal)
+ {
+ ReleaseSysCache(contup);
+ continue;
+ }
+
ConstraintSetParentConstraint(fk->conoid, InvalidOid);
/*
--
2.11.0
