Re: problems with foreign keys on partitioned tables
On 2019/01/25 2:18, Alvaro Herrera wrote: > On 2019-Jan-24, Amit Langote wrote: > >> A few hunks of the originally proposed patch are attached here as 0001, >> especially the part which fixes ATAddForeignKeyConstraint to pass the >> correct value of connoninherit to CreateConstraintEntry (which should be >> false for partitioned tables). With that change, many tests start failing >> because of the above bug. That patch also adds a test case like the one >> above, but it fails along with others due to the bug. Patch 0002 is the >> aforementioned simpler fix to make the errors (existing and the newly >> added) go away. > > Cool, thanks. I made a bunch of fixes -- I added an elog(ERROR) check > to ensure a constraint whose parent is being set does not already have a > parent; that seemed in line with the new asserts that check the > coninhcount. I also moved those asserts, changing the spirit of what > they checked. Also: I wasn't sure about stopping recursion for legacy > inheritance in ATExecDropConstraint() for non-check constraints, so I > changed that to occur in partitioned only. Also, stylistic fixes. Thanks for the fixes and committing. > I was mildly surprised to realize that the my_fkey constraint on > fk_part_1_1 is gone after dropping fkey on its parent, since it was > declared locally when that table was created. However, it makes perfect > sense in retrospect, since we made it dependent on its parent. I'm not > terribly happy about this, but I don't quite see a way to make it better > that doesn't require much more code than is warranted. Fwiw, CHECK constraints behave that way too. OTOH, detaching a partition preserves all the constraints, even the ones that were never locally defined on the partition. >> These patches apply unchanged to the PG 11 branch. > > Yeah, only if you tried to compile, it would have complained about > table_close() ;-) Oops, sorry. I was really in a hurry that day as the dinnertime had passed. Thanks, Amit
Re: problems with foreign keys on partitioned tables
On 2019-Jan-24, Amit Langote wrote: > A few hunks of the originally proposed patch are attached here as 0001, > especially the part which fixes ATAddForeignKeyConstraint to pass the > correct value of connoninherit to CreateConstraintEntry (which should be > false for partitioned tables). With that change, many tests start failing > because of the above bug. That patch also adds a test case like the one > above, but it fails along with others due to the bug. Patch 0002 is the > aforementioned simpler fix to make the errors (existing and the newly > added) go away. Cool, thanks. I made a bunch of fixes -- I added an elog(ERROR) check to ensure a constraint whose parent is being set does not already have a parent; that seemed in line with the new asserts that check the coninhcount. I also moved those asserts, changing the spirit of what they checked. Also: I wasn't sure about stopping recursion for legacy inheritance in ATExecDropConstraint() for non-check constraints, so I changed that to occur in partitioned only. Also, stylistic fixes. I was mildly surprised to realize that the my_fkey constraint on fk_part_1_1 is gone after dropping fkey on its parent, since it was declared locally when that table was created. However, it makes perfect sense in retrospect, since we made it dependent on its parent. I'm not terribly happy about this, but I don't quite see a way to make it better that doesn't require much more code than is warranted. > These patches apply unchanged to the PG 11 branch. Yeah, only if you tried to compile, it would have complained about table_close() ;-) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: problems with foreign keys on partitioned tables
On 2019-Jan-25, Amit Langote wrote: > On Fri, Jan 25, 2019 at 12:30 AM Amit Langote wrote: > > Doesn't the following performDeletion() at the start of > > ATExecDropConstraint(), through findDependentObject()'s own recursion, > > take care of deleting *all* constraints, including those of? > > Meant to say: "...including those of the grandchildren?" > > > /* > > * Perform the actual constraint deletion > > */ > > conobj.classId = ConstraintRelationId; > > conobj.objectId = con->oid; > > conobj.objectSubId = 0; > > > > performDeletion(, behavior, 0); Of course it does when the dependencies are set up -- but in the approach we just gave up on, those dependencies would not exist. Anyway, my motivation was that performMultipleDeletions has the advantage that it collects all objects to be dropped before deleting anyway, and so the error that a constraint was dropped in a previous recursion step would not occur. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: problems with foreign keys on partitioned tables
On Fri, Jan 25, 2019 at 12:30 AM Amit Langote wrote: > > On Fri, Jan 25, 2019 at 12:08 AM Alvaro Herrera > wrote: > > On 2019-Jan-24, Amit Langote wrote: > > > > > Thinking more on this, my proposal to rip dependencies between parent and > > > child constraints altogether to resolve the bug I initially reported is > > > starting to sound a bit overambitious especially considering that we'd > > > need to back-patch it (the patch didn't even consider index constraints > > > properly, creating a divergence between the behaviors of inherited foreign > > > key constraints and inherited index constraints). We can pursue it if > > > only to avoid bloating the catalog for what can be achieved with little > > > bit of additional code in tablecmds.c, but maybe we should refrain from > > > doing it in reaction to this particular bug. > > > > While studying your fix it occurred to me that perhaps we could change > > things so that we first collect a list of objects to drop, and only when > > we're done recursing perform the deletion, as per the attached patch. > > However, this fails for the test case in your 0001 patch (but not the > > one you show in your email body), because you added a stealthy extra > > ingredient to it: the constraint in the grandchild has a different name, > > so when ATExecDropConstraint() tries to search for it by name, it's just > > not there, not because it was dropped but because it has never existed > > in the first place. > > Doesn't the following performDeletion() at the start of > ATExecDropConstraint(), through findDependentObject()'s own recursion, > take care of deleting *all* constraints, including those of? Meant to say: "...including those of the grandchildren?" > /* > * Perform the actual constraint deletion > */ > conobj.classId = ConstraintRelationId; > conobj.objectId = con->oid; > conobj.objectSubId = 0; > > performDeletion(, behavior, 0); Thanks, Amit
Re: problems with foreign keys on partitioned tables
On Fri, Jan 25, 2019 at 12:08 AM Alvaro Herrera wrote: > On 2019-Jan-24, Amit Langote wrote: > > > Thinking more on this, my proposal to rip dependencies between parent and > > child constraints altogether to resolve the bug I initially reported is > > starting to sound a bit overambitious especially considering that we'd > > need to back-patch it (the patch didn't even consider index constraints > > properly, creating a divergence between the behaviors of inherited foreign > > key constraints and inherited index constraints). We can pursue it if > > only to avoid bloating the catalog for what can be achieved with little > > bit of additional code in tablecmds.c, but maybe we should refrain from > > doing it in reaction to this particular bug. > > While studying your fix it occurred to me that perhaps we could change > things so that we first collect a list of objects to drop, and only when > we're done recursing perform the deletion, as per the attached patch. > However, this fails for the test case in your 0001 patch (but not the > one you show in your email body), because you added a stealthy extra > ingredient to it: the constraint in the grandchild has a different name, > so when ATExecDropConstraint() tries to search for it by name, it's just > not there, not because it was dropped but because it has never existed > in the first place. Doesn't the following performDeletion() at the start of ATExecDropConstraint(), through findDependentObject()'s own recursion, take care of deleting *all* constraints, including those of? /* * Perform the actual constraint deletion */ conobj.classId = ConstraintRelationId; conobj.objectId = con->oid; conobj.objectSubId = 0; performDeletion(, behavior, 0); > Unless I misunderstand, this means that your plan to remove those > catalog tuples won't work at all, because there is no way to find those > constraints other than via pg_depend if they have different names. Yeah, that's right. Actually, I gave up on developing the patch further based on that approach (no-dependencies approach) when I edited the test to give the grandchild constraint its own name. > I'm leaning towards the idea that your patch is the definitive fix and > not just a backpatchable band-aid. Yeah, I think so too. Thanks, Amit
Re: problems with foreign keys on partitioned tables
Hello
On 2019-Jan-24, Amit Langote wrote:
> Thinking more on this, my proposal to rip dependencies between parent and
> child constraints altogether to resolve the bug I initially reported is
> starting to sound a bit overambitious especially considering that we'd
> need to back-patch it (the patch didn't even consider index constraints
> properly, creating a divergence between the behaviors of inherited foreign
> key constraints and inherited index constraints). We can pursue it if
> only to avoid bloating the catalog for what can be achieved with little
> bit of additional code in tablecmds.c, but maybe we should refrain from
> doing it in reaction to this particular bug.
While studying your fix it occurred to me that perhaps we could change
things so that we first collect a list of objects to drop, and only when
we're done recursing perform the deletion, as per the attached patch.
However, this fails for the test case in your 0001 patch (but not the
one you show in your email body), because you added a stealthy extra
ingredient to it: the constraint in the grandchild has a different name,
so when ATExecDropConstraint() tries to search for it by name, it's just
not there, not because it was dropped but because it has never existed
in the first place.
Unless I misunderstand, this means that your plan to remove those
catalog tuples won't work at all, because there is no way to find those
constraints other than via pg_depend if they have different names.
I'm leaning towards the idea that your patch is the definitive fix and
not just a backpatchable band-aid.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 747acbd2b9..29860acaa2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -417,7 +417,7 @@ static void CloneFkReferencing(Relation pg_constraint, Relation parentRel,
static void ATExecDropConstraint(Relation rel, const char *constrName,
DropBehavior behavior,
bool recurse, bool recursing,
- bool missing_ok, LOCKMODE lockmode);
+ bool missing_ok, LOCKMODE lockmode, ObjectAddresses *objsToDelete);
static void ATPrepAlterColumnType(List **wqueue,
AlteredTableInfo *tab, Relation rel,
bool recurse, bool recursing,
@@ -4160,12 +4160,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
case AT_DropConstraint: /* DROP CONSTRAINT */
ATExecDropConstraint(rel, cmd->name, cmd->behavior,
false, false,
- cmd->missing_ok, lockmode);
+ cmd->missing_ok, lockmode, NULL);
break;
case AT_DropConstraintRecurse: /* DROP CONSTRAINT with recursion */
ATExecDropConstraint(rel, cmd->name, cmd->behavior,
true, false,
- cmd->missing_ok, lockmode);
+ cmd->missing_ok, lockmode, NULL);
break;
case AT_AlterColumnType: /* ALTER COLUMN TYPE */
address = ATExecAlterColumnType(tab, rel, cmd, lockmode);
@@ -9219,7 +9219,8 @@ static void
ATExecDropConstraint(Relation rel, const char *constrName,
DropBehavior behavior,
bool recurse, bool recursing,
- bool missing_ok, LOCKMODE lockmode)
+ bool missing_ok, LOCKMODE lockmode,
+ ObjectAddresses *objsToDelete)
{
List *children;
ListCell *child;
@@ -9237,6 +9238,12 @@ ATExecDropConstraint(Relation rel, const char *constrName,
conrel = heap_open(ConstraintRelationId, RowExclusiveLock);
+ if (!recursing)
+ {
+ Assert(objsToDelete == NULL);
+ objsToDelete = new_object_addresses();
+ }
+
/*
* Find and drop the target constraint
*/
@@ -9290,13 +9297,13 @@ ATExecDropConstraint(Relation rel, const char *constrName,
}
/*
- * Perform the actual constraint deletion
+ * Register the constraint for deletion
*/
conobj.classId = ConstraintRelationId;
conobj.objectId = HeapTupleGetOid(tuple);
conobj.objectSubId = 0;
- performDeletion(, behavior, 0);
+ add_exact_object_address(, objsToDelete);
found = true;
}
@@ -9402,7 +9409,7 @@ ATExecDropConstraint(Relation rel, const char *constrName,
/* Time to delete this child constraint, too */
ATExecDropConstraint(childrel, constrName, behavior,
true, true,
- false, lockmode);
+ false, lockmode, objsToDelete);
}
else
{
@@ -9435,6 +9442,9 @@ ATExecDropConstraint(Relation rel, const char *constrName,
heap_close(childrel, NoLock);
}
+ if (!recursing)
+ performMultipleDeletions(objsToDelete, behavior, 0);
+
heap_close(conrel, RowExclusiveLock);
}
Re: problems with foreign keys on partitioned tables
Hi,
On 2019/01/24 6:13, Alvaro Herrera wrote:
> On 2019-Jan-22, Amit Langote wrote:
>> Done. See the attached patches for HEAD and PG 11.
>
> I'm not quite sure I understand why the one in DefineIndex needs the
> deps but nothing else does. I fear that you added that one just to
> appease the existing test that breaks otherwise, and I worry that with
> that addition we're papering over some other, more fundamental bug.
Thinking more on this, my proposal to rip dependencies between parent and
child constraints altogether to resolve the bug I initially reported is
starting to sound a bit overambitious especially considering that we'd
need to back-patch it (the patch didn't even consider index constraints
properly, creating a divergence between the behaviors of inherited foreign
key constraints and inherited index constraints). We can pursue it if
only to avoid bloating the catalog for what can be achieved with little
bit of additional code in tablecmds.c, but maybe we should refrain from
doing it in reaction to this particular bug.
I've updated the patch that implements a much simpler fix for this
particular bug. Just to reiterate, the following illustrates the bug:
create table pk (a int primary key);
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;
alter table p1 drop constraint p_a_fkey;
ERROR: constraint "p_a_fkey" of relation "p11" does not exist
The error occurs because ATExecDropConstraint when recursively called on
p11 cannot find the constraint as the dependency mechanism already dropped
it. The new fix is to return from ATExecDropConstraint without recursing
if the constraint being dropped is index or foreign constraint.
A few hunks of the originally proposed patch are attached here as 0001,
especially the part which fixes ATAddForeignKeyConstraint to pass the
correct value of connoninherit to CreateConstraintEntry (which should be
false for partitioned tables). With that change, many tests start failing
because of the above bug. That patch also adds a test case like the one
above, but it fails along with others due to the bug. Patch 0002 is the
aforementioned simpler fix to make the errors (existing and the newly
added) go away.
These patches apply unchanged to the PG 11 branch.
Thanks,
Amit
From 8eb5ce74e8656b517ad5a4e960b70de0ff3bedd5 Mon Sep 17 00:00:00 2001
From: amit
Date: Thu, 24 Jan 2019 21:29:17 +0900
Subject: [PATCH 1/2] Fix ATAddForeignKeyConstraint to use correct value of
connoinherit
While at it, add some Asserts to ConstraintSetParentConstraint to
assert the correct value of coninhcount.
Many tests fail, but the next patch should take care of them.
---
src/backend/catalog/pg_constraint.c | 11 +--
src/backend/commands/tablecmds.c | 5 -
src/test/regress/expected/foreign_key.out | 18 --
src/test/regress/sql/foreign_key.sql | 15 ++-
4 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index 446b54b9ff..d2b8489b6c 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -785,6 +785,12 @@ 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, >t_self, newtup);
@@ -797,8 +803,9 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid
parentConstrId)
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,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 738c178107..fea4d99735 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7251,6 +7251,9 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo
*tab, Relation rel,
boolold_check_ok;
ObjectAddress address;
ListCell *old_pfeqop_item = list_head(fkconstraint->old_conpfeqop);
+ /* Partitioned table's foreign
Re: problems with foreign keys on partitioned tables
On 2019-Jan-22, Amit Langote wrote: > On 2019/01/22 8:30, Alvaro Herrera wrote: > > Hi Amit, > > > > Will you please rebase 0002? Please add your proposed tests cases to > > it, too. > > Done. See the attached patches for HEAD and PG 11. I'm not quite sure I understand why the one in DefineIndex needs the deps but nothing else does. I fear that you added that one just to appease the existing test that breaks otherwise, and I worry that with that addition we're papering over some other, more fundamental bug. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: problems with foreign keys on partitioned tables
On 2019/01/22 8:30, Alvaro Herrera wrote:
> Hi Amit,
>
> Will you please rebase 0002? Please add your proposed tests cases to
> it, too.
Done. See the attached patches for HEAD and PG 11.
Thanks,
Amit
From 432c4551990d0da1c77b6b9523296b0a2a0a5119 Mon Sep 17 00:00:00 2001
From: amit
Date: Tue, 22 Jan 2019 13:20:54 +0900
Subject: [PATCH] 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 connoniherit on the parent
constraint correctly.
---
src/backend/catalog/pg_constraint.c | 27 +--
src/backend/commands/indexcmds.c | 22 --
src/backend/commands/tablecmds.c | 24
src/test/regress/expected/foreign_key.out | 17 +++--
src/test/regress/sql/foreign_key.sql | 14 +-
5 files changed, 73 insertions(+), 31 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index 446b54b9ff..855d57c65a 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -762,8 +762,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)
@@ -772,8 +772,6 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid
parentConstrId)
Form_pg_constraint constrForm;
HeapTuple tuple,
newtup;
- ObjectAddress depender;
- ObjectAddress referenced;
constrRel = table_open(ConstraintRelationId, RowExclusiveLock);
tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(childConstrId));
@@ -785,25 +783,26 @@ 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, >t_self, newtup);
-
- ObjectAddressSet(referenced, ConstraintRelationId,
parentConstrId);
- ObjectAddressSet(depender, ConstraintRelationId, childConstrId);
-
- recordDependencyOn(, ,
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, >t_self, newtup);
}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3edc94308e..6c8aa5d149 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -972,8 +972,26 @@ DefineIndex(Oid relationId,
/* Attach index to parent and
we're done. */
IndexSetParentIndex(cldidx,
indexRelationId);
if (createdConstraintId !=
InvalidOid)
-
ConstraintSetParentConstraint(cldConstrOid,
-
createdConstraintId);
+ {
+ ObjectAddress depender;
+ ObjectAddress
referenced;
+
+
Re: problems with foreign keys on partitioned tables
Hi Amit, Will you please rebase 0002? Please add your proposed tests cases to it, too. Thanks, -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: problems with foreign keys on partitioned tables
Pushed now, thanks. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: problems with foreign keys on partitioned tables
On Sat, Jan 19, 2019 at 7:16 AM Alvaro Herrera wrote: > Thanks, this is better. There were a few other things I didn't like, so > I updated it. Mostly, two things: > > 1. I didn't like a seqscan on pg_trigger, so I turned that into an > indexed scan on the constraint OID, and then the other two conditions > are checked in the returned tuples. Also, what's the point on > duplicating code and checking how many you deleted? Just delete them > all. Yeah, I didn't quite like what that code looked like, but it didn't occur to me that there's an index on tgconstraint. It looks much better now. > 2. I didn't like the ABI break, and it wasn't necessary: you can just > call createForeignKeyActionTriggers directly. That's much simpler. OK. > I also added tests. While running them, I noticed that my previous > commit was broken in terms of relcache invalidation. I don't really > know if this is a new problem with that commit, or an existing one. The > fix is 0001. Looks good. Thanks, Amit
Re: problems with foreign keys on partitioned tables
On 2019-Jan-18, Amit Langote wrote:
> OK, I agree. I have updated the patch to make things work that way.
Thanks, this is better. There were a few other things I didn't like, so
I updated it. Mostly, two things:
1. I didn't like a seqscan on pg_trigger, so I turned that into an
indexed scan on the constraint OID, and then the other two conditions
are checked in the returned tuples. Also, what's the point on
duplicating code and checking how many you deleted? Just delete them
all.
2. I didn't like the ABI break, and it wasn't necessary: you can just
call createForeignKeyActionTriggers directly. That's much simpler.
I also added tests. While running them, I noticed that my previous
commit was broken in terms of relcache invalidation. I don't really
know if this is a new problem with that commit, or an existing one. The
fix is 0001.
Haven't got around to your 0002 yet.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 2047f054facf3ffaff31e36591279b9bd76ca53c Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Fri, 18 Jan 2019 19:06:40 -0300
Subject: [PATCH 1/2] Add missing relcache reset
Adding a foreign key to a partitioned table requires applying a relcache
reset to each partition, so that the newly added FK is correctly
recorded in its entry on next rebuild.
With the previous coding of ATAddForeignKeyConstraint, this may not have
been necessary, but with the one after 0325d7a5957b, it is.
Noted while testing another bugfix.
---
src/backend/commands/tablecmds.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1a1ac698e5..5a27f64aa7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7700,6 +7700,12 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
childtab->constraints = lappend(childtab->constraints, newcon);
+ /*
+ * Also invalidate the partition's relcache entry, so that its
+ * FK list gets updated on next rebuild.
+ */
+ CacheInvalidateRelcache(partition);
+
heap_close(partition, lockmode);
}
}
--
2.11.0
>From 42b96db12aafea22285893e1d96c63a7f684abfd Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Fri, 18 Jan 2019 19:10:29 -0300
Subject: [PATCH 2/2] Fix action triggers for FK constraints on detached
partitions
---
src/backend/commands/tablecmds.c | 72 +++-
src/test/regress/sql/foreign_key.sql | 20 +-
2 files changed, 89 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5a27f64aa7..02a55ed3d6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7858,6 +7858,10 @@ CloneFkReferencing(Relation pg_constraint, Relation parentRel,
ForeignKeyCacheInfo *fk = lfirst_node(ForeignKeyCacheInfo, cell);
Form_pg_constraint partConstr;
HeapTuple partcontup;
+ Relation trigrel;
+ HeapTuple trigtup;
+ SysScanDesc scan;
+ ScanKeyData key;
attach_it = true;
@@ -7909,7 +7913,39 @@ CloneFkReferencing(Relation pg_constraint, Relation parentRel,
ReleaseSysCache(partcontup);
- /* looks good! Attach this constraint */
+ /*
+ * Looks good! Attach this constraint. Note that the action
+ * triggers are no longer needed, so remove them. We identify
+ * them because they have our constraint OID, as well as being
+ * on the referenced rel.
+ */
+ trigrel = heap_open(TriggerRelationId, RowExclusiveLock);
+ ScanKeyInit(,
+ Anum_pg_trigger_tgconstraint,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(fk->conoid));
+
+ scan = systable_beginscan(trigrel, TriggerConstraintIndexId, true,
+ NULL, 1, );
+ while ((trigtup = systable_getnext(scan)) != NULL)
+ {
+Form_pg_trigger trgform = (Form_pg_trigger) GETSTRUCT(trigtup);
+
+if (trgform->tgconstrrelid != fk->conrelid)
+ continue;
+if (trgform->tgrelid != fk->confrelid)
+ continue;
+
+deleteDependencyRecordsForClass(TriggerRelationId,
+trgform->oid,
+ConstraintRelationId,
+DEPENDENCY_INTERNAL);
+CatalogTupleDelete(trigrel, >t_self);
+ }
+
+ systable_endscan(scan);
+ heap_close(trigrel, RowExclusiveLock);
+
ConstraintSetParentConstraint(fk->conoid, parentConstrOid);
CommandCounterIncrement();
attach_it = true;
@@ -15080,19 +15116,51 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
}
heap_close(classRel, RowExclusiveLock);
- /* Detach foreign keys */
+ /*
+ * Detach any foreign keys that are inherited. This includes creating
+ * additional action triggers.
+ */
fks = copyObject(RelationGetFKeyList(partRel));
foreach(cell, fks)
{
ForeignKeyCacheInfo *fk = lfirst(cell);
HeapTuple contup;
+ Form_pg_constraint conform;
+ Constraint *fkconstraint;
Re: problems with foreign keys on partitioned tables
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│
Re: problems with foreign keys on partitioned tables
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? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: problems with foreign keys on partitioned tables
Hi Amit On 2019-Jan-09, Amit Langote wrote: > I noticed a couple of problems with foreign keys on partitioned tables. Ouch, thanks for reporting. I think 0001 needs a bit of a tweak in pg11 to avoid an ABI break -- I intend to study this one and try to push early next week. I'm going to see about pushing 0002 shortly, adding some of your tests. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
