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

(2010/01/29 9:29), Robert Haas wrote:
> 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.

Ahh, it is right. I was confused.

Is it possible to introduce the logic mathematical-strictly?
Now I'm considering whether the find_all_inheritors() logic is suitable
for any cases, or not.

What we want to compute here is:

WITH RECURSIVE r AS (
SELECT 't1'::regclass AS inhrelid
UNION ALL
SELECT c.inhrelid FROM pg_inherits c, r WHERE r.inhrelid = c.inhparent
) -- r is all the child relations inherited from 't1'
SELECT inhrelid::regclass, count(*) FROM pg_inherits
WHERE inhparent IN (SELECT inhrelid FROM r) GROUP BY inhrelid;

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