On Fri, Jan 25, 2019 at 12:08 AM Alvaro Herrera
<alvherre@2ndquadrant.com> 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(&conobj, 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