DROP COLUMN misbehaviour with multiple inheritance

Hannu Krosing <hannu@tm.ee> writes:

I've come upon a misbehaviour of drop column, where drop column
unconditionally drops inherited column from child tables.
What it should do is to check if the same column is not inherited from
other parents and drop it only when it is not

Hm. Seems like attisinherited should have been a count, not a boolean.

Is anyone sufficiently excited about this issue to force an initdb to
fix it?

regards, tom lane

Tom Lane dijo:

Hannu Krosing <hannu@tm.ee> writes:

I've come upon a misbehaviour of drop column, where drop column
unconditionally drops inherited column from child tables.
What it should do is to check if the same column is not inherited from
other parents and drop it only when it is not

Hm. Seems like attisinherited should have been a count, not a boolean.

I'll try to make a fix and submit.

Is anyone sufficiently excited about this issue to force an initdb to
fix it?

If people thinks it's important, the fix can be integrated. If not, it
can wait until 7.4.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"Aprende a avergonzarte mas ante ti que ante los demas" (Democrito)

Hannu Krosing <hannu@tm.ee> writes:

Hm. Seems like attisinherited should have been a count, not a boolean.
Is anyone sufficiently excited about this issue to force an initdb to
fix it?

The count approach seems definitely the right way, but a check (possibly
a slow one) can be probably done without initdb.

Slow, complicated to code, and deadlock-prone (since you'd have to
acquire locks on the other parent tables). My feeling is we fix this
with a counted attisinherited field, or don't fix at all.

We can certainly do the proper fix in 7.4; do we consider this bug
important enough to do an initdb for 7.3beta2? I don't have a strong
feeling either way about that.

regards, tom lane

On Thu, 2002-09-12 at 16:14, Tom Lane wrote:

Hannu Krosing <hannu@tm.ee> writes:

I've come upon a misbehaviour of drop column, where drop column
unconditionally drops inherited column from child tables.
What it should do is to check if the same column is not inherited from
other parents and drop it only when it is not

Hm. Seems like attisinherited should have been a count, not a boolean.

either that, or some check at drop column time.

Is anyone sufficiently excited about this issue to force an initdb to
fix it?

The count approach seems definitely the right way, but a check (possibly
a slow one) can be probably done without initdb.

The other sad thing about the current behaviour is that in addition to
being wrong it also breaks dump/reload - after dump/reload the initially
dropped column is back in c1.

-------------
Hannu

The count approach seems definitely the right way, but a check (possibly
a slow one) can be probably done without initdb.

We can certainly do the proper fix in 7.4; do we consider this bug
important enough to do an initdb for 7.3beta2? I don't have a strong
feeling either way about that.

I think we are too scared of doing initdb during beta...

Initdb during beta should not be evaultated on a per bug basis, but keep a
list of all things that could be fixed and judge if the total of all the
fixes is worth one initdb. Right now off the top of my head I can think of
the split function and this inherited change, are there more?

my two cents...

On Thu, 12 Sep 2002, Matthew T. OConnor wrote:

The count approach seems definitely the right way, but a check (possibly
a slow one) can be probably done without initdb.

We can certainly do the proper fix in 7.4; do we consider this bug
important enough to do an initdb for 7.3beta2? I don't have a strong
feeling either way about that.

I think we are too scared of doing initdb during beta...

Initdb during beta should not be evaultated on a per bug basis, but keep a
list of all things that could be fixed and judge if the total of all the
fixes is worth one initdb. Right now off the top of my head I can think of
the split function and this inherited change, are there more?

my two cents...

Agreed.

Actually, an argument could likely be made that changes that require
initdb should be done as early as possible since the later the change the
more people there will be to test the change, and there will be fewer
people who actually have to initdb since a lot of folks don't test beta
releases until the 3rd or 4th beta.

On Thu, 12 Sep 2002, scott.marlowe wrote:

Agreed.

Actually, an argument could likely be made that changes that require
initdb should be done as early as possible since the later the change the
more people there will be to test the change, and there will be fewer
people who actually have to initdb since a lot of folks don't test beta
releases until the 3rd or 4th beta.

My mental dyslexia strikes again, that should be:

... since the EARLIER the change the more people there will be to test the
change, ...

sheesh. Sorry...

Tom Lane dijo:

Hannu Krosing <hannu@tm.ee> writes:

The count approach seems definitely the right way, but a check (possibly
a slow one) can be probably done without initdb.

Slow, complicated to code, and deadlock-prone (since you'd have to
acquire locks on the other parent tables). My feeling is we fix this
with a counted attisinherited field, or don't fix at all.

All right, I now have all the catalog changes on place; this is the easy
part (is an int2 count enough?).

But when actually dropping a column, the recursion cannot be done the
way it's done now, fetching the whole inheritor tree in one pass,
because there's no way to distinguish the direct ones that have the
attisinherited count greater than 1 from deeper ones; it has to be done
step by step. If this is not clear, imagine the following situation:

create table p1(id int, name text);
create table p2(id2 int, name text);
create table c1(age int) inherits(p1,p2);
create table gc1() inherits (c1);

p1 and p2 have name->attisinherited=0, while c1 has
name->attisinherited=2. But gc1->name->attisinherited=1. If I just
recurse the tree the way it's done now, I will happily drop "name" from
gc1 while keeping it on c1. So I have to switch from
find_all_inheritors() to find_inheritance_children() and keep recursing
until there are no more inheritors (I still have to check if there are
other gotchas with this approach, or optimizations to be done). I am
already midway with this, but wanted to let you know in case the patch
is rejected.

Is this Ok? I see this is getting away from the "trivial fix" camp.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)

Alvaro Herrera dijo:

All right, I now have all the catalog changes on place; this is the easy
part (is an int2 count enough?).

But when actually dropping a column, the recursion cannot be done the
way it's done now, fetching the whole inheritor tree in one pass,
because there's no way to distinguish the direct ones that have the
attisinherited count greater than 1 from deeper ones; it has to be done
step by step.

Done. I attach the patch. It's huge because it needs to touch
pg_attribute.h, but it is relatively simple. This passes the regression
tests and fixes the bug reported by Hannu.

Please review and apply if OK. I didn't touch catversion.h.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"Cuando ma�ana llegue pelearemos segun lo que ma�ana exija" (Mowgli)

En 12 Sep 2002 17:23:41 +0200
Hannu Krosing <hannu@tm.ee> escribi�:

The other sad thing about the current behaviour is that in addition to
being wrong it also breaks dump/reload - after dump/reload the initially
dropped column is back in c1.

I hadn't read this paragraph before. But I don't understand what
you're saying. If I drop the column from p1 but not from p2, how is it
expected that the column doesn't show in c1, that inherits both? Truth
is that the column shouldn't have disappeared in the first place, so it
isn't a mistake that shows up in the dump.

Sure, databases before and after the dump are different, but the one
before dump is broken. I don't have the original pgsql version (without
the patch) compiled right now, but I think that if you were to select
from p2, the backend would crash (or at least elog(ERROR)).

Anyway, the patch I just submitted should fix this bug. Please test it
and thanks for the report.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"La conclusion que podemos sacar de esos estudios es que
no podemos sacar ninguna conclusion de ellos" (Tanenbaum)

Alvaro Herrera <alvherre@atentus.com> writes:

If this is not clear, imagine the following situation:

create table p1(id int, name text);
create table p2(id2 int, name text);
create table c1(age int) inherits(p1,p2);
create table gc1() inherits (c1);

p1 and p2 have name->attisinherited=0, while c1 has
name->attisinherited=2. But gc1->name->attisinherited=1.

Ick. I hadn't thought that far ahead.

We could probably cause gc1->name->attisinherited to be 2 in this
scenario; does that help?

Actually, there might not be a problem. c1.name can't be deleted until
both p1.name and p2.name go away, and at that point we want both c1.name
and gc1.name to go away. So as long as we don't *recursively* decrement
the inherits count when c1.name.attisinherited hasn't reached 0, this
might be okay. But it needs thought.

I see this is getting away from the "trivial fix" camp.

Yup. Let's step back and think carefully before we plunge into the
coding. What goes away when, and how do we define the inherits-count
to make it work right?

regards, tom lane

En Thu, 12 Sep 2002 23:40:21 -0400
Tom Lane <tgl@sss.pgh.pa.us> escribi�:

Alvaro Herrera <alvherre@atentus.com> writes:

If this is not clear, imagine the following situation:

create table p1(id int, name text);
create table p2(id2 int, name text);
create table c1(age int) inherits(p1,p2);
create table gc1() inherits (c1);

p1 and p2 have name->attisinherited=0, while c1 has
name->attisinherited=2. But gc1->name->attisinherited=1.

We could probably cause gc1->name->attisinherited to be 2 in this
scenario; does that help?

I'm trying to imagine a case where this is harmful, but cannot find any.
It would have to be proven that there is none; IMHO this is a little
deviating from the "reality".

Actually, there might not be a problem. c1.name can't be deleted until
both p1.name and p2.name go away, and at that point we want both c1.name
and gc1.name to go away. So as long as we don't *recursively* decrement
the inherits count when c1.name.attisinherited hasn't reached 0, this
might be okay. But it needs thought.

This is what I implemented on the patch I posted, I think. The idea is
that attisinherited is decremented non-recursively, i.e. only in direct
inheritors; and when it reaches zero the column is dropped, and its
inheritors have it decremented also.

In the cases I've tried this works, and it seems to me that it is
correct; however, I haven't proven it is. Multiple inheritance and
multiple generations is weird.

It just ocurred to me that maybe I overlooked the
ALTER TABLE ONLY ... DROP COLUMN case, but I'm now going to bed. I'll
think about this case tomorrow.

I see this is getting away from the "trivial fix" camp.

Yup. Let's step back and think carefully before we plunge into the
coding. What goes away when, and how do we define the inherits-count
to make it work right?

Huh, I already did. Please think about my solution.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"Para tener mas hay que desear menos"

Alvaro Herrera <alvherre@atentus.com> writes:

Tom Lane <tgl@sss.pgh.pa.us> escribi�:

Actually, there might not be a problem. c1.name can't be deleted until
both p1.name and p2.name go away, and at that point we want both c1.name
and gc1.name to go away. So as long as we don't *recursively* decrement
the inherits count when c1.name.attisinherited hasn't reached 0, this
might be okay. But it needs thought.

This is what I implemented on the patch I posted, I think. The idea is
that attisinherited is decremented non-recursively, i.e. only in direct
inheritors; and when it reaches zero the column is dropped, and its
inheritors have it decremented also.

Yeah; after marginally more thought, I'm thinking that the correct
definition of attisinherited (need new name BTW) is "number of *direct*
ancestors this table inherits this column from". I think you are
describing the same idea.

Given the obvious algorithms for updating and using such a value,
does anyone see a flaw in the behavior?

One corner case is that I think we currently allow

create table p (f1 int);
create table c (f1 int) inherits(p);

which is useless in the given example but is not useless if c
provides a default or constraints for column f1. ISTM f1 should
not go away in c if we drop it in p, in this case. Maybe we want
not an "inherits count" but a "total sources of definitions count",
which would include 1 for each ancestral table plus 1 if declared
locally. When it drops to 0, okay to delete the column.

however, I haven't proven it is. Multiple inheritance and
multiple generations is weird.

What he said... I'm way too tired to think this through tonight...

regards, tom lane

Tom Lane dijo:

One corner case is that I think we currently allow

create table p (f1 int);
create table c (f1 int) inherits(p);

In this case, c.f1.attisinherited count is 2; thus when I drop f1 from
p, it is not dropped from c.

Do you have some suggestion on what the name should be? Clearly
attisinherited is not appropiate. attinhcount maybe?

The patch submitted does what you describe. I'm leaving tomorrow and
won't be back until next weekend, so please do the name change yourself
if the patch is to be applied.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
Voy a acabar con todos los humanos / con los humanos yo acabar�
voy a acabar con todos / con todos los humanos acabar� (Bender)

I am keeing this patch so we have it to apply when we decide to force an
initdb:

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

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

Alvaro Herrera wrote:

Alvaro Herrera dijo:

All right, I now have all the catalog changes on place; this is the easy
part (is an int2 count enough?).

But when actually dropping a column, the recursion cannot be done the
way it's done now, fetching the whole inheritor tree in one pass,
because there's no way to distinguish the direct ones that have the
attisinherited count greater than 1 from deeper ones; it has to be done
step by step.

Done. I attach the patch. It's huge because it needs to touch
pg_attribute.h, but it is relatively simple. This passes the regression
tests and fixes the bug reported by Hannu.

Please review and apply if OK. I didn't touch catversion.h.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"Cuando ma?ana llegue pelearemos segun lo que ma?ana exija" (Mowgli)

Content-Description:

[ 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

[ back to thinking about this patch ]

Alvaro Herrera <alvherre@atentus.com> writes:

Tom Lane dijo:

One corner case is that I think we currently allow

create table p (f1 int);
create table c (f1 int) inherits(p);

In this case, c.f1.attisinherited count is 2; thus when I drop f1 from
p, it is not dropped from c.

That seems right, but the problem I have with it is that the resulting
state of c.f1 is attisinherited = 1. This means that you cannot drop
c.f1. It seems to me that we should have this behavior:

create table p (f1 int);
create table c (f1 int not null) inherits(p);

drop column c.f1;
-- should be rejected since c.f1 is inherited
drop column p.f1;
-- c.f1 is still there, but no longer inherited
drop column c.f1;
-- should succeed; but will fail with patch as given

as compared to

create table p (f1 int);
create table c () inherits(p);

drop column c.f1;
-- should be rejected since c.f1 is inherited
drop column p.f1;
-- c.f1 is dropped now, since there is no local definition for it

And if you aren't confused yet, what about non-recursive drops of p.f1
(ie, alter table ONLY p drop column f1)? This case seems clear:

create table p (f1 int);
create table c () inherits(p);

drop column c.f1;
-- should be rejected since c.f1 is inherited
drop ONLY column p.f1;
-- c.f1 is NOT dropped, but must now be considered non-inherited
drop column c.f1;
-- should succeed

And then I think we should say

create table p (f1 int);
create table c (f1 int not null) inherits(p);

drop column c.f1;
-- should be rejected since c.f1 is inherited
drop ONLY column p.f1;
-- c.f1 is still there, but no longer inherited
drop column c.f1;
-- should succeed

I am not sure how to make all four of these cases work. We might need
two fields :-( ... a "locally defined" boolean and a "number of times
inherited" counter. This seems like overkill though.

If we don't have the "locally defined" boolean then I think we have to
make the first case work like so:

create table p (f1 int);
create table c (f1 int not null) inherits(p);

drop column p.f1;
-- c.f1 GOES AWAY, because its inherit count went to zero

Is this reasonable behavior? I'm not sure. You could probably argue
it either way.

Another interesting case is multiple inheritance.

create table p1 (f1 int);
create table p2 (f1 int);
create table c () inherits(p1, p2);

drop ONLY column p1.f1;
drop column p2.f1;

After this sequence, what is the state of c.f1? Is it still there?
Should it be? If it is still there, will it be possible to get rid of
it with "drop column c.f1"? What if we did DROP ONLY on *both*
ancestors?

regards, tom lane

That seems right, but the problem I have with it is that the resulting
state of c.f1 is attisinherited = 1. This means that you cannot drop
c.f1. It seems to me that we should have this behavior:

Has anyone given much thought as to perhaps we could just drop multiple
inheritance from Postgres? There are people using single inheritance - but
how many actually use multiple inheritance? If we dumped it we could use
the proposed all-child-tables-in-one-relation idea, and everything would
become very easy...

Chris

Christopher Kings-Lynne wrote:

That seems right, but the problem I have with it is that the resulting
state of c.f1 is attisinherited = 1. This means that you cannot drop
c.f1. It seems to me that we should have this behavior:

Has anyone given much thought as to perhaps we could just drop multiple
inheritance from Postgres? There are people using single inheritance - but
how many actually use multiple inheritance? If we dumped it we could use
the proposed all-child-tables-in-one-relation idea, and everything would
become very easy...

I am for it. Multiple inheritance is more of a mess than a help. Just
look at C++. Everyone is moving away from multiple inheritance for that
reason.

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Christopher Kings-Lynne wrote:

Has anyone given much thought as to perhaps we could just drop multiple
inheritance from Postgres?

I am for it. Multiple inheritance is more of a mess than a help.

I'm not agin it ... but if that's the lay of the land then we have
no need to apply a last-minute catalog reformatting to fix a
multiple-inheritance bug. This patch is off the "must fix for 7.3"
list, no?

regards, tom lane