Re: Attached partition not considering altered column properties of root partition.
On Wed, Jul 31, 2019 at 2:38 AM Robert Haas wrote: > > On Tue, Jul 2, 2019 at 9:53 PM 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. > > 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
Re: Attached partition not considering altered column properties of root partition.
On Tue, Jul 2, 2019 at 9:53 PM 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. 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
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 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
attstorage-inherit-bug_v2.patch
Description: Binary data
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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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 wrote:
> Hi Prabhat,
>
> On Tue, Jul 2, 2019 at 5:12 PM Prabhat Sahu
> 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
Re: Attached partition not considering altered column properties of root partition.
Hi Prabhat,
On Tue, Jul 2, 2019 at 5:12 PM Prabhat Sahu
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
attstorage-inherit-bug.patch
Description: Binary data
Attached partition not considering altered column properties of root partition.
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) postgres=# \d+ tt_p1 Table "public.tt_p1" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +-+---+--+-+--+--+- c1 | integer | | | | plain| | c2 | text| | | | extended | | Partition of: tab1 FOR VALUES FROM (20) TO (30) Partition constraint: ((c1 IS NOT NULL) AND (c1 >= 20) AND (c1 < 30)) Access method: heap -- With Regards, Prabhat Kumar Sahu
