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

(2010/01/28 5:42), Robert Haas wrote:
> On Wed, Jan 27, 2010 at 10:17 AM, KaiGai Kohei<kaigai@kaigai.gr.jp>  wrote:
>> (2010/01/27 23:29), Robert Haas wrote:
>>>
>>> 2010/1/27 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>>>>
>>>> The attached patch is revised one based on the V3 approach.
>>>> The only difference from V3 is that it also applies checks on the
>>>> AT_AlterColumnType option, not only renameatt().
>>>
>>> I think I was clear about what the next step was for this patch in my
>>> previous email, but let me try again.
>>>
>>> http://archives.postgresql.org/pgsql-hackers/2010-01/msg02407.php
>>>
>>> See also Tom's comments here:
>>>
>>> http://archives.postgresql.org/pgsql-hackers/2010-01/msg00110.php
>>>
>>> I don't believe that either Tom or I are prepared to commit a patch
>>> based on this approach, at least not unless someone makes an attempt
>>> to do it the other way and finds an even more serious problem.  If
>>> you're not interested in rewriting the patch along the lines Tom
>>> suggested, then we should just mark this as Returned with Feedback and
>>> move on.
>>
>> The V3/V5 patch was the rewritten one based on the Tom's comment, as is.
>> It counts the expected inhcount at the first find_all_inheritors() time
>> at once, and it compares the pg_attribute.attinhcount.
>> (In actually, find_all_inheritors() does not have a capability to count
>> the number of merged from a common origin, so I newly defined the
>> find_all_inheritors_with_inhcount().)
>>
>> Am I missing something?
> 
> Err... I'm not sure.  I thought I understood what the different
> versions of this patch were doing, but apparently I'm all confused.
> I'll take another look at this.

Just to be safe, I'd like to introduce the differences between revisions.

* The V2 patch
http://archives.postgresql.org/message-id/4B3F6353.3080105@kaigai.gr.jp

It just checked whether the pg_attribute.inhcount is larger than 1, or not.
But it was problematic when diamond-inheritance case.

* The V3 patch
http://archives.postgresql.org/message-id/4B41BB04.2070609@ak.jp.nec.com

It computed an expected-inhcount for each relations when renameatt()
picks up all the child relations on the top of recursion level.
Its cost to walk on the inheritance tree is similar to existing code
except for a case when we have many diamond-inheritance, because
find_all_inheritors() does not need to walk on the child relations
that was already checked, but find_all_inheritors_with_inhcount()
has to walk on these children to compute multiplicity.

See the following example,
  T2 /  \
T1    T4 - T5 \  /  T3

In this case, find_all_inheritors() encounter T4 with T1-T2 path and
T1-T3 path. But it does not need to scan T5 on the second time,
because it already chains OID of the T4 and T5 with the first walks,
and list_concat_unique_oid() does not add duplicated OIDs anymore.

But find_all_inheritors_with_inhcount() needs to walks on the T4-T5 path
to compute the number of times being inherited, even if second walks.
Your testcase emphasized this difference so much. But, in my opinion,
it is quite natural because the existing code does not apply necessary
checks.

* The V4 patch
http://archives.postgresql.org/message-id/4B4EC1F1.30108@ak.jp.nec.com

I found out ALTER COLUMN TYPE also has same issue.
But ATPrepAlterColumnType() did recursive scan using ATSimpleRecursion(),
so it needs to modify the logic in this function. At that time, I preferred
to compute an expected inhcount for each recursion level, rather than
modifying the logic in ATPrepAlterColumnType(), although its cost was
larger than find_all_inheritors_with_inhcount().

* The V5 patch
http://archives.postgresql.org/message-id/4B5FFE4B.1060505@ak.jp.nec.com

We can find out a performance matter in the V4 patch, so I reverted the
base logic into V3 approach. In addition to the reverting, I also modified
the ATPrepAlterColumnType() to check whether the column to be altered
is inherited from multiple origin, or not.

> Bernd (or anyone), feel free to take a look in parallel.  More eyes
> would be helpful...
> 
> ...Robert
> 
-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>