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-26 01:10:58
Message-ID: 4B5E4122.20608@ak.jp.nec.com (view raw or flat)
Thread:
Lists: pgsql-hackers
(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>

In response to

Responses

pgsql-hackers by date

Next:From: Tom LaneDate: 2010-01-26 01:22:31
Subject: Re: About "Our CLUSTER implementation is pessimal" patch
Previous:From: Greg SmithDate: 2010-01-26 00:29:29
Subject: Re: Dividing progress/debug information in pg_standby, and stat before copy

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