Thread: 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
changedstaorage 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

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

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

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

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