Thread: ALTER TABLE ... ADD COLUMN

Hello hackers,

I'm thinking about ALTER TABLE ... ADD COLUMN working properly when
child tables already contain the column.

I have two proposals.  First one:

There are two cases: one when specifying ALTER TABLE ONLY, and other
when specifying recursive (not ONLY).

In the first (ONLY) case, child tables are checked for existance of the
column.  If any child does not contain the column or it has a different
atttypid or attypmod (any other field that should be checked?), the
request is aborted.  Else the column is added in the parent only, and
the child attributes have their attinhcount incremented.

In the second case, child tables are checked for existance of the
column.  If the column doesn't exist, the procedure is called
recursively.  If the column exists but has a different atttypid o
atttypmod, the request is aborted.  Else, the attribute has its
attinhcount incremented and attislocal reset.


There are two differences between these two cases:
- ONLY does not add a column in childs.  This seems natural.  Obviously the scenario where this can lead to trouble
producesan error (inexistent attr in child).
 

- ONLY does not touch attislocal in childs.


The second may seems odd, but consider the following scenario:

CREATE TABLE p1 (f1 int);
CREATE TABLE p2 (f2 int);
CREATE TABLE c (f1 int) INHERITS (p1, p2);

In this case, c.f1.attislocal is true.  Now suppose the user wants to
create p2.f1.  If the recursive version is used, attislocal will be
reset and the scenario will be equivalent to

CREATE TABLE p1 (f1 int);
CREATE TABLE p2 (f1 int, f2 int);
CREATE TABLE c () INHERITS (p1, p2);

but the natural way would be

CREATE TABLE p1 (f1 int);
CREATE TABLE p2 (f1 int, f2 int);
CREATE TABLE c (f1 int) INHERITS (p1, p2);

which is what the ONLY case does.


Second proposal: Another way of doing this would be resetting attislocal
iff attinhcount is exactly zero.  This assumes that if attislocal is
true and attinhcount is not zero, the user wants the column as locally
defined and we should keep it that way.  On the other hand, if a child
table already has the column there's no way to push the definition up
the inheritance tree.

What do you think?  I have actually a patch ready that implements the
first proposal; the second one, while simpler conceptually and in terms
of code, occurred to me just as I was writing this, and has an issue of
completeness.

-- 
Alvaro Herrera (<alvherre[a]atentus.com>)
"Las cosas son buenas o malas segun las hace nuestra opinion" (Lisias)

Alvaro Herrera <alvherre@atentus.com> writes:
> I'm thinking about ALTER TABLE ... ADD COLUMN working properly when
> child tables already contain the column.
> There are two cases: one when specifying ALTER TABLE ONLY, and other
> when specifying recursive (not ONLY).

I think ALTER TABLE ONLY ... ADD COLUMN is nonsensical and should be
rejected out of hand.  That solves that part of the problem easily ;-)

The comparison case in my mind is ALTER TABLE ONLY ... RENAME COLUMN,
which has to be rejected.  (Surely you're not going to say we should
support this by allowing the parent column to become associated with
some other child columns than before...)  ALTER ONLY ADD COLUMN cannot
add any functionality that's not perfectly well available with
ALTER ADD COLUMN.


> In the second case, child tables are checked for existance of the
> column.  If the column doesn't exist, the procedure is called
> recursively.  If the column exists but has a different atttypid o
> atttypmod, the request is aborted.  Else, the attribute has its
> attinhcount incremented and attislocal reset.

I don't like resetting attislocal here.  If you do that, then DROPping
the parent column doesn't return you to the prior state.  I think I gave
this example before, but consider

CREATE TABLE p (f1 int);
CREATE TABLE c (f2 int) INHERITS (p);
ALTER TABLE p ADD COLUMN f2 int;
-- sometime later, realize that the ADD was a mistake, so:
ALTER TABLE p DROP COLUMN f2;

If you reset attislocal then the ending state will be that c.f2 is gone.
That seems clearly wrong to me.


> The second may seems odd, but consider the following scenario:

> CREATE TABLE p1 (f1 int);
> CREATE TABLE p2 (f2 int);
> CREATE TABLE c (f1 int) INHERITS (p1, p2);

> In this case, c.f1.attislocal is true.  Now suppose the user wants to
> create p2.f1.  If the recursive version is used, attislocal will be
> reset and the scenario will be equivalent to

> CREATE TABLE p1 (f1 int);
> CREATE TABLE p2 (f1 int, f2 int);
> CREATE TABLE c () INHERITS (p1, p2);

... which is wrong also.  c had a local definition before and should
still, IMHO.  What's the argument for taking away its local definition?
        regards, tom lane

On Fri, Oct 04, 2002 at 05:57:02PM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@atentus.com> writes:
> > I'm thinking about ALTER TABLE ... ADD COLUMN working properly when
> > child tables already contain the column.
> > There are two cases: one when specifying ALTER TABLE ONLY, and other
> > when specifying recursive (not ONLY).

> I don't like resetting attislocal here.  If you do that, then DROPping
> the parent column doesn't return you to the prior state.  I think I gave
> this example before, but consider

Huh, I don't know where I got the idea you were (or someone else was?)
in the position that attislocal should be reset.  I'll clean everything
up and submit the patch I had originally made.

-- 
Alvaro Herrera (<alvherre[a]atentus.com>)
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end." (2nd Commandment for C programmers)