Robert Haas wrote:
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.
Hello Robert,
Again thanks for looking at the patch. Unfortunately I missed the
top1/top2 example earlier, but now I've seen it I understand that it is
impossible to fix this problem during the prep stage, without looking at
actual existing columns, i.e. without the merge code. Running the
top1/top2 example I'd also have expected an error while adding the
column to the second top, since the columns added to top1 and top2 are
from a different origin. It is apparently considered good behaviour,
however troubles emerge when e.g. trying to rename a_table_column in the
top1/top2 example, where that is no problem in the 'lollipop' structure,
that has a single origin.
ALTER TABLE top RENAME COLUMN a_table_column TO another_table_column;
SELECT t.oid, t.relname, a.attname, a.attinhcount
FROM pg_class t
JOIN pg_attribute a ON (a.attrelid = t.oid)
JOIN pg_namespace n ON (t.relnamespace = n.oid)
WHERE n.nspname = 'test_inheritance' AND a.attname LIKE '%table_column%'
ORDER BY oid;
I do not completely understand what you mean with the destruction of
reusability of ATOneLevelRecursion, currently only called by
ATPrepAddColumn - in the patch it is documented in the definition of
relVisited that is it visit info for each subcommand. The loop over
subcommands is in ATController, where the value is properly reset for
each all current and future subcommands. Hence the ATOneLevelRecursion
routing is usable in its current form for all callers during the prep
stage, and not only ATPrepAddColumn.
I am of the opinion that the chances of a unifying solution popping up
are pretty much zero. :-)
Me too, now I understand the fixes must be in the execution stages.
regards,
Yeb Havinga
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
