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

KaiGai Kohei wrote:

postgres=# SELECT * FROM t2;
ERROR: could not find inherited attribute "b" of relation "t3"

Because t3.b is also inherited from the t2, but ALTER TABLE does not
care about multiple inherited columns well.

I think we should not allow to rename a column with attinhcount > 1.

I think we should fix ALTER TABLE to cope with multiple inheritance.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2009/11/4 Alvaro Herrera <alvherre@commandprompt.com>:

KaiGai Kohei wrote:

  postgres=# SELECT * FROM t2;
  ERROR:  could not find inherited attribute "b" of relation "t3"

Because t3.b is also inherited from the t2, but ALTER TABLE does not
care about multiple inherited columns well.

I think we should not allow to rename a column with attinhcount > 1.

I think we should fix ALTER TABLE to cope with multiple inheritance.

I'd be interested to see how this should work. Given KaiGai's
example, how would that be resolved? That column would already be
merged for the inheriting table, so would renaming it somehow unmerge
it? Given an insertion into t3 would propagate to both column b's in
table t1 and t2, and then renaming b in t1 wouldn't make sense. Or
would renaming it be prevented due to dependants? Or should t3's
inheritance of t1 and t2 implicitly bind t1's and t2's column b to one
another so that one affects the other? (i.e. renaming column b in t1
would also rename column b in t2, or both would require renaming
during the same transaction.)

...or something less confusing. :)

Thom

Thom Brown <thombrown@gmail.com> writes:

2009/11/4 Alvaro Herrera <alvherre@commandprompt.com>:

KaiGai Kohei wrote:

I think we should not allow to rename a column with attinhcount > 1.

I think we should fix ALTER TABLE to cope with multiple inheritance.

I'd be interested to see how this should work.

Yeah. I don't think a "fix" is possible, because there is no
non-astonishing way for it to behave. I think KaiGai is right that
forbidding the rename is the best solution.

regards, tom lane

Tom Lane wrote:

Thom Brown <thombrown@gmail.com> writes:

2009/11/4 Alvaro Herrera <alvherre@commandprompt.com>:

KaiGai Kohei wrote:

I think we should not allow to rename a column with attinhcount > 1.

I think we should fix ALTER TABLE to cope with multiple inheritance.

I'd be interested to see how this should work.

Yeah. I don't think a "fix" is possible, because there is no
non-astonishing way for it to behave. I think KaiGai is right that
forbidding the rename is the best solution.

The attached patch forbids rename when the attribute is inherited
from multiple parents.

postgres=# CREATE TABLE t1 (a int, b int);
CREATE TABLE
postgres=# CREATE TABLE t2 (b int, c int);
CREATE TABLE
postgres=# CREATE TABLE t3 (d int) INHERITS (t1, t2);
NOTICE: merging multiple inherited definitions of column "b"
CREATE TABLE
postgres=# SELECT * FROM t3;
a | b | c | d
---+---+---+---
(0 rows)

postgres=# ALTER TABLE t1 RENAME b TO x;
ERROR: cannot rename multiple inherited column "b"

The regression test detected a matter in the misc test.

It tries to rename column "a" of "a_star" table, but it failed due to
the new restriction.

--
-- test the "star" operators a bit more thoroughly -- this time,
-- throw in lots of NULL fields...
--
-- a is the type root
-- b and c inherit from a (one-level single inheritance)
-- d inherits from b and c (two-level multiple inheritance)
-- e inherits from c (two-level single inheritance)
-- f inherits from e (three-level single inheritance)
--
CREATE TABLE a_star (
class char,
a int4
);

CREATE TABLE b_star (
b text
) INHERITS (a_star);

CREATE TABLE c_star (
c name
) INHERITS (a_star);

CREATE TABLE d_star (
d float8
) INHERITS (b_star, c_star);

At the misc test,

  --- 242,278 ----
    ALTER TABLE c_star* RENAME COLUMN c TO cc;
    ALTER TABLE b_star* RENAME COLUMN b TO bb;
    ALTER TABLE a_star* RENAME COLUMN a TO aa;
  + ERROR:  cannot rename multiple inherited column "a"
    SELECT class, aa
       FROM a_star* x
       WHERE aa ISNULL;
  ! ERROR:  column "aa" does not exist
  ! LINE 1: SELECT class, aa
  !

It seems to me it is a case the regression test to be fixed up.
(We don't have any reasonable way to know whether a certain attribute
has a same source, or not.)

Any comments?
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

It is a patch for the matter which I reported before.

When a column is inherited from multiple relations, ALTER TABLE with
RENAME TO option is problematic.
This patch fixes the matter. In correctly, it prevent to rename columns
inherited from multiple relations and merged.

Also see the past discussion:
http://archives.postgresql.org/pgsql-hackers/2009-11/msg00138.php

postgres=# CREATE TABLE t1 (a int, b int);
CREATE TABLE
postgres=# CREATE TABLE t2 (b int, c int);
CREATE TABLE
postgres=# CREATE TABLE t3 (x text) inherits (t1, t2);
NOTICE: merging multiple inherited definitions of column "b"
CREATE TABLE
postgres=# SELECT * FROM t3;
a | b | c | x
---+---+---+---
(0 rows)

It looks to me fine.

postgres=# ALTER TABLE t1 RENAME b TO y;
ALTER TABLE
postgres=# SELECT * FROM t3;
a | y | c | x
---+---+---+---
(0 rows)

postgres=# SELECT * FROM t1;
a | y
---+---
(0 rows)

It looks to me fine.

postgres=# SELECT * FROM t2;
ERROR: could not find inherited attribute "b" of relation "t3"

Oops, when we refer the t3 via t2, it expects the inherited relation
also has the column "b", but it was already renamed.

One trouble is regression test. The misc_create test create a_star
table, then it is inherited by b_star and c_star, then these are
inherited to d_star table. Then misc test rename the a_star.a, but
this patch prevent it.

It looks like works well, but it is a corner case, because d_star.a
is eventually inherited from a_star via b_star and c_star, and these
are all the inherited relations.
In generally, we don't have reasonable way to rename all the related
columns upper and lower of the inheritance relationship.

Thanks,

(2009/11/05 9:57), KaiGai Kohei wrote:

Tom Lane wrote:

Thom Brown<thombrown@gmail.com> writes:

2009/11/4 Alvaro Herrera<alvherre@commandprompt.com>:

KaiGai Kohei wrote:

I think we should not allow to rename a column with attinhcount> 1.

I think we should fix ALTER TABLE to cope with multiple inheritance.

I'd be interested to see how this should work.

Yeah. I don't think a "fix" is possible, because there is no
non-astonishing way for it to behave. I think KaiGai is right that
forbidding the rename is the best solution.

The attached patch forbids rename when the attribute is inherited
from multiple parents.

postgres=# CREATE TABLE t1 (a int, b int);
CREATE TABLE
postgres=# CREATE TABLE t2 (b int, c int);
CREATE TABLE
postgres=# CREATE TABLE t3 (d int) INHERITS (t1, t2);
NOTICE: merging multiple inherited definitions of column "b"
CREATE TABLE
postgres=# SELECT * FROM t3;
a | b | c | d
---+---+---+---
(0 rows)

postgres=# ALTER TABLE t1 RENAME b TO x;
ERROR: cannot rename multiple inherited column "b"

The regression test detected a matter in the misc test.

It tries to rename column "a" of "a_star" table, but it failed due to
the new restriction.

--
-- test the "star" operators a bit more thoroughly -- this time,
-- throw in lots of NULL fields...
--
-- a is the type root
-- b and c inherit from a (one-level single inheritance)
-- d inherits from b and c (two-level multiple inheritance)
-- e inherits from c (two-level single inheritance)
-- f inherits from e (three-level single inheritance)
--
CREATE TABLE a_star (
class char,
a int4
);

CREATE TABLE b_star (
b text
) INHERITS (a_star);

CREATE TABLE c_star (
c name
) INHERITS (a_star);

CREATE TABLE d_star (
d float8
) INHERITS (b_star, c_star);

At the misc test,

--- 242,278 ----
ALTER TABLE c_star* RENAME COLUMN c TO cc;
ALTER TABLE b_star* RENAME COLUMN b TO bb;
ALTER TABLE a_star* RENAME COLUMN a TO aa;
+ ERROR:  cannot rename multiple inherited column "a"
SELECT class, aa
FROM a_star* x
WHERE aa ISNULL;
! ERROR:  column "aa" does not exist
! LINE 1: SELECT class, aa
!

It seems to me it is a case the regression test to be fixed up.
(We don't have any reasonable way to know whether a certain attribute
has a same source, or not.)

Any comments?

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

2009/12/16 KaiGai Kohei <kaigai@ak.jp.nec.com>:

It is a patch for the matter which I reported before.

When a column is inherited from multiple relations, ALTER TABLE with
RENAME TO option is problematic.
This patch fixes the matter. In correctly, it prevent to rename columns
inherited from multiple relations and merged.

No longer applies. Can you rebase?

Thanks,

...Robert

(2009/12/30 10:38), Robert Haas wrote:

2009/12/16 KaiGai Kohei<kaigai@ak.jp.nec.com>:

It is a patch for the matter which I reported before.

When a column is inherited from multiple relations, ALTER TABLE with
RENAME TO option is problematic.
This patch fixes the matter. In correctly, it prevent to rename columns
inherited from multiple relations and merged.

No longer applies. Can you rebase?

The attached patch is the rebased revision.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

KaiGai Kohei <kaigai@kaigai.gr.jp> writes:

(2009/12/30 10:38), Robert Haas wrote:

No longer applies. Can you rebase?

The attached patch is the rebased revision.

I'm not really impressed with this patch, because it will reject
perfectly legitimate multiple-inheritance cases (ie, cases where there's
more than one inheritance path from the same parent). This works fine
at the moment:

regression=# create table p1(f1 int, f2 int);
CREATE TABLE
regression=# create table c1(f3 int) inherits (p1);
CREATE TABLE
regression=# create table c2(f4 int) inherits (p1);
CREATE TABLE
regression=# create table cc(f5 int) inherits (c1,c2);
NOTICE: merging multiple inherited definitions of column "f1"
NOTICE: merging multiple inherited definitions of column "f2"
CREATE TABLE
regression=# \d cc
Table "public.cc"
Column | Type | Modifiers
--------+---------+-----------
f1 | integer |
f2 | integer |
f3 | integer |
f4 | integer |
f5 | integer |
Inherits: c1,
c2

regression=# alter table p1 rename f2 to ff2;
ALTER TABLE
regression=# \d cc
Table "public.cc"
Column | Type | Modifiers
--------+---------+-----------
f1 | integer |
ff2 | integer |
f3 | integer |
f4 | integer |
f5 | integer |
Inherits: c1,
c2

I don't think that protecting against cases where things won't work
is an adequate reason for breaking cases that do work.

regards, tom lane

On Sat, Jan 2, 2010 at 2:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

KaiGai Kohei <kaigai@kaigai.gr.jp> writes:

(2009/12/30 10:38), Robert Haas wrote:

No longer applies.  Can you rebase?

The attached patch is the rebased revision.

I'm not really impressed with this patch, because it will reject
perfectly legitimate multiple-inheritance cases (ie, cases where there's
more than one inheritance path from the same parent).  This works fine
at the moment:

[...]

I don't think that protecting against cases where things won't work
is an adequate reason for breaking cases that do work.

Upthread you appeared to be endorsing what KaiGai has implemented here:

http://archives.postgresql.org/pgsql-hackers/2009-11/msg00147.php

Rereading this a few times, perhaps you meant that we should prohibit
renaming an ancestor when one of its descendents has a second and
distinct ancestor, but the email you actually sent reads as if you
were endorsing a blanket prohibition when attinhcount > 1. Can you
clarify?

Thanks,

...Robert

Robert Haas <robertmhaas@gmail.com> writes:

Upthread you appeared to be endorsing what KaiGai has implemented here:
http://archives.postgresql.org/pgsql-hackers/2009-11/msg00147.php

No, I said that forbidding conflicting renames would be a good solution.
I did not endorse any specific means of testing for such a conflict.
The one in this patch is not good enough to avoid breaking cases that
actually are perfectly OK.

It would be possible to detect the problematic cases correctly by first
descending the inheritance tree and counting the number of arrivals at
different children, and then doing it again and complaining if
attinhcount was different from the count obtained the first time.
This could probably be done by modifying find_all_inheritors to count
duplicate occurrences rather than just discarding them. Whether it's
worth it is not clear.

In practice the reasonable engineering alternatives may just be to do
what KaiGai's patch does, or to do nothing. In that case I think a good
argument can be made for the latter. Nobody has ever complained about
this from the field AFAIR; but we might get complaints if we disable
cases that used to work fine.

regards, tom lane

On Jan 3, 2010, at 12:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

In practice the reasonable engineering alternatives may just be to do
what KaiGai's patch does, or to do nothing. In that case I think a
good
argument can be made for the latter. Nobody has ever complained about
this from the field AFAIR; but we might get complaints if we disable
cases that used to work fine.

Maybe. The current behavior of allowing the rename but then breaking
queries certainly isn't awesome. I think if someone is willing to
implement a more careful check we should accept it.

...Robert

(2010/01/04 4:06), Robert Haas wrote:

On Jan 3, 2010, at 12:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

In practice the reasonable engineering alternatives may just be to do
what KaiGai's patch does, or to do nothing. In that case I think a good
argument can be made for the latter. Nobody has ever complained about
this from the field AFAIR; but we might get complaints if we disable
cases that used to work fine.

Maybe. The current behavior of allowing the rename but then breaking
queries certainly isn't awesome. I think if someone is willing to
implement a more careful check we should accept it.

The condition to prevent problematic renaming might be modified to handle
diamond inheritances correctly.

The current patch raises an error when pg_attribute.inhcount > 1.
But, in actually, it should raise an error when the number of origins
of the attribute to be renamed is larger than 1.
It shall be match with the inhcount unless it does not have diamond
inheritances.

We can easily check the number of origins with walking on the pg_inherits
catalog. So, it seems to me the condition should be rewritten like:

BEFORE:
if (attform->attinhcount > 1)
ereport(ERROR, ...);

AFTER:
if (number_of_attribute_origin(myrelid, oldattname) > 1)
ereport(ERROR, ...);

Am I missing something?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

2010/1/3 KaiGai Kohei <kaigai@ak.jp.nec.com>:

(2010/01/04 4:06), Robert Haas wrote:

On Jan 3, 2010, at 12:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

In practice the reasonable engineering alternatives may just be to do
what KaiGai's patch does, or to do nothing. In that case I think a good
argument can be made for the latter. Nobody has ever complained about
this from the field AFAIR; but we might get complaints if we disable
cases that used to work fine.

Maybe. The current behavior of allowing the rename but then breaking
queries certainly isn't awesome. I think if someone is willing to
implement a more careful check we should accept it.

The condition to prevent problematic renaming might be modified to handle
diamond inheritances correctly.

The current patch raises an error when pg_attribute.inhcount > 1.
But, in actually, it should raise an error when the number of origins
of the attribute to be renamed is larger than 1.
It shall be match with the inhcount unless it does not have diamond
inheritances.

We can easily check the number of origins with walking on the pg_inherits
catalog. So, it seems to me the condition should be rewritten like:

BEFORE:
 if (attform->attinhcount > 1)
     ereport(ERROR, ...);

AFTER:
 if (number_of_attribute_origin(myrelid, oldattname) > 1)
     ereport(ERROR, ...);

Am I missing something?

That sounds about right to me, though that function doesn't exist yet. :-)

...Robert

Robert Haas <robertmhaas@gmail.com> writes:

2010/1/3 KaiGai Kohei <kaigai@ak.jp.nec.com>:

�if (number_of_attribute_origin(myrelid, oldattname) > 1)
� � �ereport(ERROR, ...);

Am I missing something?

That sounds about right to me,

It looks remarkably inefficient to me. Do you propose to search the
entire database's inheritance tree to derive that number? And do it
over again at each child table? The method I suggested would allow the
necessary information to be extracted during the initial search for
child tables, which we have to do anyway.

regards, tom lane

(2010/01/04 13:18), Tom Lane wrote:

Robert Haas<robertmhaas@gmail.com> writes:

2010/1/3 KaiGai Kohei<kaigai@ak.jp.nec.com>:

�if (number_of_attribute_origin(myrelid, oldattname)> 1)
� � �ereport(ERROR, ...);

Am I missing something?

That sounds about right to me,

It looks remarkably inefficient to me. Do you propose to search the
entire database's inheritance tree to derive that number? And do it
over again at each child table?

Yes,

The method I suggested would allow the
necessary information to be extracted during the initial search for
child tables, which we have to do anyway.

find_all_inheritors() returns a clean list which does not contain
duplicated OID of the inherited relation, so it seems to me we need
to change the function prototype but it affects other parts, or to add
a new function which also returns number of duplications, not only OIDs.

Or, we can call find_inheritance_children() in renameatt() as if
find_all_inheritors() doing except for list_concat_unique_oid().

What is the most preferable?
I don't have any preference in the way to fix the problem.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

The method I suggested would allow the
necessary information to be extracted during the initial search for
child tables, which we have to do anyway.

find_all_inheritors() returns a clean list which does not contain
duplicated OID of the inherited relation, so it seems to me we need
to change the function prototype but it affects other parts, or to add
a new function which also returns number of duplications, not only OIDs.

Or, we can call find_inheritance_children() in renameatt() as if
find_all_inheritors() doing except for list_concat_unique_oid().

The attached patch modified the condition to prevent renaming.

It computes an expected inhcount for each child relations on the initial
call of the renameatt() for the parent relation.
The find_all_inheritors_with_inhcount() returns OID of the inherited
relations and the expected inhcoundt. If a child relation has diamond
inheritance tree, it has its expected inhcount larger than 1.

This patch raises an error, if pg_attribute.inhcount is larger than
the expected inhcount. It can be happen when the attribute to be
renamed is merged from any other unrelated relations in the child
relations.

See the example:

postgres=# CREATE TABLE t1 (a int);
CREATE TABLE
postgres=# CREATE TABLE t2 (b int) inherits (t1);
CREATE TABLE
postgres=# CREATE TABLE t3 (c int) inherits (t1);
CREATE TABLE
postgres=# CREATE TABLE t4 (d int) inherits (t2, t3);
NOTICE: merging multiple inherited definitions of column "a"
CREATE TABLE

postgres=# ALTER TABLE t1 RENAME a TO x;
ALTER TABLE
postgres=# \d t4
Table "public.t4"
Column | Type | Modifiers
--------+---------+-----------
x | integer |
b | integer |
c | integer |
d | integer |
Inherits: t2,
t3

We can rename a of t1, t2, t3 and t4 correctly, although t4.a has inherited from
multiple relations.

postgres=# CREATE TABLE s1 (x int);
CREATE TABLE
postgres=# CREATE TABLE t5 (e int) inherits (t2, t3, s1);
NOTICE: merging multiple inherited definitions of column "x"
NOTICE: merging multiple inherited definitions of column "x"
CREATE TABLE
postgres=# ALTER TABLE t1 RENAME x TO y;
ERROR: cannot rename multiple inherited column "x"

But, the new t5 prevent to rename "x" to "y", because t5.x is also inherited from
the s1 and merged. So, its inhcount is 3 larger than expected inhcount (=2).

postgres=# SELECT attname, attinhcount FROM pg_attribute where attname='x' and attrelid='t5'::regclass;
attname | attinhcount
---------+-------------
x | 3
(1 row)

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

On Sun, Jan 3, 2010 at 11:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

2010/1/3 KaiGai Kohei <kaigai@ak.jp.nec.com>:

 if (number_of_attribute_origin(myrelid, oldattname) > 1)
     ereport(ERROR, ...);

Am I missing something?

That sounds about right to me,

It looks remarkably inefficient to me.  Do you propose to search the
entire database's inheritance tree to derive that number?  And do it
over again at each child table?  The method I suggested would allow the
necessary information to be extracted during the initial search for
child tables, which we have to do anyway.

I haven't read the code in enough detail to have an educated opinion
about whether that would induce enough overhead to be worth worrying
about it, so I will refrain from comment on this until I have done my
homework.

...Robert

The similar matter can be reproduced with ALTER TABLE ... TYPE statement,
not only RENAME TO option.

postgres=# CREATE TABLE t1 (a int);
CREATE TABLE
postgres=# CREATE TABLE s1 (a int);
CREATE TABLE

postgres=# CREATE TABLE ts (b int) inherits (t1, s1);
NOTICE: merging multiple inherited definitions of column "a"
CREATE TABLE
postgres=# ALTER TABLE t1 ALTER a TYPE text;
ALTER TABLE

postgres=# insert into t1 values ('aaa');
INSERT 0 1
postgres=# insert into ts values ('bbb', 2);
INSERT 0 1
postgres=# SELECT * FROM t1;
a
-----
aaa
bbb
(2 rows)

postgres=# SELECT * FROM s1;
ERROR: attribute "a" of relation "ts" does not match parent's type

In the renameatt(), we can count an expected inhcount of the column to be
renamed on find_all_inheritors() at the top-level recursion.
But it does not work well for the new one, because it is handled within
the common ATPrepCmd() scheme.

I reconsidered that we need a function to check whether the given column
is inherited from multiple root parents, or not, for each levels.
Then, it can be called from both of renameatt() and ATPrepAlterColumnType().

(2010/01/04 18:55), KaiGai Kohei wrote:

The method I suggested would allow the
necessary information to be extracted during the initial search for
child tables, which we have to do anyway.

find_all_inheritors() returns a clean list which does not contain
duplicated OID of the inherited relation, so it seems to me we need
to change the function prototype but it affects other parts, or to add
a new function which also returns number of duplications, not only OIDs.

Or, we can call find_inheritance_children() in renameatt() as if
find_all_inheritors() doing except for list_concat_unique_oid().

The attached patch modified the condition to prevent renaming.

It computes an expected inhcount for each child relations on the initial
call of the renameatt() for the parent relation.
The find_all_inheritors_with_inhcount() returns OID of the inherited
relations and the expected inhcoundt. If a child relation has diamond
inheritance tree, it has its expected inhcount larger than 1.

This patch raises an error, if pg_attribute.inhcount is larger than
the expected inhcount. It can be happen when the attribute to be
renamed is merged from any other unrelated relations in the child
relations.

See the example:

postgres=# CREATE TABLE t1 (a int);
CREATE TABLE
postgres=# CREATE TABLE t2 (b int) inherits (t1);
CREATE TABLE
postgres=# CREATE TABLE t3 (c int) inherits (t1);
CREATE TABLE
postgres=# CREATE TABLE t4 (d int) inherits (t2, t3);
NOTICE: merging multiple inherited definitions of column "a"
CREATE TABLE

postgres=# ALTER TABLE t1 RENAME a TO x;
ALTER TABLE
postgres=# \d t4
Table "public.t4"
Column | Type | Modifiers
--------+---------+-----------
x | integer |
b | integer |
c | integer |
d | integer |
Inherits: t2,
t3

We can rename a of t1, t2, t3 and t4 correctly, although t4.a has inherited from
multiple relations.

postgres=# CREATE TABLE s1 (x int);
CREATE TABLE
postgres=# CREATE TABLE t5 (e int) inherits (t2, t3, s1);
NOTICE: merging multiple inherited definitions of column "x"
NOTICE: merging multiple inherited definitions of column "x"
CREATE TABLE
postgres=# ALTER TABLE t1 RENAME x TO y;
ERROR: cannot rename multiple inherited column "x"

But, the new t5 prevent to rename "x" to "y", because t5.x is also inherited from
the s1 and merged. So, its inhcount is 3 larger than expected inhcount (=2).

postgres=# SELECT attname, attinhcount FROM pg_attribute where attname='x' and attrelid='t5'::regclass;
attname | attinhcount
---------+-------------
x | 3
(1 row)

Thanks,

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

The attached patch fixes bugs when we try to rename (and change type) on
a column inherited from the different origin and merged.

This patch adds:

List *find_column_origin(Oid relOid, const char *colName)

It returns the list of relation OIDs which originally defines the given
column. In most cases, it returns a list with an element. But, if the column
is inherited from multiple parent relations and merged during the inheritance
tree, the returned list contains multiple OIDs.
In this case, we have to forbid changing type and renaming to keep correctness
of the table definition.

Example)
postgres=# CREATE TABLE t1 (a int, b int);
CREATE TABLE
postgres=# CREATE TABLE s1 (b int, c int);
CREATE TABLE
postgres=# CREATE TABLE ts (d int) INHERITS (t1, s1);
NOTICE: merging multiple inherited definitions of column "b"
CREATE TABLE

postgres=# ALTER TABLE t1 ALTER a TYPE text;
ALTER TABLE
postgres=# ALTER TABLE t1 ALTER b TYPE text;
ERROR: cannot alter multiple inherited column "b" <---- (*)
(*) ts.b is also inherited from s1, not only t1, so forbid changing type

postgres=# ALTER TABLE s1 RENAME c TO cc;
ALTER TABLE
postgres=# ALTER TABLE s1 RENAME b TO bb;
ERROR: cannot rename multiple inherited column "b" <---- (*)
(*) ts.b is also inherited from s1, not only t1, so forbid renaming

postgres=# \d+ ts
Table "public.ts"
Column | Type | Modifiers | Storage | Description
--------+---------+-----------+----------+-------------
a | text | | extended |
b | integer | | plain |
cc | integer | | plain |
d | integer | | plain |
Inherits: t1,
s1
Has OIDs: no

In the case when a column is inherited from multiple relations but it
eventually has same origin (diamond inheritance tree case), we don't
need to forbid these operations.
In this case, find_column_origin() does not return duplicated OIDs,
all the caller has to do is to check whether length of the returned
list is larger than 1, or no.

Thanks,

(2010/01/14 12:43), KaiGai Kohei wrote:

The similar matter can be reproduced with ALTER TABLE ... TYPE statement,
not only RENAME TO option.

postgres=# CREATE TABLE t1 (a int);
CREATE TABLE
postgres=# CREATE TABLE s1 (a int);
CREATE TABLE

postgres=# CREATE TABLE ts (b int) inherits (t1, s1);
NOTICE: merging multiple inherited definitions of column "a"
CREATE TABLE
postgres=# ALTER TABLE t1 ALTER a TYPE text;
ALTER TABLE

postgres=# insert into t1 values ('aaa');
INSERT 0 1
postgres=# insert into ts values ('bbb', 2);
INSERT 0 1
postgres=# SELECT * FROM t1;
a
-----
aaa
bbb
(2 rows)

postgres=# SELECT * FROM s1;
ERROR: attribute "a" of relation "ts" does not match parent's type

In the renameatt(), we can count an expected inhcount of the column to be
renamed on find_all_inheritors() at the top-level recursion.
But it does not work well for the new one, because it is handled within
the common ATPrepCmd() scheme.

I reconsidered that we need a function to check whether the given column
is inherited from multiple root parents, or not, for each levels.
Then, it can be called from both of renameatt() and ATPrepAlterColumnType().

(2010/01/04 18:55), KaiGai Kohei wrote:

The method I suggested would allow the
necessary information to be extracted during the initial search for
child tables, which we have to do anyway.

find_all_inheritors() returns a clean list which does not contain
duplicated OID of the inherited relation, so it seems to me we need
to change the function prototype but it affects other parts, or to add
a new function which also returns number of duplications, not only OIDs.

Or, we can call find_inheritance_children() in renameatt() as if
find_all_inheritors() doing except for list_concat_unique_oid().

The attached patch modified the condition to prevent renaming.

It computes an expected inhcount for each child relations on the initial
call of the renameatt() for the parent relation.
The find_all_inheritors_with_inhcount() returns OID of the inherited
relations and the expected inhcoundt. If a child relation has diamond
inheritance tree, it has its expected inhcount larger than 1.

This patch raises an error, if pg_attribute.inhcount is larger than
the expected inhcount. It can be happen when the attribute to be
renamed is merged from any other unrelated relations in the child
relations.

See the example:

postgres=# CREATE TABLE t1 (a int);
CREATE TABLE
postgres=# CREATE TABLE t2 (b int) inherits (t1);
CREATE TABLE
postgres=# CREATE TABLE t3 (c int) inherits (t1);
CREATE TABLE
postgres=# CREATE TABLE t4 (d int) inherits (t2, t3);
NOTICE: merging multiple inherited definitions of column "a"
CREATE TABLE

postgres=# ALTER TABLE t1 RENAME a TO x;
ALTER TABLE
postgres=# \d t4
Table "public.t4"
Column | Type | Modifiers
--------+---------+-----------
x | integer |
b | integer |
c | integer |
d | integer |
Inherits: t2,
t3

We can rename a of t1, t2, t3 and t4 correctly, although t4.a has inherited from
multiple relations.

postgres=# CREATE TABLE s1 (x int);
CREATE TABLE
postgres=# CREATE TABLE t5 (e int) inherits (t2, t3, s1);
NOTICE: merging multiple inherited definitions of column "x"
NOTICE: merging multiple inherited definitions of column "x"
CREATE TABLE
postgres=# ALTER TABLE t1 RENAME x TO y;
ERROR: cannot rename multiple inherited column "x"

But, the new t5 prevent to rename "x" to "y", because t5.x is also inherited from
the s1 and merged. So, its inhcount is 3 larger than expected inhcount (=2).

postgres=# SELECT attname, attinhcount FROM pg_attribute where attname='x' and attrelid='t5'::regclass;
attname | attinhcount
---------+-------------
x | 3
(1 row)

Thanks,

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>