Re: patch for check constraints using multiple inheritance

On Mon, Aug 2, 2010 at 10:47 AM, Yeb Havinga <yebhavinga@gmail.com> wrote:
>>> The attached patch uses the globally defined list.
>>
>> I can't speak for any other committer, but personally I'm prepared to
>> reject out of hand any solution involving a global variable.  This
>> code is none to easy to follow as it is and adding global variables to
>> the mix is not going to make it better, even if we can convince
>> ourselves that it's safe and correct.  This is something of a corner
>> case, so I won't be crushed if a cleaner fix is too invasive to
>> back-patch.
>
> Thanks for looking at the patch. I've attached a bit more wordy version,
> that adds a boolean to AlteredTableInfo and a function to wipe that boolean
> between processing of subcommands.

I don't think that this is much cleaner than the global variable
solution; you haven't really localized that need to know about the new
flag in any meaningful way, the hacks in ATOneLevelRecusion()
basically destroy any pretense of that code possibly being reusable
for some other caller.  However, there's a more serious problem, which
is that it doesn't in general fix the bug: try it with the
top1/top2/bottom/basement example I posted upthread.  If you add the
same column to both top1 and top2 and then drop it in both top1 and
top2, basement ends up with a leftover copy.  The problem is that
"only visit each child once" is not the right algorithm; what you need
to do is "only visit the descendents of each child if no merge
happened at the parent".  I believe that the only way to do this
correct is to merge the prep stage into the execution stage, as the
code for adding constraints already does.  At the prep stage, you
don't have enough information to determine which relations you'll
ultimately need to update, since you don't know where the merges will
happen.

>>  Incidentally, we need to shift this discussion to a new
>> subject line; we have a working patch for the original subject of this
>> thread, and are now discussing the related problem I found with
>> attributes.
>>
>
> Both solutions have changes in callers of 'find_inheritance_children'. I was
> working in the hope a unifiying solution would pop up naturally, but so far
> it has not. Checking of the new AlteredTableInfo->relVisited boolean could
> perhaps be pushed down into find_inheritance_children, if multiple-'doing
> things' for the childs under a multiple-inheriting relation is unwanted for
> every kind of action. It seems to me that the question whether that is the
> case must be answered, before the current working patch for coninhcount is
> 'ready for committer'.

I am of the opinion that the chances of a unifying solution popping up
are pretty much zero.  :-)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company