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 Herrera https://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(&conobj, behavior, 0);
+ add_exact_object_address(&conobj, 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);
}
