Re: [BUG?] strange behavior in ALTER TABLE ... RENAME TO on inherited columns

(2010/02/02 3:01), Robert Haas wrote:
> 2010/1/31 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> The attached patch modified find_all_inheritors() to return the list of
>> expected inhcount, if List * pointer is given. And, it focuses on only
>> the bugs in renameatt() case.
>
> I have cleaned up and simplified this patch. Attached is the version
> I intend to commit. Changes:
>
> - make find_all_inheritors return the list of OIDs, as previously,
> instead of using an out parameter
> - undo some useless variable renamings
> - undo some useless whitespace changes
> - rework comments
> - fix regression tests to avoid using the same alias twice in the same query
> - add an ORDER BY clause to the regression tests so that they pass on my machine
> - improve the names of some of the new variables
> - instead of adding an additional argument to renameatt(), just
> replace the existing 'bool recursing' with 'int expected_parents'.
> This allows merging the two versions of the "cannot rename inherited
> column" message together, which seems like a reasonably good idea to
> me, though if someone has a better idea that's fine. I didn't think
> the one additional word added to the message provided enough clarity
> to make it worth creating another translatable string.

Thanks for your checks.

>> Also, the ALTER COLUMN TYPE case should be also fixed in the 9.1 release
>> (or 9.0.1?).
>
> If the fix is something we could commit for 9.0.1, then we ought to do
> it now before 9.0 is released. If you want to submit a follow-on
> patch to address ALTER COLUMN TYPE once this is committed, then please
> do so.

The attached patch also fixes ALTER COLUMN TYPE case.

It replaced the 'recursing' argument in ATPrepCmd() by 'expected_parents',
and it is delivered to ATPrepAlterColumnType(). The logic to forbid altering
the column type is same as renameatt().
ATSimpleRecursion() is also modified to use forboth() to call ATPrepCmd()
recursively.

One concern is at ATOneLevelRecursion() which is called by ATPrepAddColumn()
only, and it also calls ATPrepCmd() for the direct children.
Right now, I set 1 on the 'expected_parents'. However, IMO, here is no reason
why we cannot rewrite the ATPrepAddColumn() using ATSimpleRecursion().

Eventually, ATExecAddColumn() shall be invoked several times for same column,
if the inheritance tree has diamond-inheritance structure. And, it increments
pg_attribute.attinhcount except for the first invocation.
If we store the 'expected_parents' on the ColumnDef->inhcount, we don't need
to call the ATExecAddColumn() more than once in a single ALTER TABLE command.

Any comments? And, when should we do it? 9.0? 9.1?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>