Bug in MergeAttributesIntoExisting() function.

Started by amul sulabout 10 years ago6 messages

sul_amul@yahoo.co.in

about 10 years ago

1 attachment(s)

Hi,

In inheritance, child column's pg_attribute.attislocal flag not getting updated, if it is inherited using ALTER TABLE <child> INHERIT <parent>.

Due to this, if we try to drop column(s) from parent table, which are not getting drop from child.
Attached herewith is quick patch fixing this issue.

----------------------Demonstration:
----------------------
CREATE TABLE p1 (a int , b int, c int, d int);

CREATE TABLE c1 () inherits (p1);CREATE TABLE c2 (a int , b int, c int, d int);

--Drop parent's column
ALTER TABLE p1 DROP COLUMN b;
ALTER TABLE p1 DROP COLUMN c;
ALTER TABLE p1 DROP COLUMN d;

postgres=# \d p1
Table "public.p1"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
Number of child tables: 2 (Use \d+ to list them.)

postgres=# \d c1
Table "public.c1"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
Inherits: p1

----------------------
You can see columns are not dropped from child c2 table, which we have inherited using ALTER command.

Regards,
Amul Sul

Attachments:

MergeAttributesIntoExisting_fix.patchapplication/octet-streamDownload

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 0b4a334..4c135cb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10275,6 +10275,7 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
 			 * later on, this change will just roll back.)
 			 */
 			childatt->attinhcount++;
+			childatt->attislocal = false;
 			simple_heap_update(attrrel, &tuple->t_self, tuple);
 			CatalogUpdateIndexes(attrrel, tuple);
 			heap_freetuple(tuple);
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 89b6c1c..459f523 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1090,6 +1090,37 @@ Inherits: test_foreign_constraints
 DROP TABLE test_foreign_constraints_inh;
 DROP TABLE test_foreign_constraints;
 DROP TABLE test_primary_constraints;
+-- Test for DROP COLUMN
+CREATE TABLE p1 (a int , b int);
+CREATE TABLE c1 () inherits (p1);
+CREATE TABLE c2 (a int , b int);
+ALTER TABLE c2 INHERIT p1;
+ALTER TABLE p1 DROP COLUMN b;
+\d p1
+      Table "public.p1"
+ Column |  Type   | Modifiers 
+--------+---------+-----------
+ a      | integer | 
+Number of child tables: 2 (Use \d+ to list them.)
+
+\d c1
+      Table "public.c1"
+ Column |  Type   | Modifiers 
+--------+---------+-----------
+ a      | integer | 
+Inherits: p1
+
+\d c2
+      Table "public.c2"
+ Column |  Type   | Modifiers 
+--------+---------+-----------
+ a      | integer | 
+Inherits: p1
+
+DROP TABLE p1 CASCADE;
+NOTICE:  drop cascades to 2 other objects
+DETAIL:  drop cascades to table c1
+drop cascades to table c2
 --
 -- Test parameterized append plans for inheritance trees
 --
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 793c216..bc7ca55 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -334,6 +334,20 @@ DROP TABLE test_foreign_constraints_inh;
 DROP TABLE test_foreign_constraints;
 DROP TABLE test_primary_constraints;
 
+-- Test for DROP COLUMN
+CREATE TABLE p1 (a int , b int);
+CREATE TABLE c1 () inherits (p1);
+CREATE TABLE c2 (a int , b int);
+
+ALTER TABLE c2 INHERIT p1;
+ALTER TABLE p1 DROP COLUMN b;
+
+\d p1
+\d c1
+\d c2
+
+DROP TABLE p1 CASCADE;
+
 --
 -- Test parameterized append plans for inheritance trees
 --

Import Notes

Reference msg id not found: 1938741307.6256457.1451905888144.JavaMail.yahoo.ref@mail.yahoo.com

Rushabh Lathia

rushabh.lathia@gmail.com

about 10 years ago

In reply to: amul sul (#1)

Re: Bug in MergeAttributesIntoExisting() function.

On Mon, Jan 4, 2016 at 4:41 PM, amul sul <sul_amul@yahoo.co.in> wrote:

Hi,

In inheritance, child column's pg_attribute.attislocal flag not getting
updated, if it is inherited using ALTER TABLE <child> INHERIT <parent>.

Due to this, if we try to drop column(s) from parent table, which are not
getting drop from child.
Attached herewith is quick patch fixing this issue.

----------------------Demonstration:
----------------------
CREATE TABLE p1 (a int , b int, c int, d int);

CREATE TABLE c1 () inherits (p1);CREATE TABLE c2 (a int , b int, c int, d
int);

--Drop parent's column
ALTER TABLE p1 DROP COLUMN b;
ALTER TABLE p1 DROP COLUMN c;
ALTER TABLE p1 DROP COLUMN d;

postgres=# \d p1
Table "public.p1"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
Number of child tables: 2 (Use \d+ to list them.)

postgres=# \d c1
Table "public.c1"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
Inherits: p1

postgres=# \d c2
Table "public.c2"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
b | integer |
c | integer |
d | integer |
Inherits: p1

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

Seems like you missed following command in the demonstration test:

ALTER TABLE c2 INHERIT p1;

You can see columns are not dropped from child c2 table, which we have
inherited using ALTER command.

I took a quick look at this and did some testing. Patch looks good to me.
ALTER TABLE INHERIT missing the attislocal = false check.

Regards,
Amul Sul

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
Rushabh Lathia
www.EnterpriseDB.com

Amit Langote

amitlangote09@gmail.com

about 10 years ago

In reply to: amul sul (#1)

Re: Bug in MergeAttributesIntoExisting() function.

Hi,

On Mon, Jan 4, 2016 at 8:11 PM, amul sul <sul_amul@yahoo.co.in> wrote:

Hi,

In inheritance, child column's pg_attribute.attislocal flag not getting updated, if it is inherited using ALTER TABLE <child> INHERIT <parent>.

Due to this, if we try to drop column(s) from parent table, which are not getting drop from child.
Attached herewith is quick patch fixing this issue.

----------------------Demonstration:
----------------------
CREATE TABLE p1 (a int , b int, c int, d int);

CREATE TABLE c1 () inherits (p1);CREATE TABLE c2 (a int , b int, c int, d int);

--Drop parent's column
ALTER TABLE p1 DROP COLUMN b;
ALTER TABLE p1 DROP COLUMN c;
ALTER TABLE p1 DROP COLUMN d;

postgres=# \d p1
Table "public.p1"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
Number of child tables: 2 (Use \d+ to list them.)

postgres=# \d c1
Table "public.c1"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
Inherits: p1

postgres=# \d c2
Table "public.c2"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
b | integer |
c | integer |
d | integer |
Inherits: p1

----------------------
You can see columns are not dropped from child c2 table, which we have inherited using ALTER command.

I'm afraid the patched behavior of MergeAttributeIntoExisting() would
be inconsistent with MergeAttributes(). For example, try with the
following:

CREATE TABLE c1(b int) INHERITS(p1);

In this case, MergeAttributes() would cause 'b' to be defined to be a
local attribute (ie, with attislocal = true) and hence would not be
dropped unlike c2 which the patched behavior would cause to be
dropped.

Am I missing something?

Thanks,
Amit

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

about 10 years ago

In reply to: Rushabh Lathia (#2)

Re: Bug in MergeAttributesIntoExisting() function.

On Mon, Jan 4, 2016 at 9:28 AM, Rushabh Lathia <rushabh.lathia@gmail.com> wrote:

On Mon, Jan 4, 2016 at 4:41 PM, amul sul <sul_amul@yahoo.co.in> wrote:

Hi,
In inheritance, child column's pg_attribute.attislocal flag not getting
updated, if it is inherited using ALTER TABLE <child> INHERIT <parent>.

Due to this, if we try to drop column(s) from parent table, which are not
getting drop from child.
Attached herewith is quick patch fixing this issue.

Seems like you missed following command in the demonstration test:

ALTER TABLE c2 INHERIT p1;

You can see columns are not dropped from child c2 table, which we have
inherited using ALTER command.

I took a quick look at this and did some testing. Patch looks good to me.
ALTER TABLE INHERIT missing the attislocal = false check.

This is not a bug. ALTER TABLE .. INHERIT isn't supposed to set
attislocal = false. attislocal tracks whether there was a local copy
of the attribute definition originally - here, the answer is yes, even
after the ALTER TABLE .. INHERIT.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

about 10 years ago

In reply to: amul sul (#1)

Re: Bug in MergeAttributesIntoExisting() function.

amul sul <sul_amul@yahoo.co.in> writes:

In inheritance, child column's pg_attribute.attislocal flag not getting updated, if it is inherited using ALTER TABLE <child> INHERIT <parent>.

I think this patch is wrong and you have broken the intended behavior.
It's a bit hard to tell though because your example is confused.

CREATE TABLE p1 (a int , b int, c int, d int);

CREATE TABLE c1 () inherits (p1);CREATE TABLE c2 (a int , b int, c int, d int);

--Drop parent's column
ALTER TABLE p1 DROP COLUMN b;
ALTER TABLE p1 DROP COLUMN c;
ALTER TABLE p1 DROP COLUMN d;

postgres=# \d p1
Table "public.p1"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
Number of child tables: 2 (Use \d+ to list them.)

Say what? At this point only c1 is a child of p1. I assume you've
left something out, either an ALTER INHERIT or an INHERITS clause
in CREATE TABLE c2.

Either way, however, the way you declared c2, it has an independent
local definition of all four columns, and so they should not go away
even if the parent's columns go away. This is exactly the purpose
that attislocal was created to serve, and your patch destroys it.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

amul sul

sul_amul@yahoo.co.in

about 10 years ago

In reply to: Tom Lane (#5)

Re: Bug in MergeAttributesIntoExisting() function.

On Monday, 4 January 2016 8:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Either way, however, the way you declared c2, it has an independent
local definition of all four columns, and so they should not go away
even if the parent's columns go away. This is exactly the purpose
that attislocal was created to serve, and your patch destroys it.

Understood this is not a bug, thank you.

I missed ALTER TABLE statement in previous test case, correct test case is as follow:
-- create table
CREATE TABLE p1 (a int , b int);
CREATE TABLE c2 (a int , b int);

-- alter c2' inheritance
ALTER TABLE c2 INHERIT p1;

-- drop column b
ALTER TABLE p1 DROP COLUMN b;

-- table description

postgres=# \d p1
Table "public.p1"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
Number of child tables: 1 (Use \d+ to list them.)

Regards,
Amul Sul

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers