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 produces an 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)

Fri, 4 Oct 2002 17:21:49 -0400
Alvaro Herrera <alvherre@atentus.com>
ALTER TABLE ... ADD COLUMN
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

Fri, 04 Oct 2002 17:57:02 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: ALTER TABLE ... ADD COLUMN 
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)

Fri, 4 Oct 2002 18:21:06 -0400
Alvaro Herrera <alvherre@atentus.com>
Re: ALTER TABLE ... ADD COLUMN
En Fri, 4 Oct 2002 18:21:06 -0400
Alvaro Herrera <alvherre@atentus.com> escribiï¿½:

> 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.

All right, this is it.  This patch merely checks if child tables have
the column.  If atttypid and atttypmod are the same, the attributes'
attinhcount is incremented; else the operation is aborted.  If child
tables don't have the column, recursively add it.

attislocal is not touched in any case.

-- 
Alvaro Herrera (<alvherre[a]atentus.com>)
"El Maquinismo fue proscrito so pena de cosquilleo hasta la muerte"
(Ijon Tichy en Viajes, Stanislaw Lem)
Sat, 5 Oct 2002 04:46:23 -0400
Alvaro Herrera <alvherre@atentus.com>
Re: [HACKERS] ALTER TABLE ... ADD COLUMN

Your patch has been added to the PostgreSQL unapplied patches list at:

	http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------


Alvaro Herrera wrote:
> En Fri, 4 Oct 2002 18:21:06 -0400
> Alvaro Herrera <alvherre@atentus.com> escribi?:
> 
> > 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.
> 
> All right, this is it.  This patch merely checks if child tables have
> the column.  If atttypid and atttypmod are the same, the attributes'
> attinhcount is incremented; else the operation is aborted.  If child
> tables don't have the column, recursively add it.
> 
> attislocal is not touched in any case.
> 
> -- 
> Alvaro Herrera (<alvherre[a]atentus.com>)
> "El Maquinismo fue proscrito so pena de cosquilleo hasta la muerte"
> (Ijon Tichy en Viajes, Stanislaw Lem)

[ Attachment, skipping... ]

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
> 
> http://archives.postgresql.org

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Mon, 14 Oct 2002 00:34:52 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] ALTER TABLE ... ADD COLUMN

Patch applied.  Thanks.

---------------------------------------------------------------------------


Alvaro Herrera wrote:
> En Fri, 4 Oct 2002 18:21:06 -0400
> Alvaro Herrera <alvherre@atentus.com> escribi?:
> 
> > 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.
> 
> All right, this is it.  This patch merely checks if child tables have
> the column.  If atttypid and atttypmod are the same, the attributes'
> attinhcount is incremented; else the operation is aborted.  If child
> tables don't have the column, recursively add it.
> 
> attislocal is not touched in any case.
> 
> -- 
> Alvaro Herrera (<alvherre[a]atentus.com>)
> "El Maquinismo fue proscrito so pena de cosquilleo hasta la muerte"
> (Ijon Tichy en Viajes, Stanislaw Lem)

[ Attachment, skipping... ]

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
> 
> http://archives.postgresql.org

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Fri, 18 Oct 2002 22:09:35 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] ALTER TABLE ... ADD COLUMN