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

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(at)ak(dot)jp(dot)nec(dot)com>