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

2010/1/28 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> (2010/01/29 0:46), Robert Haas wrote:
>> 2010/1/27 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> Hmm, indeed, this logic (V3/V5) is busted.
>>>
>>> The idea of V4 patch can also handle this case correctly, although it
>>> is lesser in performance.
>>> I wonder whether it is really unacceptable cost in performance, or not.
>>> Basically, I assume ALTER TABLE RENAME/TYPE is not frequent operations,
>>> and I don't think this bugfix will damage to the reputation of PostgreSQL.
>>>
>>> Where should we go on the next?
>>
>> Isn't the problem here just that the following comment is 100% wrong?
>>
>>                  /*
>>                   * Unlike find_all_inheritors(), we need to walk on
>> child relations
>>                   * that have diamond inheritance tree, because this
>> function has to
>>                   * return correct expected inhecount to the caller.
>>                   */
>>
>> It seems to me that the right solution here is to just add one more
>> argument to find_all_inheritors(), something like List
>> **expected_inh_count.
>>
>> Am I missing something?
>
> The find_all_inheritors() does not walk on child relations more than
> two times, even if a child has multiple parents inherited from common
> origin, because list_concat_unique_oid() ignores the given OID if it
> is already on the list. It means all the child relations under the
> relation already walked on does not checked anywhere. (Of course,
> this assumption is correct for the purpose of find_all_inheritors()
> with minimum cost.)
>
> What we want to do here is to compute the number of times a certain
> child relation is inherited from a common origin; it shall be the
> expected-inhcount. So, we need an arrangement to the logic.
>
> For example, see the following diagram.
>
>   T2
>  /  \
> T1    T4---T5
>  \  /
>   T3
>
> If we call find_all_inheritors() with T1. The find_inheritance_children()
> returns T2 and T3 for T1.
> Then, it calls find_inheritance_children() for T2, and it returns T4.
> Then, it calls find_inheritance_children() for T3, and it returns T4, but
> it is already in the "rels_list", so list_concat_unique_oid() ignores it.
> Then, it calls find_inheritance_children() for T4, and it returns T5.
>
> In this example, we want the expected inhcount for T2 and T3 should be 1,
> for T4 and T5 should be 2. However, it walks on T4 and T5 only once, so
> they will have 1 incorrectly.
> Even if we count up the ignored OID (T4), find_all_inheritors() does not
> walk on T5, because it is already walked on obviously when T4 is ignored.

I think the count for T5 should be 1, and I think that the count for
T4 can easily be made to be 2 by coding the algorithm correctly.

...Robert