Hi,
Thanks for updating the patch. I'll reply to other parts separately.
On 2019/03/19 7:02, Alvaro Herrera wrote:
> A pretty silly bug remains here. Watch:
>
> create table pk (a int primary key) partition by list (a);
> create table pk1 partition of pk for values in (1);
> create table fk (a int references pk);
> insert into pk values (1);
> insert into fk values (1);
> drop table pk cascade;
>
> Note that the DROP of the main PK table is pretty aggressive: since it
> cascades, you want it to drop the constraint on the FK side. This would
> work without a problem if 'pk' was a non-partitioned table, but in this
> case it fails:
>
> alvherre=# drop table pk cascade;
> NOTICE: drop cascades to constraint fk_a_fkey on table fk
> ERROR: removing partition "pk1" violates foreign key constraint "fk_a_fkey1"
> DETALLE: Key (a)=(1) still referenced from table "fk".
>
> The reason is that the "pre drop check" that's done before allow a drop
> of the partition doesn't realize that the constraint is also being
> dropped (and so the check ought to be skipped). If you were to do just
> "DROP TABLE pk1" then this complaint would be correct, since it would
> leave the constraint in an invalid state. But here, it's bogus and
> annoying. You can DELETE the matching values from table FK and then the
> DROP works. Here's another problem caused by the same misbehavior:
>
> alvherre=# drop table pk, fk;
> ERROR: removing partition "pk1" violates foreign key constraint "fk_a_fkey1"
> DETALLE: Key (a)=(1) still referenced from table "fk".
>
> Note here we want to get rid of table 'fk' completely. If you split it
> up in a DROP of fk, followed by a DROP of pk, it works.
>
> I'm not yet sure what's the best way to attack this. Maybe the
> "pre-drop check" needs a completely different approach. The simplest
> approach is to prohibit a table drop or detach for any partitioned table
> that's referenced by a foreign key, but that seems obnoxious and
> inconvenient.
Agreed.
Will it suffice or be OK if we skipped invoking the pre-drop callback for
objects that are being "indirectly" dropped? I came up with the attached
patch to resolve this problem, if that idea has any merit. We also get
slightly better error message as seen the expected/foreign_key.out changes.
Thanks,
Amit
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 11ec9d2f85..223ff9e4d8 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -237,17 +237,15 @@ deleteObjectsInList(ObjectAddresses *targetObjects,
Relation *depRel,
{
const ObjectAddress *thisobj = &targetObjects->refs[i];
Oid objectClass = getObjectClass(thisobj);
+ const ObjectAddressExtra *extra = &targetObjects->extras[i];
+ bool original = extra->flags & DEPFLAG_ORIGINAL;
if (trackDroppedObjectsNeeded() && !(flags &
PERFORM_DELETION_INTERNAL))
{
if (EventTriggerSupportsObjectClass(objectClass))
{
- bool original = false;
bool normal = false;
- const ObjectAddressExtra *extra =
&targetObjects->extras[i];
- if (extra->flags & DEPFLAG_ORIGINAL)
- original = true;
if (extra->flags & DEPFLAG_NORMAL ||
extra->flags & DEPFLAG_REVERSE)
normal = true;
@@ -256,6 +254,13 @@ deleteObjectsInList(ObjectAddresses *targetObjects,
Relation *depRel,
}
}
+ /*
+ * Do not invoke the callback for objects that are being dropped
+ * indirectly.
+ */
+ if (!original)
+ continue;
+
/* Invoke class-specific pre-deletion checks */
switch (objectClass)
{
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 6680f755dd..9305ba8017 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1837,14 +1837,11 @@ pre_drop_class_check(Oid relationId, Oid objectSubId)
relation = relation_open(relationId, NoLock);
/*
- * For leaf partitions, this is our last chance to verify any foreign
keys
+ * For partitions, this is our last chance to verify any foreign keys
* that may point to the partition as referenced table.
*/
- if (relation->rd_rel->relkind == RELKIND_RELATION &&
- relation->rd_rel->relispartition)
- CheckNoForeignKeyRefs(relation,
-
GetParentedForeignKeyRefs(relation),
- true);
+ if (relation->rd_rel->relispartition)
+ CheckNoForeignKeyRefs(relation,
GetParentedForeignKeyRefs(relation));
relation_close(relation, NoLock);
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8e26053a5c..ac4378db09 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15768,7 +15768,7 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
/* Ensure that foreign keys still hold after this detach */
refconstraints = GetParentedForeignKeyRefs(partRel);
- CheckNoForeignKeyRefs(partRel, refconstraints, false);
+ CheckNoForeignKeyRefs(partRel, refconstraints);
/* All inheritance related checks are performed within the function */
RemoveInheritance(partRel, rel);
@@ -16348,21 +16348,10 @@ GetParentedForeignKeyRefs(Relation partition)
* Returns a list of such constraints.
*/
void
-CheckNoForeignKeyRefs(Relation partition, List *constraints, bool isDrop)
+CheckNoForeignKeyRefs(Relation partition, List *constraints)
{
ListCell *cell;
- /*
- * In the DROP case, we can skip this check when this is a partitioned
- * partition, because its partitions will go through this also, and we'd
- * run the check twice uselessly.
- *
- * In the DETACH case, this is only called for the top-level relation,
so
- * we must run it nevertheless.
- */
- if (isDrop && partition->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- return;
-
foreach(cell, constraints)
{
Oid constrOid = lfirst_oid(cell);
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index efbb5c3a93..03783abc6d 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -53,8 +53,7 @@ extern void AlterRelationNamespaceInternal(Relation classRel,
Oid relOid,
extern void CheckTableNotInUse(Relation rel, const char *stmt);
extern List *GetParentedForeignKeyRefs(Relation partition);
-extern void CheckNoForeignKeyRefs(Relation partition, List *constraints,
- bool isDrop);
+extern void CheckNoForeignKeyRefs(Relation partition, List *constraints);
extern void ExecuteTruncate(TruncateStmt *stmt);
extern void ExecuteTruncateGuts(List *explicit_rels, List *relids, List
*relids_logged,
diff --git a/src/test/regress/expected/foreign_key.out
b/src/test/regress/expected/foreign_key.out
index 810a689ab5..dd88167062 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -2143,8 +2143,8 @@ drop table droppk1;
ERROR: removing partition "droppk1" violates foreign key constraint
"dropfk_a_fkey1"
DETAIL: Key (a)=(1) still referenced from table "dropfk".
drop table droppk2;
-ERROR: removing partition "droppk2_d" violates foreign key constraint
"dropfk_a_fkey4"
-DETAIL: Key (a)=(1500) still referenced from table "dropfk".
+ERROR: removing partition "droppk2" violates foreign key constraint
"dropfk_a_fkey2"
+DETAIL: Key (a)=(1000) still referenced from table "dropfk".
drop table droppk21;
ERROR: removing partition "droppk21" violates foreign key constraint
"dropfk_a_fkey3"
DETAIL: Key (a)=(1000) still referenced from table "dropfk".
