Skip site navigation (1) Skip section navigation (2)

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

From: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
To: Bernd Helmle <mailings(at)oopsware(dot)de>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>, pgsql-hackers(at)postgresql(dot)org, Thom Brown <thombrown(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)commandprompt(dot)com>
Subject: Re: [BUG?] strange behavior in ALTER TABLE ... RENAME TO on inherited columns
Date: 2010-01-27 08:50:19
Message-ID: 4B5FFE4B.1060505@ak.jp.nec.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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>

Attachment: pgsql-fix-inherit-rename.5.patch
Description: application/octect-stream (15.2 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Leonardo FDate: 2010-01-27 09:15:38
Subject: Re: About "Our CLUSTER implementation is pessimal" patch
Previous:From: Alex HunsakerDate: 2010-01-27 07:46:42
Subject: Re: Add on_perl_init and proper destruction to plperl [PATCH]

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group