Attached partition not considering altered column properties of root partition.

Started by Prabhat Sahuover 6 years ago7 messages

prabhat.sahu@enterprisedb.com

over 6 years ago

Hi,

In below testcase when I changed the staorage option for root partition,
newly attached partition not including the changed staorage option.
Is this an expected behavior?

postgres=# CREATE TABLE tab1 (c1 INT, c2 text) PARTITION BY RANGE(c1);
CREATE TABLE
postgres=# create table tt_p1 as select * from tab1 where 1=2;
SELECT 0
postgres=# alter table tab1 alter COLUMN c2 set storage main;
ALTER TABLE
postgres=#
postgres=# alter table tab1 attach partition tt_p1 for values from (20) to
(30);
ALTER TABLE
postgres=# \d+ tab1
Partitioned table "public.tab1"
Column | Type | Collation | Nullable | Default | Storage | Stats target
| Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
c1 | integer | | | | plain |
|
c2 | text | | | | main |
|
Partition key: RANGE (c1)
Partitions: tt_p1 FOR VALUES FROM (20) TO (30)

With Regards,

Prabhat Kumar Sahu

Amit Langote

amitlangote09@gmail.com

over 6 years ago

In reply to: Prabhat Sahu (#1)

1 attachment(s)

Re: Attached partition not considering altered column properties of root partition.

Hi Prabhat,

On Tue, Jul 2, 2019 at 5:12 PM Prabhat Sahu
<prabhat.sahu@enterprisedb.com> wrote:

Hi,

In below testcase when I changed the staorage option for root partition, newly attached partition not including the changed staorage option.
Is this an expected behavior?

Thanks for the report. This seems like a bug. Documentation claims
that the child tables inherit column storage options from the parent
table. That's actually enforced in only some cases.

1. If you create the child table as a child to begin with (that is,
not attach it as child after the fact):

create table parent (a text);
create table child () inherits (parent);
select attrelid::regclass, attname, attstorage from pg_attribute where
attrelid in ('parent'::regclass, 'child'::regclass) and attname = 'a';
attrelid │ attname │ attstorage
──────────┼─────────┼────────────
parent │ a │ x
child │ a │ x
(2 rows)

2. If you change the parent's column's storage option, child's column
is recursively changed.

alter table parent alter a set storage main;
select attrelid::regclass, attname, attstorage from pg_attribute where
attrelid in ('parent'::regclass, 'child'::regclass) and attname = 'a';
attrelid │ attname │ attstorage
──────────┼─────────┼────────────
parent │ a │ m
child │ a │ m
(2 rows)

However, we fail to enforce the rule when the child is attached after the fact:

create table child2 (a text);
alter table child2 inherit parent;
select attrelid::regclass, attname, attstorage from pg_attribute where
attrelid in ('parent'::regclass, 'child'::regclass,
'child2'::regclass) and attname = 'a';
attrelid │ attname │ attstorage
──────────┼─────────┼────────────
parent │ a │ m
child │ a │ m
child2 │ a │ x
(3 rows)

To fix this, MergeAttributesIntoExisting() should check that the
attribute options of a child don't conflict with the parent, which the
attached patch implements. Note that partitioning uses the same code
as inheritance, so the fix applies to it too. After the patch:

create table p (a int, b text) partition by list (a);
create table p1 partition of p for values in (1);
select attrelid::regclass, attname, attstorage from pg_attribute where
attrelid in ('p'::regclass, 'p1'::regclass) and attname = 'b';
attrelid │ attname │ attstorage
──────────┼─────────┼────────────
p │ b │ x
p1 │ b │ x
(2 rows)

alter table p alter b set storage main;
select attrelid::regclass, attname, attstorage from pg_attribute where
attrelid in ('p'::regclass, 'p1'::regclass) and attname = 'b';
attrelid │ attname │ attstorage
──────────┼─────────┼────────────
p │ b │ m
p1 │ b │ m
(2 rows)

create table p2 (like p);
select attrelid::regclass, attname, attstorage from pg_attribute where
attrelid in ('p'::regclass, 'p1'::regclass, 'p2'::regclass) and
attname = 'b';
attrelid │ attname │ attstorage
──────────┼─────────┼────────────
p │ b │ m
p1 │ b │ m
p2 │ b │ x
(3 rows)

alter table p attach partition p2 for values in (2);
ERROR: child table "p2" has different storage option for column "b" than parent
DETAIL: EXTENDED versus MAIN

-- ok after changing p2 to match
alter table p2 alter b set storage main;
alter table p attach partition p2 for values in (2);

Thanks,
Amit

Attachments:

attstorage-inherit-bug.patchapplication/octet-stream; name=attstorage-inherit-bug.patchDownload

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3aee2d82ce..fa947e4e01 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13023,6 +13023,17 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
 						 errmsg("column \"%s\" in child table must be marked NOT NULL",
 								attributeName)));
 
+			/* No conflicting storage options allowed either. */
+			if (attribute->attstorage != childatt->attstorage)
+				ereport(ERROR,
+						(errcode(ERRCODE_DATATYPE_MISMATCH),
+						 errmsg("child table \"%s\" has different storage option for column \"%s\" than parent",
+								RelationGetRelationName(child_rel),
+								attributeName),
+						 errdetail("%s versus %s",
+								   storage_name(childatt->attstorage),
+								   storage_name(attribute->attstorage))));
+
 			/*
 			 * OK, bump the child column's inheritance count.  (If we fail
 			 * later on, this change will just roll back.)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index d621f61c62..6b3eb3c17d 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4073,3 +4073,15 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
 drop table at_test_sql_partop;
 drop operator class at_test_sql_partop using btree;
 drop function at_test_sql_partop;
+-- check the inheritance of attribute storage option
+create table attstorage_inh_test (a text);
+alter table attstorage_inh_test alter a set storage main;
+create table attstorage_inh_test1 (a text);
+-- error
+alter table attstorage_inh_test1 inherit attstorage_inh_test;
+ERROR:  child table "attstorage_inh_test1" has different storage option for column "a" than parent
+DETAIL:  EXTENDED versus MAIN
+alter table attstorage_inh_test1 alter a set storage main;
+-- ok
+alter table attstorage_inh_test1 inherit attstorage_inh_test;
+drop table attstorage_inh_test, attstorage_inh_test1;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 8016f8a823..9155731b42 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2712,3 +2712,14 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
 drop table at_test_sql_partop;
 drop operator class at_test_sql_partop using btree;
 drop function at_test_sql_partop;
+
+-- check the inheritance of attribute storage option
+create table attstorage_inh_test (a text);
+alter table attstorage_inh_test alter a set storage main;
+create table attstorage_inh_test1 (a text);
+-- error
+alter table attstorage_inh_test1 inherit attstorage_inh_test;
+alter table attstorage_inh_test1 alter a set storage main;
+-- ok
+alter table attstorage_inh_test1 inherit attstorage_inh_test;
+drop table attstorage_inh_test, attstorage_inh_test1;

Prabhat Sahu

prabhat.sahu@enterprisedb.com

over 6 years ago

In reply to: Amit Langote (#2)

Re: Attached partition not considering altered column properties of root partition.

Thanks Amit for the fix patch,

I have applied the patch and verified the issue.
The attached partition with altered column properties shows error as below:
postgres=# alter table p attach partition p2 for values in (2);
psql: ERROR: child table "p2" has different storage option for column "b"
than parent
DETAIL: EXTENDED versus MAIN

Thanks,
Prabhat Sahu

On Wed, Jul 3, 2019 at 7:23 AM Amit Langote <amitlangote09@gmail.com> wrote:

Show quoted text

Hi Prabhat,

On Tue, Jul 2, 2019 at 5:12 PM Prabhat Sahu
<prabhat.sahu@enterprisedb.com> wrote:

Hi,

In below testcase when I changed the staorage option for root partition,

newly attached partition not including the changed staorage option.

Is this an expected behavior?

Thanks for the report. This seems like a bug. Documentation claims
that the child tables inherit column storage options from the parent
table. That's actually enforced in only some cases.

1. If you create the child table as a child to begin with (that is,
not attach it as child after the fact):

create table parent (a text);
create table child () inherits (parent);
select attrelid::regclass, attname, attstorage from pg_attribute where
attrelid in ('parent'::regclass, 'child'::regclass) and attname = 'a';
attrelid │ attname │ attstorage
──────────┼─────────┼────────────
parent │ a │ x
child │ a │ x
(2 rows)

2. If you change the parent's column's storage option, child's column
is recursively changed.

alter table parent alter a set storage main;
select attrelid::regclass, attname, attstorage from pg_attribute where
attrelid in ('parent'::regclass, 'child'::regclass) and attname = 'a';
attrelid │ attname │ attstorage
──────────┼─────────┼────────────
parent │ a │ m
child │ a │ m
(2 rows)

However, we fail to enforce the rule when the child is attached after the
fact:

create table child2 (a text);
alter table child2 inherit parent;
select attrelid::regclass, attname, attstorage from pg_attribute where
attrelid in ('parent'::regclass, 'child'::regclass,
'child2'::regclass) and attname = 'a';
attrelid │ attname │ attstorage
──────────┼─────────┼────────────
parent │ a │ m
child │ a │ m
child2 │ a │ x
(3 rows)

To fix this, MergeAttributesIntoExisting() should check that the
attribute options of a child don't conflict with the parent, which the
attached patch implements. Note that partitioning uses the same code
as inheritance, so the fix applies to it too. After the patch:

create table p (a int, b text) partition by list (a);
create table p1 partition of p for values in (1);
select attrelid::regclass, attname, attstorage from pg_attribute where
attrelid in ('p'::regclass, 'p1'::regclass) and attname = 'b';
attrelid │ attname │ attstorage
──────────┼─────────┼────────────
p │ b │ x
p1 │ b │ x
(2 rows)

alter table p alter b set storage main;
select attrelid::regclass, attname, attstorage from pg_attribute where
attrelid in ('p'::regclass, 'p1'::regclass) and attname = 'b';
attrelid │ attname │ attstorage
──────────┼─────────┼────────────
p │ b │ m
p1 │ b │ m
(2 rows)

create table p2 (like p);
select attrelid::regclass, attname, attstorage from pg_attribute where
attrelid in ('p'::regclass, 'p1'::regclass, 'p2'::regclass) and
attname = 'b';
attrelid │ attname │ attstorage
──────────┼─────────┼────────────
p │ b │ m
p1 │ b │ m
p2 │ b │ x
(3 rows)

alter table p attach partition p2 for values in (2);
ERROR: child table "p2" has different storage option for column "b" than
parent
DETAIL: EXTENDED versus MAIN

-- ok after changing p2 to match
alter table p2 alter b set storage main;
alter table p attach partition p2 for values in (2);

Thanks,
Amit

Alvaro Herrera

alvherre@2ndquadrant.com

over 6 years ago

In reply to: Amit Langote (#2)

Re: Attached partition not considering altered column properties of root partition.

On 2019-Jul-03, Amit Langote wrote:

Thanks for the report. This seems like a bug. Documentation claims
that the child tables inherit column storage options from the parent
table. That's actually enforced in only some cases.

To fix this, MergeAttributesIntoExisting() should check that the
attribute options of a child don't conflict with the parent, which the
attached patch implements. Note that partitioning uses the same code
as inheritance, so the fix applies to it too. After the patch:

Thanks for the patch!

I'm not completely sold on back-patching this. Should we consider
changing it in 12beta and up only, or should we just backpatch it all
the way back to 9.4? It's going to raise errors in cases that
previously worked.

On the patch itself: I think ERRCODE_DATATYPE_MISMATCH is not the
correct one to use for this; maybe ERRCODE_INVALID_OBJECT_DEFINITION or,
more likely, ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE.

"FOO versus BAR" does not sound proper style. Maybe "Child table has
FOO, parent table expects BAR." Or maybe put it all in errmsg,
errmsg("child table \"%s\" has storage option \"%s\" for column \"%s\" mismatching \"%s\" on parent",

--
ï¿½lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Amit Langote

amitlangote09@gmail.com

over 6 years ago

In reply to: Alvaro Herrera (#4)

1 attachment(s)

Re: Attached partition not considering altered column properties of root partition.

Hi Alvaro,

Thanks for looking at this.

On Sat, Jul 27, 2019 at 8:38 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Thanks for the patch!

I'm not completely sold on back-patching this. Should we consider
changing it in 12beta and up only, or should we just backpatch it all
the way back to 9.4? It's going to raise errors in cases that
previously worked.

Applying the fix to only 12beta and up is perhaps fine. AFAICT,
there's no clear design reason for why the attribute storage property
must be the same in all child tables, so most users wouldn't even be
aware that we ensure that in some cases. OTOH, getting an error now
to ensure the invariant more strictly might be more annoying than
helpful.

On the patch itself: I think ERRCODE_DATATYPE_MISMATCH is not the
correct one to use for this; maybe ERRCODE_INVALID_OBJECT_DEFINITION or,
more likely, ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE.

OK, I prefer the latter.

"FOO versus BAR" does not sound proper style. Maybe "Child table has
FOO, parent table expects BAR." Or maybe put it all in errmsg,
errmsg("child table \"%s\" has storage option \"%s\" for column \"%s\" mismatching \"%s\" on parent",

I too found the "FOO versus BAR" style a bit odd, so changed to the
error message you suggest. There are other instances of this style
though:

$ git grep "%s versus %s"
src/backend/commands/tablecmds.c:
errdetail("%s versus %s",
src/backend/commands/tablecmds.c:
errdetail("%s versus %s",
src/backend/commands/tablecmds.c:
errdetail("%s versus %s",
src/backend/commands/tablecmds.c:
errdetail("%s versus %s",
src/backend/parser/parse_coerce.c:
errdetail("%s versus %s",
src/backend/parser/parse_coerce.c:
errdetail("%s versus %s",
src/backend/parser/parse_coerce.c:
errdetail("%s versus %s",
src/backend/parser/parse_coerce.c: errdetail("%s versus %s",
src/backend/parser/parse_coerce.c: errdetail("%s versus %s",
src/backend/parser/parse_param.c: errdetail("%s versus %s",

Should we leave them alone?

Attached updated patch.

Thanks,
Amit

Attachments:

attstorage-inherit-bug_v2.patchapplication/octet-stream; name=attstorage-inherit-bug_v2.patchDownload

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fb2be10794..33d79a6633 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13050,6 +13050,16 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
 						 errmsg("column \"%s\" in child table must be marked NOT NULL",
 								attributeName)));
 
+			/* No conflicting storage options allowed either. */
+			if (attribute->attstorage != childatt->attstorage)
+				ereport(ERROR,
+						(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+						 errmsg("child table \"%s\" has storage option \"%s\" for column \"%s\" mismatching \"%s\" on parent",
+								RelationGetRelationName(child_rel),
+								storage_name(childatt->attstorage),
+								attributeName,
+								storage_name(attribute->attstorage))));
+
 			/*
 			 * OK, bump the child column's inheritance count.  (If we fail
 			 * later on, this change will just roll back.)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index e5407bbf0f..eeb1bbce81 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4073,3 +4073,14 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
 drop table at_test_sql_partop;
 drop operator class at_test_sql_partop using btree;
 drop function at_test_sql_partop;
+-- check the inheritance of attribute storage option
+create table attstorage_inh_test (a text);
+alter table attstorage_inh_test alter a set storage main;
+create table attstorage_inh_test1 (a text);
+-- error
+alter table attstorage_inh_test1 inherit attstorage_inh_test;
+ERROR:  child table "attstorage_inh_test1" has storage option "EXTENDED" for column "a" mismatching "MAIN" on parent
+alter table attstorage_inh_test1 alter a set storage main;
+-- ok
+alter table attstorage_inh_test1 inherit attstorage_inh_test;
+drop table attstorage_inh_test, attstorage_inh_test1;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 99af0b851b..9e58c77260 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2712,3 +2712,14 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
 drop table at_test_sql_partop;
 drop operator class at_test_sql_partop using btree;
 drop function at_test_sql_partop;
+
+-- check the inheritance of attribute storage option
+create table attstorage_inh_test (a text);
+alter table attstorage_inh_test alter a set storage main;
+create table attstorage_inh_test1 (a text);
+-- error
+alter table attstorage_inh_test1 inherit attstorage_inh_test;
+alter table attstorage_inh_test1 alter a set storage main;
+-- ok
+alter table attstorage_inh_test1 inherit attstorage_inh_test;
+drop table attstorage_inh_test, attstorage_inh_test1;

Robert Haas

robertmhaas@gmail.com

over 6 years ago

In reply to: Amit Langote (#2)

Re: Attached partition not considering altered column properties of root partition.

On Tue, Jul 2, 2019 at 9:53 PM Amit Langote <amitlangote09@gmail.com> wrote:

Thanks for the report. This seems like a bug. Documentation claims
that the child tables inherit column storage options from the parent
table. That's actually enforced in only some cases.

I realize I'm just repeating the same argument I've already made
before on other related topics, but I don't think this is a bug.
Inherited-from-parent is not the same as
enforced-to-always-be-the-same-as-parent. Note that this is allowed,
changing only the child:

rhaas=# create table foo (a int, b text) partition by range (a);
CREATE TABLE
rhaas=# create table foo1 partition of foo for values from (0) to (10);
CREATE TABLE
rhaas=# alter table foo1 alter column b set storage plain;
ALTER TABLE

As is this, changing only the parent:

rhaas=# alter table only foo1 alter column b set storage plain;
ALTER TABLE

How can you possibly argue that the intended behavior is
everything-always-matches when that's not what's documented and
there's absolutely nothing that tries to enforce it?

I'm getting really tired of people thinking that they can invent rules
for table partitioning that were (1) never intended by the original
implementation and (2) not even slightly enforced by the code, and
then decide that those behavior changes should not only be made but
back-patched. That is just dead wrong. There is no problem here.
There is no need to change ANYTHING. There is even less need to do it
in the back branches.

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

Amit Langote

amitlangote09@gmail.com

over 6 years ago

In reply to: Robert Haas (#6)

Re: Attached partition not considering altered column properties of root partition.

On Wed, Jul 31, 2019 at 2:38 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Jul 2, 2019 at 9:53 PM Amit Langote <amitlangote09@gmail.com> wrote:

Thanks for the report. This seems like a bug. Documentation claims
that the child tables inherit column storage options from the parent
table. That's actually enforced in only some cases.

I realize I'm just repeating the same argument I've already made
before on other related topics, but I don't think this is a bug.
Inherited-from-parent is not the same as
enforced-to-always-be-the-same-as-parent. Note that this is allowed,
changing only the child:

rhaas=# create table foo (a int, b text) partition by range (a);
CREATE TABLE
rhaas=# create table foo1 partition of foo for values from (0) to (10);
CREATE TABLE
rhaas=# alter table foo1 alter column b set storage plain;
ALTER TABLE

As is this, changing only the parent:

rhaas=# alter table only foo1 alter column b set storage plain;
ALTER TABLE

How can you possibly argue that the intended behavior is
everything-always-matches when that's not what's documented and
there's absolutely nothing that tries to enforce it?

You're right. The patch as proposed is barely enough to ensure the
everything-always-matches behavior. Let's update^H^H^H^H^H^H^H forget
about the patch. :)

I do remember that we made a list of things that we decided must match
in all partitions, which ended up being slightly bigger than the same
list for regular inheritance children, but much smaller than the list
of all the things that could be different among children. I forgot we
did that when replying to Prabhat's report. In this particular case,
I do have doubts about whether we really need attstorage to be the
same in all the children, so maybe I should've first asked why we
should think of this as a bug.

I'm getting really tired of people thinking that they can invent rules
for table partitioning that were (1) never intended by the original
implementation and (2) not even slightly enforced by the code, and
then decide that those behavior changes should not only be made but
back-patched. That is just dead wrong. There is no problem here.
There is no need to change ANYTHING. There is even less need to do it
in the back branches.

OK, I'm withdrawing my patch.

Thanks,
Amit