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

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().

The performance was almost same as the V3 case.

> * CVS HEAD
> 0.828s
> 0.828s
> 0.833s
> 0.829s
> 0.838s

- ALTER RENAME TO with V5 patch
2.419s
2.418s
2.418s
2.426s

I also checked ALTER ... TYPE cases. It is relatively heavy operation than
renameatt(), so its affects was relatively smaller.

- ALTER ... TYPE with CVS HEAD
28.888s
29.948s
30.738s
30.600s

- ALTER ... TYPE with V5 patch
28.067s
28.212s
28.038s
29.497s

(2010/01/26 10:10), KaiGai Kohei wrote:
> (2010/01/26 1:11), Bernd Helmle wrote:
>>
>>
>> --On 25. Januar 2010 11:39:21 +0900 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>
>> wrote:
>>
>>> (echo "CREATE TABLE t (a int);"
>>> for i in `seq 0 9`; do
>>> echo "CREATE TABLE s$i (b int) INHERITS(t);"
>>> for j in `seq 0 9`; do
>>> echo "CREATE TABLE v$i$j (c int) INHERITS(s$i);"
>>> for k in `seq 0 9`; do
>>> echo "CREATE TABLE w$i$j$k (d int) INHERITS(v$i$j);"
>>> for l in `seq 0 9`; do
>>> echo "CREATE TABLE x$i$j$k$l (e int) INHERITS(w$i$j$k);"
>>> done
>>> done
>>> done
>>> done) | psql test
>>
>> Well, each table inherits one table in your test. In my test, I inherit
>> from multiple tables for each table. My script generates the following
>> inheritance tree (and wins a price of copy& paste ugliness, see
>> attachment):
>>
>> A1, A2, A3, ..., Am
>> B1 INHERITS(A1...A10), B2 INHERITS(A1...A10, B3 INHERITS(A1...A10), ...Bn
>> C1 INHERITS(B1...B10), C2 INHERITS(B1...B10), ... Co
>> D1 INHERITS(C1...C10), ..., Dp
>>
>> m = 10
>> n = 10
>> o = 10
>> p = 1000
>>
>> Repeating this on my MacBook gives:
>>
>> ALTER TABLE a1 RENAME COLUMN acol1 TO xyz;
>>
>> -HEAD:
>>
>> Time: 382,427 ms
>> Time: 375,974 ms
>> Time: 385,478 ms
>> Time: 371,067 ms
>> Time: 410,834 ms
>> Time: 386,382 ms
>>
>> Recent V4 patch:
>>
>> Time: 6065,673 ms
>> Time: 3823,206 ms
>> Time: 4037,933 ms
>> Time: 3873,029 ms
>> Time: 3899,607 ms
>> Time: 3963,308 ms
>
> Hmm... I also could observe similar result in 4 times iteration of
> ALTER TABLE with your test_rename.sql.
> I agree the recent V4 patch is poor in performance perspective.
>
> * CVS HEAD
> 0.828s
> 0.828s
> 0.833s
> 0.829s
> 0.838s
>
> * Rcent V4 patch:
> 10.283s
> 10.135s
> 10.107s
> 10.382s
> 10.162s
>
> * Previous V3 patch:
> 2.607s
> 2.429s
> 2.431s
> 2.436s
> 2.428s
>
> The V3 patch is designed to compute an expected inhcount for each relations
> to be altered at first, then it shall be compared to pg_attribute.inhcount
> to be renamed.
>
> Basically, its execution cost is same order except for a case when a relation
> has diamond inheritance tree.
>
> The find_all_inheritors() does not check child relations which is already
> scanned. However, in this case, we have to check how many times is the child
> relation inherited from a common origin.
> I guess it is reason of the different between -HEAD and V3.
>
> For example, if we have the following inheritance tree,
>
> A2 A5
> / \ \
> A1 A4
> \ / \
> A3 -- A6
>
> The find_all_inheritors() checks existence of directly inherited relations
> at A1, ... , A6 without any duplications, because this function does not
> intend to compute how many times was it inherited.
>
> The find_all_inheritors_with_inhcount() in V3 patch checks existence of
> directly inherited relations, even if the target relation is already checked,
> because it also has to return the times to be inherited from a common origin.
> In this example, it considers the above structure is same as the following
> tree. In this diagram, we can find A4 and A5 twice, and A6 thrice.
>
> A5
> /
> A2 - A4 - A6
> \
> A1
> \
> A3 - A4 - A6
> \ \
> A6 A5
>
> Thus, the test_rename.sql was the worst case test for V3 also.
> However, I don't think we should keep the bug in the next release.
> The CVS HEAD's performance is the result of omission for necessary checks.
>
> I think we should back to the V3 patch approach, and also reconsider
> the logic in ATPrepAlterColumnType().
>
> Thanks,

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