On Mon, Aug 2, 2010 at 10:47 AM, Yeb Havinga <yebhavi...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
