On Fri, Jan 25, 2019 at 12:08 AM Alvaro Herrera <alvhe...@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