Thread: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table
In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE
command for those schema elements of a table that could not be included
directly in the CREATE TABLE command for the table.
For example:
create table p (a int, b int) partition by range (a);
create table p1 partition of p for values from (1) to (10) partition by
range (b);
create table p11 partition of p1 for values from (1) to (10);
pg_dump -s gives:
CREATE TABLE p (
a integer NOT NULL,
b integer
)
PARTITION BY RANGE (a);
CREATE TABLE p1 PARTITION OF p
FOR VALUES FROM (1) TO (10)
PARTITION BY RANGE (b);
ALTER TABLE ONLY p1 ALTER COLUMN a SET NOT NULL;
ALTER TABLE ONLY p1 ALTER COLUMN b SET NOT NULL;
<snip>
Note the ONLY in the above emitted command. Now if I run the above
commands in another database, the following error occurs:
ERROR: constraint must be added to child tables too
That's because specifying ONLY for the AT commands on partitioned tables
that must recurse causes an error.
Attached patch fixes that - it prevents emitting ONLY for those ALTER
TABLE commands, which if run, would cause an error like the one above.
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
Attachment
Amit, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE > command for those schema elements of a table that could not be included > directly in the CREATE TABLE command for the table. Any chance we could start adding regression tests for how pg_dump handles partitions? I'm just about to the point where I have pretty much everything else covered (at least in pg_dump.c, where it's not a hard-to-reproduce error/exit case, or something version-dependent). If you have any questions about how the TAP tests for pg_dump work, or about how to generate code-coverage checks to make sure you're at least hitting every line (tho, of course, not every possible path), let me know. I'd be happy to explain them. Thanks! Stephen
Hi Stephen, On 2017/02/17 22:32, Stephen Frost wrote: > Amit, > > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: >> In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE >> command for those schema elements of a table that could not be included >> directly in the CREATE TABLE command for the table. > > Any chance we could start adding regression tests for how pg_dump > handles partitions? I'm just about to the point where I have pretty > much everything else covered (at least in pg_dump.c, where it's not a > hard-to-reproduce error/exit case, or something version-dependent). > > If you have any questions about how the TAP tests for pg_dump work, or > about how to generate code-coverage checks to make sure you're at least > hitting every line (tho, of course, not every possible path), let me > know. I'd be happy to explain them. Yeah, I guess it would be a good idea to have some pg_dump TAP test coverage for the new partitioning stuff. I will look into that and get back to you if I don't grok something there. Thanks, Amit
Amit, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > On 2017/02/17 22:32, Stephen Frost wrote: > > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > >> In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE > >> command for those schema elements of a table that could not be included > >> directly in the CREATE TABLE command for the table. > > > > Any chance we could start adding regression tests for how pg_dump > > handles partitions? I'm just about to the point where I have pretty > > much everything else covered (at least in pg_dump.c, where it's not a > > hard-to-reproduce error/exit case, or something version-dependent). > > > > If you have any questions about how the TAP tests for pg_dump work, or > > about how to generate code-coverage checks to make sure you're at least > > hitting every line (tho, of course, not every possible path), let me > > know. I'd be happy to explain them. > > Yeah, I guess it would be a good idea to have some pg_dump TAP test > coverage for the new partitioning stuff. I will look into that and get > back to you if I don't grok something there. As you may have seen, I've added some tests to the pg_dump TAP tests for partitioning to cover lines of code not already covered. There are still some bits not covered though, which you can see here: https://coverage.postgresql.org/src/bin/pg_dump/pg_dump.c.gcov.html If you have any questions about the way the pg_dump tests work, feel free to ask. Thanks! Stephen
On Fri, Feb 17, 2017 at 3:23 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE > command for those schema elements of a table that could not be included > directly in the CREATE TABLE command for the table. > > For example: > > create table p (a int, b int) partition by range (a); > create table p1 partition of p for values from (1) to (10) partition by > range (b); > create table p11 partition of p1 for values from (1) to (10); > > pg_dump -s gives: > > CREATE TABLE p ( > a integer NOT NULL, > b integer > ) > PARTITION BY RANGE (a); > > CREATE TABLE p1 PARTITION OF p > FOR VALUES FROM (1) TO (10) > PARTITION BY RANGE (b); > ALTER TABLE ONLY p1 ALTER COLUMN a SET NOT NULL; > ALTER TABLE ONLY p1 ALTER COLUMN b SET NOT NULL; > > <snip> > > Note the ONLY in the above emitted command. Now if I run the above > commands in another database, the following error occurs: > > ERROR: constraint must be added to child tables too > > That's because specifying ONLY for the AT commands on partitioned tables > that must recurse causes an error. > > Attached patch fixes that - it prevents emitting ONLY for those ALTER > TABLE commands, which if run, would cause an error like the one above. Isn't it bogus that this is generating ALTER TABLE .. SET NOT NULL columns at all? You didn't say anything like that when setting up the database, so why should it be there when dumping? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi Stephen, On 2017/03/21 1:40, Stephen Frost wrote: > Amit, > > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: >> On 2017/02/17 22:32, Stephen Frost wrote: >>> * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: >>>> In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE >>>> command for those schema elements of a table that could not be included >>>> directly in the CREATE TABLE command for the table. >>> >>> Any chance we could start adding regression tests for how pg_dump >>> handles partitions? I'm just about to the point where I have pretty >>> much everything else covered (at least in pg_dump.c, where it's not a >>> hard-to-reproduce error/exit case, or something version-dependent). >>> >>> If you have any questions about how the TAP tests for pg_dump work, or >>> about how to generate code-coverage checks to make sure you're at least >>> hitting every line (tho, of course, not every possible path), let me >>> know. I'd be happy to explain them. >> >> Yeah, I guess it would be a good idea to have some pg_dump TAP test >> coverage for the new partitioning stuff. I will look into that and get >> back to you if I don't grok something there. > > As you may have seen, I've added some tests to the pg_dump TAP tests for > partitioning to cover lines of code not already covered. There are > still some bits not covered though, which you can see here: > > https://coverage.postgresql.org/src/bin/pg_dump/pg_dump.c.gcov.html > > If you have any questions about the way the pg_dump tests work, feel > free to ask. Sorry that it took me week to thank you for doing this. Thanks, Amit
On 2017/03/27 23:30, Robert Haas wrote: > On Fri, Feb 17, 2017 at 3:23 AM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE >> command for those schema elements of a table that could not be included >> directly in the CREATE TABLE command for the table. >> >> For example: >> >> create table p (a int, b int) partition by range (a); >> create table p1 partition of p for values from (1) to (10) partition by >> range (b); >> create table p11 partition of p1 for values from (1) to (10); >> >> pg_dump -s gives: >> >> CREATE TABLE p ( >> a integer NOT NULL, >> b integer >> ) >> PARTITION BY RANGE (a); >> >> CREATE TABLE p1 PARTITION OF p >> FOR VALUES FROM (1) TO (10) >> PARTITION BY RANGE (b); >> ALTER TABLE ONLY p1 ALTER COLUMN a SET NOT NULL; >> ALTER TABLE ONLY p1 ALTER COLUMN b SET NOT NULL; >> >> <snip> >> >> Note the ONLY in the above emitted command. Now if I run the above >> commands in another database, the following error occurs: >> >> ERROR: constraint must be added to child tables too >> >> That's because specifying ONLY for the AT commands on partitioned tables >> that must recurse causes an error. >> >> Attached patch fixes that - it prevents emitting ONLY for those ALTER >> TABLE commands, which if run, would cause an error like the one above. > > Isn't it bogus that this is generating ALTER TABLE .. SET NOT NULL > columns at all? You didn't say anything like that when setting up the > database, so why should it be there when dumping? So we should find a way for the NOT NULL constraints added for the range partition key columns to not be emitted *separately*? Like when a table has primary key: -- -- Name: foo; Type: TABLE; Schema: public; Owner: amit -- CREATE TABLE foo ( a integer NOT NULL ); ALTER TABLE foo OWNER TO amit; -- -- Name: foo foo_pkey; Type: CONSTRAINT; Schema: public; Owner: amit -- ALTER TABLE ONLY foo ADD CONSTRAINT foo_pkey PRIMARY KEY (a); The NOT NULL constraint is emitted with CREATE TABLE, not separately. Thanks, Amit
On Tue, Mar 28, 2017 at 6:50 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> Isn't it bogus that this is generating ALTER TABLE .. SET NOT NULL >> columns at all? You didn't say anything like that when setting up the >> database, so why should it be there when dumping? > > So we should find a way for the NOT NULL constraints added for the range > partition key columns to not be emitted *separately*? Like when a table > has primary key: > > -- > -- Name: foo; Type: TABLE; Schema: public; Owner: amit > -- > > CREATE TABLE foo ( > a integer NOT NULL > ); > > > ALTER TABLE foo OWNER TO amit; > > -- > -- Name: foo foo_pkey; Type: CONSTRAINT; Schema: public; Owner: amit > -- > > ALTER TABLE ONLY foo > ADD CONSTRAINT foo_pkey PRIMARY KEY (a); > > The NOT NULL constraint is emitted with CREATE TABLE, not separately. Hmm, that's not exactly what I was thinking, but I think what I was thinking was wrong, so, yes, can we do what you said? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2017/03/29 0:39, Robert Haas wrote: > On Tue, Mar 28, 2017 at 6:50 AM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: >>> Isn't it bogus that this is generating ALTER TABLE .. SET NOT NULL >>> columns at all? You didn't say anything like that when setting up the >>> database, so why should it be there when dumping? >> >> So we should find a way for the NOT NULL constraints added for the range >> partition key columns to not be emitted *separately*? Like when a table >> has primary key: >> >> -- >> -- Name: foo; Type: TABLE; Schema: public; Owner: amit >> -- >> >> CREATE TABLE foo ( >> a integer NOT NULL >> ); >> >> >> ALTER TABLE foo OWNER TO amit; >> >> -- >> -- Name: foo foo_pkey; Type: CONSTRAINT; Schema: public; Owner: amit >> -- >> >> ALTER TABLE ONLY foo >> ADD CONSTRAINT foo_pkey PRIMARY KEY (a); >> >> The NOT NULL constraint is emitted with CREATE TABLE, not separately. > > Hmm, that's not exactly what I was thinking, but I think what I was > thinking was wrong, so, yes, can we do what you said? I thought about this for a while. Although it seems we can do what I said for (partitioned) tables themselves, it's not real clear to me how straightforward it is to do for partitions (child tables). Inheritance or localness of attributes/constraints including NOT NULL dictates whether an attribute or a constraint is emitted separately. I think that any additional consideration will make the logic to *not* dump separately (or perhaps to not emit at all) will become more involved. For example, if a NOT NULL constraint on a column has been inherited and originated in the parent from the range partition key, then does it mean we should not emit it or emit it but not separately? Thanks, Amit
On Wed, Mar 29, 2017 at 05:38:41PM +0900, Amit Langote wrote: > On 2017/03/29 0:39, Robert Haas wrote: > > On Tue, Mar 28, 2017 at 6:50 AM, Amit Langote > > <Langote_Amit_f8@lab.ntt.co.jp> wrote: > >>> Isn't it bogus that this is generating ALTER TABLE .. SET NOT NULL > >>> columns at all? You didn't say anything like that when setting up the > >>> database, so why should it be there when dumping? > >> > >> So we should find a way for the NOT NULL constraints added for the range > >> partition key columns to not be emitted *separately*? Like when a table > >> has primary key: > >> > >> -- > >> -- Name: foo; Type: TABLE; Schema: public; Owner: amit > >> -- > >> > >> CREATE TABLE foo ( > >> a integer NOT NULL > >> ); > >> > >> > >> ALTER TABLE foo OWNER TO amit; > >> > >> -- > >> -- Name: foo foo_pkey; Type: CONSTRAINT; Schema: public; Owner: amit > >> -- > >> > >> ALTER TABLE ONLY foo > >> ADD CONSTRAINT foo_pkey PRIMARY KEY (a); > >> > >> The NOT NULL constraint is emitted with CREATE TABLE, not separately. > > > > Hmm, that's not exactly what I was thinking, but I think what I was > > thinking was wrong, so, yes, can we do what you said? > > I thought about this for a while. Although it seems we can do what I said > for (partitioned) tables themselves, it's not real clear to me how > straightforward it is to do for partitions (child tables). Inheritance or > localness of attributes/constraints including NOT NULL dictates whether an > attribute or a constraint is emitted separately. I think that any > additional consideration will make the logic to *not* dump separately (or > perhaps to not emit at all) will become more involved. For example, if a > NOT NULL constraint on a column has been inherited and originated in the > parent from the range partition key, then does it mean we should not emit > it or emit it but not separately? [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Robert, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On Sun, Apr 9, 2017 at 7:50 PM, Noah Misch <noah@leadboat.com> wrote: > The above-described topic is currently a PostgreSQL 10 open item. Robert, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. I would appreciate help from other contributors and committers on this open item; pg_dump is not my strong point. In the absence of such help, I will do my best with it. I will set aside time this week to study this and send another update no later than Thursday. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> I would appreciate help from other contributors and committers on this
> open item; pg_dump is not my strong point. In the absence of such
> help, I will do my best with it. I will set aside time this week to
> study this and send another update no later than Thursday.
The proposed patch seems rather ad-hoc, and I think that it is working
around a backend behavior that might be broken.
While I admit that I've not been paying close attention to the whole
table partitioning business, I wonder whether we have any clearly written
down specification about (a) how much partition member tables are allowed
to deviate schema-wise from their parent, and (b) how DDL semantics on
partitioned tables differ from DDL semantics for traditional inheritance.
Obviously those are closely related questions. But the fact that this
bug exists at all shows that there's been some lack of clarity on (b),
and so I wonder whether we have any clarity on (a) either.
regards, tom lane
Tom, Robert, * Tom Lane (tgl@sss.pgh.pa.us) wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > I would appreciate help from other contributors and committers on this > > open item; pg_dump is not my strong point. In the absence of such > > help, I will do my best with it. I will set aside time this week to > > study this and send another update no later than Thursday. > > The proposed patch seems rather ad-hoc, and I think that it is working > around a backend behavior that might be broken. Yeah, I'm not a big fan of the proposed patch either. > While I admit that I've not been paying close attention to the whole > table partitioning business, I wonder whether we have any clearly written > down specification about (a) how much partition member tables are allowed > to deviate schema-wise from their parent, and (b) how DDL semantics on > partitioned tables differ from DDL semantics for traditional inheritance. > Obviously those are closely related questions. But the fact that this > bug exists at all shows that there's been some lack of clarity on (b), > and so I wonder whether we have any clarity on (a) either. Right. I thought I had understood that partition child tables really can't deviate from the parent table in terms of columns or constraints but might be allowed to differ with regard to indexes (?). I'll try looking into this also, I should be able to spend some time on it this week. Thanks! Stephen
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> While I admit that I've not been paying close attention to the whole
>> table partitioning business, I wonder whether we have any clearly written
>> down specification about (a) how much partition member tables are allowed
>> to deviate schema-wise from their parent, and (b) how DDL semantics on
>> partitioned tables differ from DDL semantics for traditional inheritance.
> Right. I thought I had understood that partition child tables really
> can't deviate from the parent table in terms of columns or constraints
> but might be allowed to differ with regard to indexes (?).
Well, there's an awful lot of logic that's only of interest if partition
children can have different column orders from their parents. I really
fail to understand what's the point of allowing that.
regards, tom lane
On Sun, Apr 9, 2017 at 10:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > While I admit that I've not been paying close attention to the whole > table partitioning business, I wonder whether we have any clearly written > down specification about (a) how much partition member tables are allowed > to deviate schema-wise from their parent, and (b) how DDL semantics on > partitioned tables differ from DDL semantics for traditional inheritance. > Obviously those are closely related questions. But the fact that this > bug exists at all shows that there's been some lack of clarity on (b), > and so I wonder whether we have any clarity on (a) either. Children can have constraints (including NOT NULL constraints) which parents lack, and can have a different column order, but must have exactly the same column names and types. The point here is, of course, not that there's any real value in the parent columns being (a, b) and the child columns being (b, a), although we do allow that, but rather than somebody might have a parent with (a, b) and a child that has those plus a dropped column. Explaining to a user - to whom the dropped column is invisible - why that child couldn't be attached as a partition of that parent would be difficult, so it seemed best (to me, anyway, and I think to other people who were paying attention) to rule that the partitioning code has to cope with the possibility of attribute numbers varying across partitions. (Also consider the reverse case, where the parent has a dropped column and the prospective child doesn't have one with the same width in the same location.) In Amit's example from the original post, the child has an implicit NOT NULL constraint that does not exist in the parent. p1.b isn't declared NOT NULL, but the fact that it is range-partitioned on b requires it to be so, just as we would do if b were declared as the PRIMARY KEY. Somehow that's not playing nice with pg_dump, but I'm still fuzzy on the details. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2017/04/11 0:26, Robert Haas wrote:
> On Sun, Apr 9, 2017 at 10:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> While I admit that I've not been paying close attention to the whole
>> table partitioning business, I wonder whether we have any clearly written
>> down specification about (a) how much partition member tables are allowed
>> to deviate schema-wise from their parent, and (b) how DDL semantics on
>> partitioned tables differ from DDL semantics for traditional inheritance.
>> Obviously those are closely related questions. But the fact that this
>> bug exists at all shows that there's been some lack of clarity on (b),
>> and so I wonder whether we have any clarity on (a) either.
>
> Children can have constraints (including NOT NULL constraints) which
> parents lack, and can have a different column order, but must have
> exactly the same column names and types.
Also, what is different in the partitioned parent case is that NOT NULL
constraints must be inherited. That is, one cannot add it only to the parent.
--
-- traditional inheritance
--
create table parent (a int);
create table child () inherits (parent);
alter table only parent add constraint chka check (a > 0);
-- ERROR: constraint must be added to child tables too
-- the following is OK, because of the explicit NO INHERIT request
alter table only parent add constraint chka check (a > 0) no inherit;
-- the following is also OK, because NOT NULL constraints are not
-- inherited with regular inheritance
alter table only parent alter a set not null;
--
-- partitioning inheritance
--
create table parted_parent (a int) partition by list (a);
create table part partition of parted_parent for values in (1);
-- this is same as traditional inheritance
alter table only parted_parent add constraint chka check (a > 0);
-- ERROR: constraint must be added to child tables too
-- requesting NO INHERIT doesn't make sense on what is an empty relation
-- by itself
alter table only parted_parent add constraint chka check (a > 0) no inherit;
-- ERROR: cannot add NO INHERIT constraint to partitioned table
"parted_table"
-- NOT NULL constraint must be inherited too
alter table only parted_parent alter a set not null;
-- ERROR: constraint must be added to child tables too
-- both ok (no inheritance here)
alter table part alter a set not null;
alter table part alter a drop not null;
-- request whole-tree constraint
alter table parted_parent alter a set not null;
-- can't drop it from a partition
alter table part alter a drop not null;
-- ERROR: column "a" is marked NOT NULL in parent table
> In Amit's example from the original post, the child has an implicit
> NOT NULL constraint that does not exist in the parent. p1.b isn't
> declared NOT NULL, but the fact that it is range-partitioned on b
> requires it to be so, just as we would do if b were declared as the
> PRIMARY KEY. Somehow that's not playing nice with pg_dump, but I'm
> still fuzzy on the details.
Actually, I would like to change the problem definition from "ALTER TABLE
ONLY partitioned_table should be avoided" to "Emitting partition's
attributes separately should be avoided".
There is a shouldPrintColumn() which returns true if a table's attribute
should be emitted in the CREATE TABLE command at all. For example, it
won't be necessary if the attribute is purely inherited
(attislocal=false). But once an attribute is determined to not be
emitted, any attribute-level constraints and defaults need to be marked to
be emitted separately (using ALTER TABLE ONLY), which can be undesirable
because of the rules about using ONLY with declarative partitioning
inheritance.
I think we can change shouldPrintColumn() so that it always returns true
if the table is a partition, that is, a partition's attributes should
always be emitted with CREATE TABLE, if necessary (it isn't required if
there isn't a NOT NULL constraint or DEFAULT defined on the attributes).
When dumping a partition using the PARTITION OF syntax, an attribute will
emitted if it has a NOT NULL constraint or a default.
So if we have:
create table p (a int, b int) partition by list (a);
create table p1 partition of p for values in (1) partition by range (b);
create table p11 partition of p1 (
a not null default '1'
) for values from (1) to (10);
Proposed would make pg_dump -s output the following (some lines removed
for clarity):
CREATE TABLE p (
a integer,
b integer
)
PARTITION BY LIST (a);
CREATE TABLE p1 PARTITION OF p (
b NOT NULL
)
FOR VALUES IN (1)
PARTITION BY RANGE (b);
CREATE TABLE p11 PARTITION OF p1 (
a DEFAULT 1 NOT NULL,
b NOT NULL
)
FOR VALUES FROM (1) TO (10);
Which also happens to a legal input to the backend, as far as partitioning
is concerned. With the existing approach, NOT NULL on p1's b attribute is
emitted as ALTER TABLE ONLY p1 ALTER b SET NOT NULL, which would cause
error, because p1 is partitioned.
Attached patch implements that and also updates some comments. I think
some updates to pg_dump's regression tests for partitioned tables will be
required, but my attempt to understand how to go about updating the
expected output failed. I will try again tomorrow.
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
Attachment
Amit, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > On 2017/04/11 0:26, Robert Haas wrote: > > Children can have constraints (including NOT NULL constraints) which > > parents lack, and can have a different column order, but must have > > exactly the same column names and types. > > Also, what is different in the partitioned parent case is that NOT NULL > constraints must be inherited. That is, one cannot add it only to the parent. If I'm following, children can have additional constraints but any constraints on the parent must also exist on all the children. Is that correct? > -- > -- partitioning inheritance > -- > create table parted_parent (a int) partition by list (a); > create table part partition of parted_parent for values in (1); > > -- this is same as traditional inheritance > alter table only parted_parent add constraint chka check (a > 0); > -- ERROR: constraint must be added to child tables too Ok, this makes sense, but surely that constraint does, in fact, exist on the child already or we wouldn't be trying to dump out this constraint that exists on the parent? > > In Amit's example from the original post, the child has an implicit > > NOT NULL constraint that does not exist in the parent. p1.b isn't > > declared NOT NULL, but the fact that it is range-partitioned on b > > requires it to be so, just as we would do if b were declared as the > > PRIMARY KEY. Somehow that's not playing nice with pg_dump, but I'm > > still fuzzy on the details. > > Actually, I would like to change the problem definition from "ALTER TABLE > ONLY partitioned_table should be avoided" to "Emitting partition's > attributes separately should be avoided". I don't follow why we think doing: CREATE TABLE t1 (c1 int); ALTER TABLE ONLY t1 SET c1 NOT NULL; is really different from: CREATE TABLE t1 (c1 int NOT NULL); or why we should teach pg_dump that it's "correct" to consider those two to be different. There are specific cases where they have to be done independently, but that's for views because we don't have a way to set a default on a view column during CREATE VIEW, or to deal with dropped columns or traditionally inheirited columns. What isn't clear to me is why the CREATE TABLE + ALTER TABLE isn't working, when apparently a CREATE TABLE with the NOT NULL included would work. The issue here seems like it's the order in which the operations are happening in, and not that CREATE TABLE + ALTER TABLE is somehow different than just the CREATE TABLE. > create table p (a int, b int) partition by list (a); > create table p1 partition of p for values in (1) partition by range (b); > create table p11 partition of p1 ( > a not null default '1' > ) for values from (1) to (10); Using the above example, doing a pg_dump and then a restore (into a clean initdb'd cluster), I get the following: =# CREATE TABLE p ( -# a integer, -# b integer -# ) -# PARTITION BY LIST (a); CREATE TABLE =*# CREATE TABLE p1 PARTITION OF p -*# FOR VALUES IN (1) -*# PARTITION BY RANGE (b); CREATE TABLE =*# ALTER TABLE ONLY p1 ALTER COLUMN b SET NOT NULL; ERROR: constraint must be added to child tables too Now, perhaps I'm confused, but isn't p1 the child here? Which is supposed to be able to have constraints that the parent doesn't? We haven't even gotten to the point where p1 is a parent yet because p11 hasn't been created yet. Further, according to psql's \d, 'p1.b' already has a NOT NULL constraint on it, so the above really should just be a no-op. I get the feeling that we're looking in the wrong place for the issue here. Thanks! Stephen
Hi Stephen, On 2017/04/11 22:12, Stephen Frost wrote: > Amit, > > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: >> On 2017/04/11 0:26, Robert Haas wrote: >>> Children can have constraints (including NOT NULL constraints) which >>> parents lack, and can have a different column order, but must have >>> exactly the same column names and types. >> >> Also, what is different in the partitioned parent case is that NOT NULL >> constraints must be inherited. That is, one cannot add it only to the parent. > > If I'm following, children can have additional constraints but any > constraints on the parent must also exist on all the children. Is that > correct? Yes. As shown in the examples I posted, in traditional inheritance, only CHECK constraints are treated that way, whereas NOT NULL constraints are not. With partitioning, even the NOT NULL constraints are inherited. So if a particular column is set NOT NULL in the parent, all partitions in the tree must have that constraint and it cannot be dropped from a partition without dropping it from the parent as well. Traditional inheritance applies that rule only to attributes and CHECK constraints. >> -- >> -- partitioning inheritance >> -- >> create table parted_parent (a int) partition by list (a); >> create table part partition of parted_parent for values in (1); >> >> -- this is same as traditional inheritance >> alter table only parted_parent add constraint chka check (a > 0); >> -- ERROR: constraint must be added to child tables too > > Ok, this makes sense, but surely that constraint does, in fact, exist on > the child already or we wouldn't be trying to dump out this constraint > that exists on the parent? Yes, that's right. Now that I have thought about this a bit more, I think I can articulate the problem statement more clearly. The problem we have here is about non-inherited constraints on partitions, specifically, the NOT NULL constraints which need to be emitted per attribute. We have two options of doing it: emit it inline with CREATE TABLE or separately with ALTER TABLE ONLY. I have been saying that it is preferable to use CREATE TABLE, because ALTER TABLE ONLY will fail if the partition is itself partitioned. Why does it cause an error? Because of the ONLY part. It is a user error to specify ONLY when adding a constraint to a partitioned tables, but... >>> In Amit's example from the original post, the child has an implicit >>> NOT NULL constraint that does not exist in the parent. p1.b isn't >>> declared NOT NULL, but the fact that it is range-partitioned on b >>> requires it to be so, just as we would do if b were declared as the >>> PRIMARY KEY. Somehow that's not playing nice with pg_dump, but I'm >>> still fuzzy on the details. >> >> Actually, I would like to change the problem definition from "ALTER TABLE >> ONLY partitioned_table should be avoided" to "Emitting partition's >> attributes separately should be avoided". > > I don't follow why we think doing: > > CREATE TABLE t1 (c1 int); > ALTER TABLE ONLY t1 SET c1 NOT NULL; > > is really different from: > > CREATE TABLE t1 (c1 int NOT NULL); > > or why we should teach pg_dump that it's "correct" to consider those two > to be different. There are specific cases where they have to be done > independently, but that's for views because we don't have a way to set a > default on a view column during CREATE VIEW, or to deal with dropped > columns or traditionally inheirited columns. > > What isn't clear to me is why the CREATE TABLE + ALTER TABLE isn't > working, when apparently a CREATE TABLE with the NOT NULL included would > work. The issue here seems like it's the order in which the operations > are happening in, and not that CREATE TABLE + ALTER TABLE is somehow > different than just the CREATE TABLE. I think you are right. CREATE TABLE + ALTER TABLE ONLY should work just fine in this particular case (i.e. the way pg_dump outputs it). >> create table p (a int, b int) partition by list (a); >> create table p1 partition of p for values in (1) partition by range (b); >> create table p11 partition of p1 ( >> a not null default '1' >> ) for values from (1) to (10); > > Using the above example, doing a pg_dump and then a restore (into a > clean initdb'd cluster), I get the following: > > =# CREATE TABLE p ( > -# a integer, > -# b integer > -# ) > -# PARTITION BY LIST (a); > CREATE TABLE > > =*# CREATE TABLE p1 PARTITION OF p > -*# FOR VALUES IN (1) > -*# PARTITION BY RANGE (b); > CREATE TABLE > > =*# ALTER TABLE ONLY p1 ALTER COLUMN b SET NOT NULL; > ERROR: constraint must be added to child tables too > > Now, perhaps I'm confused, but isn't p1 the child here? Which is > supposed to be able to have constraints that the parent doesn't? Actually, p1 is a partitioned table, so the error. And I realize that that's a wrong behavior. Currently the check is performed using only the relkind, which is bogus. Specifying ONLY should cause an error only when the table has partitions. > We haven't even gotten to the point where p1 is a parent yet because p11 > hasn't been created yet. Further, according to psql's \d, 'p1.b' > already has a NOT NULL constraint on it, so the above really should just > be a no-op. Right. The already existing NOT NULL constraint results from p1.b being the range partition key column. > I get the feeling that we're looking in the wrong place for the issue > here. Yes, I think we should consider fixing the backend behavior here as Tom also suspected. Throwing an error when no partitions yet exist might be a bad idea after all. Also, I mentioned above that the problem is only with non-inherited constraints of partitions, but I've just discovered an issue with inherited constraints as well. They need not be emitted for partitions *again*, but they are. So, here are the patches: 0001: Fix ALTER TABLE .. SET/DROP NOT NULL and DROP CONSTRAINT so that they don't cause an error when ONLY is specified and no partitions exist. 0002: Fix pg_dump so that inherited constraints are not printed separately for partitions. (pg_dump TAP test output is updated) 0003: Fix pg_dump to not emit WITH OPTIONS when printing constraints of a partition's columns It would be great if you could take a look. 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
Attachment
On Wed, Apr 12, 2017 at 6:29 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > Actually, p1 is a partitioned table, so the error. And I realize that > that's a wrong behavior. Currently the check is performed using only the > relkind, which is bogus. Specifying ONLY should cause an error only when > the table has partitions. That sounds like a REALLY bad idea, because now you're going to end up with a constraint that can never be enforced against any actual data rows ... or else you're going to later pretend that ONLY wasn't specified. I think the rule that partitioned tables can't have non-inherited constraints is absolutely right, and relaxing it is quite wrong. I think you had the right idea upthread when you suggested dumping it this way: CREATE TABLE p1 PARTITION OF p ( b NOT NULL ) FOR VALUES IN (1) PARTITION BY RANGE (b); That looks absolutely right to me, and very much principled. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Wed, Apr 12, 2017 at 6:29 AM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > Actually, p1 is a partitioned table, so the error. And I realize that > > that's a wrong behavior. Currently the check is performed using only the > > relkind, which is bogus. Specifying ONLY should cause an error only when > > the table has partitions. > > That sounds like a REALLY bad idea, because now you're going to end up > with a constraint that can never be enforced against any actual data > rows ... or else you're going to later pretend that ONLY wasn't > specified. I think the rule that partitioned tables can't have > non-inherited constraints is absolutely right, and relaxing it is > quite wrong. I'm not following what you're getting at here. There's already a constraint on the table, and ALTER TABLE ONLY doesn't say anything about what happens later on (certainly it doesn't make new tables created with 'LIKE' have bits omitted, if that's what you were thinking). Lastly, the error being thrown certainly seems to imply that one needs to go fix all the child tables to have the constraint first and then the constraint can be added to the parent (presumably using the same ALTER TABLE ONLY command). If there aren't any child tables, then it should work, if there *are* child tables and they've got the necessary constraint, then this should be allowed, so that future child tables create will have the constraint. > I think you had the right idea upthread when you suggested dumping it this way: > > CREATE TABLE p1 PARTITION OF p ( > b NOT NULL > ) > FOR VALUES IN (1) > PARTITION BY RANGE (b); > > That looks absolutely right to me, and very much principled. It's not principled at all to claim that CREATE TABLE + NOT NULL is somehow different from CREATE TABLE + ALTER TABLE SET NOT NULL and that one should work and the other shouldn't. Thanks! Stephen
On Wed, Apr 12, 2017 at 3:29 PM, Stephen Frost <sfrost@snowman.net> wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: >> On Wed, Apr 12, 2017 at 6:29 AM, Amit Langote >> <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> > Actually, p1 is a partitioned table, so the error. And I realize that >> > that's a wrong behavior. Currently the check is performed using only the >> > relkind, which is bogus. Specifying ONLY should cause an error only when >> > the table has partitions. >> >> That sounds like a REALLY bad idea, because now you're going to end up >> with a constraint that can never be enforced against any actual data >> rows ... or else you're going to later pretend that ONLY wasn't >> specified. I think the rule that partitioned tables can't have >> non-inherited constraints is absolutely right, and relaxing it is >> quite wrong. > > I'm not following what you're getting at here. Urk, I might be confusing ONLY with NO INHERIT. Let me think about this again... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Apr 12, 2017 at 3:29 PM, Stephen Frost <sfrost@snowman.net> wrote: > I'm not following what you're getting at here. > > There's already a constraint on the table, and ALTER TABLE ONLY doesn't > say anything about what happens later on (certainly it doesn't make new > tables created with 'LIKE' have bits omitted, if that's what you were > thinking). Lastly, the error being thrown certainly seems to imply that > one needs to go fix all the child tables to have the constraint first > and then the constraint can be added to the parent (presumably using the > same ALTER TABLE ONLY command). If there aren't any child tables, then > it should work, if there *are* child tables and they've got the > necessary constraint, then this should be allowed, so that future child > tables create will have the constraint. So I think I was indeed confused before, and I think you're basically right here, but on one point I think you are not right -- ALTER TABLE ONLY .. CHECK () doesn't work on a table with inheritance children regardless of whether the children already have the matching constraint: rhaas=# create table foo (a int, b text); CREATE TABLE rhaas=# create table bar () inherits (foo); CREATE TABLE rhaas=# alter table only foo add check (a = 1); ERROR: constraint must be added to child tables too rhaas=# alter table only bar add check (a = 1); ALTER TABLE rhaas=# alter table only foo add check (a = 1); ERROR: constraint must be added to child tables too It looks like ALTER TABLE ONLY works find on a table with no children, but once it's got children it no longer works, period. However, you're right that you can add the constraint to the as-yet-childless table and then future children will inherit the constraint properly. Continuing the previous example: rhaas=# drop table bar; DROP TABLE rhaas=# alter table only foo add check (a = 1); ALTER TABLE rhaas=# create table bar () inherits (foo); CREATE TABLE So, regarding Amit's 0001: - I think we should update the relevant hunk of the documentation rather than just removing it. - Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo .. to work on a partitioned table without partitions, or is that just pointless tinkering? That seems to be the only case where, after this patch, an ONLY operation will fail on a childless partitioned table. Do you want to be responsible for committing these or should I do it? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2017/04/13 6:22, Robert Haas wrote: > On Wed, Apr 12, 2017 at 3:29 PM, Stephen Frost <sfrost@snowman.net> wrote: >> I'm not following what you're getting at here. >> >> There's already a constraint on the table, and ALTER TABLE ONLY doesn't >> say anything about what happens later on (certainly it doesn't make new >> tables created with 'LIKE' have bits omitted, if that's what you were >> thinking). Lastly, the error being thrown certainly seems to imply that >> one needs to go fix all the child tables to have the constraint first >> and then the constraint can be added to the parent (presumably using the >> same ALTER TABLE ONLY command). If there aren't any child tables, then >> it should work, if there *are* child tables and they've got the >> necessary constraint, then this should be allowed, so that future child >> tables create will have the constraint. > > So I think I was indeed confused before, and I think you're basically > right here, but on one point I think you are not right -- ALTER TABLE > ONLY .. CHECK () doesn't work on a table with inheritance children > regardless of whether the children already have the matching > constraint: > > rhaas=# create table foo (a int, b text); > CREATE TABLE > rhaas=# create table bar () inherits (foo); > CREATE TABLE > rhaas=# alter table only foo add check (a = 1); > ERROR: constraint must be added to child tables too > rhaas=# alter table only bar add check (a = 1); > ALTER TABLE > rhaas=# alter table only foo add check (a = 1); > ERROR: constraint must be added to child tables too > > It looks like ALTER TABLE ONLY works find on a table with no children, > but once it's got children it no longer works, period. By the way, there is a workaround with traditional inheritance: alter table only foo add constraint chka check (a > 0) no inherit; ALTER TABLE But we don't allow NO INHERIT constraints on partitioned tables, so we will get an error with them anyway. alter table only parted_parent add constraint chka check (a > 0) no inherit; ERROR: cannot add NO INHERIT constraint to partitioned table "parted_parent" > However, > you're right that you can add the constraint to the as-yet-childless > table and then future children will inherit the constraint properly. > Continuing the previous example: > > rhaas=# drop table bar; > DROP TABLE > rhaas=# alter table only foo add check (a = 1); > ALTER TABLE > rhaas=# create table bar () inherits (foo); > CREATE TABLE > > So, regarding Amit's 0001: > > - I think we should update the relevant hunk of the documentation > rather than just removing it. OK, I agree. I tweaked the existing bullet point about differences from traditional inheritance when using ONLY with partitioned tables. > - Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo > .. to work on a partitioned table without partitions, or is that just > pointless tinkering? That seems to be the only case where, after this > patch, an ONLY operation will fail on a childless partitioned table. I fixed TRUNCATE ONLY to not complain when no partitions exist. Patch already takes care of the ALTER TABLE ONLY cases. Updated patches attached (0002 and 0003 unchanged). 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
Attachment
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > So I think I was indeed confused before, and I think you're basically > right here, but on one point I think you are not right -- ALTER TABLE > ONLY .. CHECK () doesn't work on a table with inheritance children > regardless of whether the children already have the matching > constraint: To be clear- I wasn't thinking about what's possible today with inheiritance or partitioning or in PG at all, but rather focusing on what a user is likely to expect and understand from the messaging. > rhaas=# create table foo (a int, b text); > CREATE TABLE > rhaas=# create table bar () inherits (foo); > CREATE TABLE > rhaas=# alter table only foo add check (a = 1); > ERROR: constraint must be added to child tables too > rhaas=# alter table only bar add check (a = 1); > ALTER TABLE > rhaas=# alter table only foo add check (a = 1); > ERROR: constraint must be added to child tables too If that's the case then we should really change that error message, in my view. I'd be happier if we did support adding the constraint after child tables exist, but if we don't, we shouldn't imply to the user that they can add it after adding it to the children. > So, regarding Amit's 0001: > > - I think we should update the relevant hunk of the documentation > rather than just removing it. Alright. > - Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo > .. to work on a partitioned table without partitions, or is that just > pointless tinkering? That seems to be the only case where, after this > patch, an ONLY operation will fail on a childless partitioned table. I'm less sure about TRUNCATE ONLY because that really isn't meaningful at all, is it? A partitioned table doesn't have any data to truncate and truncating it doesn't have any impact on what happens later, does it? Having a sensible error be reported if someone tries to do that would be good though. > Do you want to be responsible for committing these or should I do it? Sure, though I won't be able to today and I've got some doubts about the other patches. I'll have more time tomorrow though. Thanks! Stephen
On Thu, Apr 13, 2017 at 11:05 AM, Stephen Frost <sfrost@snowman.net> wrote: > Sure, though I won't be able to today and I've got some doubts about the > other patches. I'll have more time tomorrow though. OK, cool. I'll mark you down as the owner on the open items list. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi Stephen, On 2017/04/14 0:05, Stephen Frost wrote: > Robert, > > * Robert Haas (robertmhaas@gmail.com) wrote: >> So I think I was indeed confused before, and I think you're basically >> right here, but on one point I think you are not right -- ALTER TABLE >> ONLY .. CHECK () doesn't work on a table with inheritance children >> regardless of whether the children already have the matching >> constraint: > > To be clear- I wasn't thinking about what's possible today with > inheiritance or partitioning or in PG at all, but rather focusing on > what a user is likely to expect and understand from the messaging. > >> rhaas=# create table foo (a int, b text); >> CREATE TABLE >> rhaas=# create table bar () inherits (foo); >> CREATE TABLE >> rhaas=# alter table only foo add check (a = 1); >> ERROR: constraint must be added to child tables too >> rhaas=# alter table only bar add check (a = 1); >> ALTER TABLE >> rhaas=# alter table only foo add check (a = 1); >> ERROR: constraint must be added to child tables too > > If that's the case then we should really change that error message, in > my view. I'd be happier if we did support adding the constraint after > child tables exist, Do you mean to use ONLY even if the child tables exist and still succeed? You can succeed in that case only by specifying NO INHERIT against the constraint with traditional inheritance. It does not work however with partitioned tables, since we do not allow NO INHERIT constraints to be defined on the partitioned tables; such a constraint would never be enforced if we had allowed that and just sit there. In the traditional inheritance case, it is applied to the parent's own rows. > but if we don't, we shouldn't imply to the user that > they can add it after adding it to the children. Hmm, the error message as it is *might* give the impression that we are asking a user to go add the constraint to the child tables first. Another way the user might interpret the message is that it is asking to drop the ONLY from the command (at least if they have read the documentation of ONLY on the ALTER TABLE reference page). But it perhaps isn't readily apparent from the error message itself, so maybe a HINT message along those lines might work. >> So, regarding Amit's 0001: >> >> - I think we should update the relevant hunk of the documentation >> rather than just removing it. > > Alright. You may have seen that the latest patch has taken care of this. But maybe you'll want to tweak my rewording further as you see fit. >> - Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo >> .. to work on a partitioned table without partitions, or is that just >> pointless tinkering? That seems to be the only case where, after this >> patch, an ONLY operation will fail on a childless partitioned table. > > I'm less sure about TRUNCATE ONLY because that really isn't meaningful > at all, is it? A partitioned table doesn't have any data to truncate > and truncating it doesn't have any impact on what happens later, does > it? That's right. If you perform truncate (or truncate only) on an empty partitioned table (i.e. with no partitions yet), it's essentially a no-op.Nor does it affect anything in the future. > Having a sensible error be reported if someone tries to do that > would be good though. The latest patch implements the following: -- create an empty partitioned table, aka without partitions create table p (a int) partition by list (a); -- no error, though nothing really happens truncate only p; TRUNCATE TABLE -- add a partition create table p1 partition of p for values in (1); -- error, because a partition exists truncate only p; ERROR: must truncate partitions too -- ok, partition p1 will be truncated truncate p; TRUNCATE TABLE I do now wonder if the error message "must truncate partitions *too*" is somewhat inappropriate here. The "too" seems to imply that table specified in the command (which is partitioned) is somehow truncate-able, which it is not. Hmm. >> Do you want to be responsible for committing these or should I do it? > > Sure, though I won't be able to today and I've got some doubts about the > other patches. I'll have more time tomorrow though. Thanks, I'll revise the patches per any review comments you might have. Regards, Amit
On Thu, Apr 13, 2017 at 11:38:08AM -0400, Robert Haas wrote: > On Thu, Apr 13, 2017 at 11:05 AM, Stephen Frost <sfrost@snowman.net> wrote: > > Sure, though I won't be able to today and I've got some doubts about the > > other patches. I'll have more time tomorrow though. > > OK, cool. I'll mark you down as the owner on the open items list. [Action required within three days.] The above-described topic is currently a PostgreSQL 10 open item. Stephen, as its owner, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
Amit, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > OK, I agree. I tweaked the existing bullet point about differences from > traditional inheritance when using ONLY with partitioned tables. Please take a look at the attached and let me know your thoughts on it. I changed the code to complain again regarding TRUNCATE ONLY, since that never actually makes sense on a partitioned table, unlike ALTER TABLE ONLY, and adjusted the error messages and added hints. > Updated patches attached (0002 and 0003 unchanged). Regarding these, 0002 changes pg_dump to handle partitions much more like inheritance, which at least seems like a decent idea but makes me a bit concerned about making sure we're doing everything correctly. In particular, we should really have regression tests for non-inherited CHECK (and NOT NULL) constraints on partitions. Perhaps there are tests covering those cases in the standard regression suite, but it's not obvious from the SQL. Thanks! Stephen
Attachment
Noah, all, * Noah Misch (noah@leadboat.com) wrote: > On Thu, Apr 13, 2017 at 11:38:08AM -0400, Robert Haas wrote: > > On Thu, Apr 13, 2017 at 11:05 AM, Stephen Frost <sfrost@snowman.net> wrote: > > > Sure, though I won't be able to today and I've got some doubts about the > > > other patches. I'll have more time tomorrow though. > > > > OK, cool. I'll mark you down as the owner on the open items list. > > [Action required within three days.] I've put up a new patch for review on the thread and plan to commit that tomorrow, assuming there isn't anything further. That should resolve the immediate issue, but I do think there's some merit to Amit's follow-on patches and will continue to discuss those and may commit one or both of those later this week. Thanks! Stephen
Hi Stephen,
On 2017/04/18 1:43, Stephen Frost wrote:
> Amit,
>
> * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
>> OK, I agree. I tweaked the existing bullet point about differences from
>> traditional inheritance when using ONLY with partitioned tables.
>
> Please take a look at the attached and let me know your thoughts on it.
> I changed the code to complain again regarding TRUNCATE ONLY, since that
> never actually makes sense on a partitioned table, unlike ALTER TABLE
> ONLY, and adjusted the error messages and added hints.
Thanks for polishing the patch. It looks mostly good to me.
+ Once
+ partitions exist, we do not support using <literal>ONLY</literal> to
+ add or drop constraints on only the partitioned table.
I wonder if the following sounds a bit more informative: Once partitions
exist, using <literal>ONLY</literal> will result in an error, because the
constraint being added or dropped must be added or dropped from the
partitions too.
>> Updated patches attached (0002 and 0003 unchanged).
>
> Regarding these, 0002 changes pg_dump to handle partitions much more
> like inheritance, which at least seems like a decent idea but makes me a
> bit concerned about making sure we're doing everything correctly.
Are you concerned about the correctness of the code in pg_dump or backend?
Rules of inheritance enforced by flagInhAttrs() are equally applicable to
the partitioning case, so actions performed by it for the partitioning
cases will not emit incorrect text.
The rule that backend follows is this: if a constraint is added to the
partitioned table (either a named check constraint or a NOT NULL
constraint), that constraint becomes an inherited *non-local* constraint
in all the partitions no matter if it was present in the partitions before
or not.
> In
> particular, we should really have regression tests for non-inherited
> CHECK (and NOT NULL) constraints on partitions. Perhaps there are tests
> covering those cases in the standard regression suite, but it's not
> obvious from the SQL.
There is one in create_table.sql that looks like this:
CREATE TABLE part_b PARTITION OF parted (
b NOT NULL DEFAULT 1 CHECK (b >= 0),
CONSTRAINT check_a CHECK (length(a) > 0)
) FOR VALUES IN ('b');
-- conislocal should be false for any merged constraints
SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid =
'part_b'::regclass AND conname = 'check_a';
But it doesn't really look like it's testing non-inherited constraints a
whole lot (CHECK (b >= 0) above is a local non-inherited constraint).
I added a few more tests right below the above test so that there is a bit
more coverage. Keep the rule I mentioned above when looking at these. I
also added a test in the pg_dump suite. That's in patch 0001 (since you
posted your version of what was 0001 before, I'm no longer including it here).
By the way, 0003 is a bug-fix. WITH OPTIONS that is emitted currently
like below is not the acceptable syntax:
CREATE TABLE a_partition OF a_partitioned_table (
a WITH OPTIONS NOT NULL DEFAULT 1
) FOR VALUES blah
But looking at the pg_dump code closely and also considering our
discussion upthread, I don't think this form is ever emitted. Because we
never emit a partition's column-level constraints and column defaults in
the CREATE TABLE, but rather separately using ALTER TABLE. In any case,
applying 0003 seems to be the right thing to do anyway, in case we might
rejigger things so that whatever can be emitted within the CREATE TABLE
text will be.
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
Attachment
On Mon, Apr 17, 2017 at 03:41:25PM -0400, Stephen Frost wrote: > * Noah Misch (noah@leadboat.com) wrote: > > On Thu, Apr 13, 2017 at 11:38:08AM -0400, Robert Haas wrote: > > > On Thu, Apr 13, 2017 at 11:05 AM, Stephen Frost <sfrost@snowman.net> wrote: > > > > Sure, though I won't be able to today and I've got some doubts about the > > > > other patches. I'll have more time tomorrow though. > > > > > > OK, cool. I'll mark you down as the owner on the open items list. > > > > [Action required within three days.] > > I've put up a new patch for review on the thread and plan to commit > that tomorrow, assuming there isn't anything further. That should > resolve the immediate issue, but I do think there's some merit to Amit's > follow-on patches and will continue to discuss those and may commit one > or both of those later this week. This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
Amit,
* Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
> On 2017/04/18 1:43, Stephen Frost wrote:
> > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
> >> OK, I agree. I tweaked the existing bullet point about differences from
> >> traditional inheritance when using ONLY with partitioned tables.
> >
> > Please take a look at the attached and let me know your thoughts on it.
> > I changed the code to complain again regarding TRUNCATE ONLY, since that
> > never actually makes sense on a partitioned table, unlike ALTER TABLE
> > ONLY, and adjusted the error messages and added hints.
>
> Thanks for polishing the patch. It looks mostly good to me.
>
> + Once
> + partitions exist, we do not support using <literal>ONLY</literal> to
> + add or drop constraints on only the partitioned table.
>
> I wonder if the following sounds a bit more informative: Once partitions
> exist, using <literal>ONLY</literal> will result in an error, because the
> constraint being added or dropped must be added or dropped from the
> partitions too.
My thinking here is that we may add support later on down the line for
using the ONLY keyword, when the constraint has already been created on
the partitions themselves, so I would rather just clarify that we don't
(yet) support that. Your wording, at least to me, seems to imply that
if the constraint was added to or dropped from the partitions then the
ONLY keyword could be used, which isn't accurate today.
> >> Updated patches attached (0002 and 0003 unchanged).
> >
> > Regarding these, 0002 changes pg_dump to handle partitions much more
> > like inheritance, which at least seems like a decent idea but makes me a
> > bit concerned about making sure we're doing everything correctly.
>
> Are you concerned about the correctness of the code in pg_dump or backend?
My concern is with regard to pg_dump in this case, primairly.
> Rules of inheritance enforced by flagInhAttrs() are equally applicable to
> the partitioning case, so actions performed by it for the partitioning
> cases will not emit incorrect text.
I understand that this *should* be the case, but that's not how the code
in pg_dump was written originally when it came to native partitions,
having its own flagPartitions() function in fact, which makes me
hesitate to start assuming what we do for inheritance is going to work
in these cases the same.
> The rule that backend follows is this: if a constraint is added to the
> partitioned table (either a named check constraint or a NOT NULL
> constraint), that constraint becomes an inherited *non-local* constraint
> in all the partitions no matter if it was present in the partitions before
> or not.
Right, I get that.
> > In
> > particular, we should really have regression tests for non-inherited
> > CHECK (and NOT NULL) constraints on partitions. Perhaps there are tests
> > covering those cases in the standard regression suite, but it's not
> > obvious from the SQL.
>
> There is one in create_table.sql that looks like this:
>
> CREATE TABLE part_b PARTITION OF parted (
> b NOT NULL DEFAULT 1 CHECK (b >= 0),
> CONSTRAINT check_a CHECK (length(a) > 0)
> ) FOR VALUES IN ('b');
> -- conislocal should be false for any merged constraints
> SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid =
> 'part_b'::regclass AND conname = 'check_a';
>
> But it doesn't really look like it's testing non-inherited constraints a
> whole lot (CHECK (b >= 0) above is a local non-inherited constraint).
>
> I added a few more tests right below the above test so that there is a bit
> more coverage. Keep the rule I mentioned above when looking at these. I
> also added a test in the pg_dump suite. That's in patch 0001 (since you
> posted your version of what was 0001 before, I'm no longer including it here).
Right, I saw the additional tests in your original patch, though it
DROP'd at least parted_no_parts, meaning that it wasn't being tested as
part of pg_upgrade/pg_dump testing.
> By the way, 0003 is a bug-fix. WITH OPTIONS that is emitted currently
> like below is not the acceptable syntax:
>
> CREATE TABLE a_partition OF a_partitioned_table (
> a WITH OPTIONS NOT NULL DEFAULT 1
> ) FOR VALUES blah
>
> But looking at the pg_dump code closely and also considering our
> discussion upthread, I don't think this form is ever emitted. Because we
> never emit a partition's column-level constraints and column defaults in
> the CREATE TABLE, but rather separately using ALTER TABLE. In any case,
> applying 0003 seems to be the right thing to do anyway, in case we might
> rejigger things so that whatever can be emitted within the CREATE TABLE
> text will be.
Hm, ok.
I'm going over your latest set of patches.
Thanks!
Stephen
Greetings, * Noah Misch (noah@leadboat.com) wrote: > On Mon, Apr 17, 2017 at 03:41:25PM -0400, Stephen Frost wrote: > > I've put up a new patch for review on the thread and plan to commit > > that tomorrow, assuming there isn't anything further. That should > > resolve the immediate issue, but I do think there's some merit to Amit's > > follow-on patches and will continue to discuss those and may commit one > > or both of those later this week. > > This PostgreSQL 10 open item is past due for your status update. Kindly send > a status update within 24 hours, and include a date for your subsequent status > update. Refer to the policy on open item ownership: > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com I thought I'd be able to get to this today, but other activities have taken up the time I had planned to work on this. I'll be on it again tomorrow morning and will provide an update (most likely a couple of commits) by COB tomorrow. Thanks! Stephen
Hi Stephen,
On 2017/04/21 8:43, Stephen Frost wrote:
> * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
>> On 2017/04/18 1:43, Stephen Frost wrote:
>>> Please take a look at the attached and let me know your thoughts on it.
>>> I changed the code to complain again regarding TRUNCATE ONLY, since that
>>> never actually makes sense on a partitioned table, unlike ALTER TABLE
>>> ONLY, and adjusted the error messages and added hints.
>>
>> Thanks for polishing the patch. It looks mostly good to me.
>>
>> + Once
>> + partitions exist, we do not support using <literal>ONLY</literal> to
>> + add or drop constraints on only the partitioned table.
>>
>> I wonder if the following sounds a bit more informative: Once partitions
>> exist, using <literal>ONLY</literal> will result in an error, because the
>> constraint being added or dropped must be added or dropped from the
>> partitions too.
>
> My thinking here is that we may add support later on down the line for
> using the ONLY keyword, when the constraint has already been created on
> the partitions themselves, so I would rather just clarify that we don't
> (yet) support that.
OK, I wanted to word it like that, because I have been thinking that we
will never support that.
So right now, we do not support ONLY (or how I tend to read it: cause
error on specifying ONLY) if partitions exist at all. In the future, we
will not output error until we have also seen that some partition does not
possess the constraint being added. IOW, specifying ONLY won't cause an
error even with non-zero partitions if all of them have the constraint
already defined. Maybe that's something we will support, but I am not
sure how many users will want to do things that way instead of the other
way around; I mean if the constraint is to be enforced on the whole
partitioned table anyway, why not just define it on the partitioned table
in the first place. But then again, why restrict users in many number of
ways.
So alright, I'm fine with the wording. If someone complains later that
they want more clarification on that point, it could be done later.
> Your wording, at least to me, seems to imply that
> if the constraint was added to or dropped from the partitions then the
> ONLY keyword could be used, which isn't accurate today.
Yes, that's likely to be the impression.
>>>> Updated patches attached (0002 and 0003 unchanged).
>>>
>>> Regarding these, 0002 changes pg_dump to handle partitions much more
>>> like inheritance, which at least seems like a decent idea but makes me a
>>> bit concerned about making sure we're doing everything correctly.
>>
>> Are you concerned about the correctness of the code in pg_dump or backend?
>
> My concern is with regard to pg_dump in this case, primairly.
>
>> Rules of inheritance enforced by flagInhAttrs() are equally applicable to
>> the partitioning case, so actions performed by it for the partitioning
>> cases will not emit incorrect text.
>
> I understand that this *should* be the case, but that's not how the code
> in pg_dump was written originally when it came to native partitions,
> having its own flagPartitions() function in fact, which makes me
> hesitate to start assuming what we do for inheritance is going to work
> in these cases the same.
I think why getPartitions() is separate from getInherits() and then
flagPartitions() separate from flagInhTables() is because I thought
originally that mixing the two would be undesirable. In the partitioning
case, getPartitions() joins pg_inherits with pg_class so that it can also
get hold of relpartbound, which I then thought would be a bad idea to do
for all of the inheritance tables. If we're OK with always doing the
join, I don't see why we couldn't get rid of the separation.
flagInhAttrs(), OTOH, seems unaffected by that concern and I think using
it for both inheritance and partitioning is fine. It affects NOT NULL
constraints and defaults, which as far as it goes, applies to both
inheritance and partitioning the same.
>>> In
>>> particular, we should really have regression tests for non-inherited
>>> CHECK (and NOT NULL) constraints on partitions. Perhaps there are tests
>>> covering those cases in the standard regression suite, but it's not
>>> obvious from the SQL.
>>
>> There is one in create_table.sql that looks like this:
>>
>> CREATE TABLE part_b PARTITION OF parted (
>> b NOT NULL DEFAULT 1 CHECK (b >= 0),
>> CONSTRAINT check_a CHECK (length(a) > 0)
>> ) FOR VALUES IN ('b');
>> -- conislocal should be false for any merged constraints
>> SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid =
>> 'part_b'::regclass AND conname = 'check_a';
>>
>> But it doesn't really look like it's testing non-inherited constraints a
>> whole lot (CHECK (b >= 0) above is a local non-inherited constraint).
>>
>> I added a few more tests right below the above test so that there is a bit
>> more coverage. Keep the rule I mentioned above when looking at these. I
>> also added a test in the pg_dump suite. That's in patch 0001 (since you
>> posted your version of what was 0001 before, I'm no longer including it here).
>
> Right, I saw the additional tests in your original patch, though it
> DROP'd at least parted_no_parts, meaning that it wasn't being tested as
> part of pg_upgrade/pg_dump testing.
So, the newly added tests seem enough for now?
>> By the way, 0003 is a bug-fix. WITH OPTIONS that is emitted currently
>> like below is not the acceptable syntax:
>>
>> CREATE TABLE a_partition OF a_partitioned_table (
>> a WITH OPTIONS NOT NULL DEFAULT 1
>> ) FOR VALUES blah
>>
>> But looking at the pg_dump code closely and also considering our
>> discussion upthread, I don't think this form is ever emitted. Because we
>> never emit a partition's column-level constraints and column defaults in
>> the CREATE TABLE, but rather separately using ALTER TABLE. In any case,
>> applying 0003 seems to be the right thing to do anyway, in case we might
>> rejigger things so that whatever can be emitted within the CREATE TABLE
>> text will be.
>
> Hm, ok.
>
> I'm going over your latest set of patches.
Thanks.
Regards,
Amit
On Fri, Apr 21, 2017 at 1:43 AM, Stephen Frost <sfrost@snowman.net> wrote: >> + Once >> + partitions exist, we do not support using <literal>ONLY</literal> to >> + add or drop constraints on only the partitioned table. >> >> I wonder if the following sounds a bit more informative: Once partitions >> exist, using <literal>ONLY</literal> will result in an error, because the >> constraint being added or dropped must be added or dropped from the >> partitions too. > > My thinking here is that we may add support later on down the line for > using the ONLY keyword, when the constraint has already been created on > the partitions themselves, so I would rather just clarify that we don't > (yet) support that. Your wording, at least to me, seems to imply that > if the constraint was added to or dropped from the partitions then the > ONLY keyword could be used, which isn't accurate today. Well, I think that Amit's wording has the virtue of giving a reason which is basically valid, whereas someone reading your text might conceivably be left wondering what the reason for the restriction is. Also, I think saying that it it will result in an error is a bit more helpful than saying that it is is not supported, since the latter might lead someone to believe that it could result in undefined behavior (i.e. arbitrarily awful misbehavior) which is not the case. So I like what he wrote, for whatever that's worth. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Apr 20, 2017 at 09:53:28PM -0400, Stephen Frost wrote: > * Noah Misch (noah@leadboat.com) wrote: > > On Mon, Apr 17, 2017 at 03:41:25PM -0400, Stephen Frost wrote: > > > I've put up a new patch for review on the thread and plan to commit > > > that tomorrow, assuming there isn't anything further. That should > > > resolve the immediate issue, but I do think there's some merit to Amit's > > > follow-on patches and will continue to discuss those and may commit one > > > or both of those later this week. > > > > This PostgreSQL 10 open item is past due for your status update. Kindly send > > a status update within 24 hours, and include a date for your subsequent status > > update. Refer to the policy on open item ownership: > > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > I thought I'd be able to get to this today, but other activities have > taken up the time I had planned to work on this. I'll be on it again > tomorrow morning and will provide an update (most likely a couple of > commits) by COB tomorrow. This PostgreSQL 10 open item is again past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On Sat, Apr 22, 2017 at 01:14:08PM -0700, Noah Misch wrote: > On Thu, Apr 20, 2017 at 09:53:28PM -0400, Stephen Frost wrote: > > * Noah Misch (noah@leadboat.com) wrote: > > > On Mon, Apr 17, 2017 at 03:41:25PM -0400, Stephen Frost wrote: > > > > I've put up a new patch for review on the thread and plan to commit > > > > that tomorrow, assuming there isn't anything further. That should > > > > resolve the immediate issue, but I do think there's some merit to Amit's > > > > follow-on patches and will continue to discuss those and may commit one > > > > or both of those later this week. > > > > > > This PostgreSQL 10 open item is past due for your status update. Kindly send > > > a status update within 24 hours, and include a date for your subsequent status > > > update. Refer to the policy on open item ownership: > > > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > I thought I'd be able to get to this today, but other activities have > > taken up the time I had planned to work on this. I'll be on it again > > tomorrow morning and will provide an update (most likely a couple of > > commits) by COB tomorrow. > > This PostgreSQL 10 open item is again past due for your status update. Kindly > send a status update within 24 hours, and include a date for your subsequent > status update. Refer to the policy on open item ownership: > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past due for your status update. Please reacquaint yourself with the policy on open item ownership[1] and then reply immediately. If I do not hear from you by 2017-04-25 00:00 UTC, I will transfer this item to release management team ownership without further notice. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
Noah, all,
On Sun, Apr 23, 2017 at 19:52 Noah Misch <noah@leadboat.com> wrote:
On Sat, Apr 22, 2017 at 01:14:08PM -0700, Noah Misch wrote:
> On Thu, Apr 20, 2017 at 09:53:28PM -0400, Stephen Frost wrote:
> > * Noah Misch (noah@leadboat.com) wrote:
> > > On Mon, Apr 17, 2017 at 03:41:25PM -0400, Stephen Frost wrote:
> > > > I've put up a new patch for review on the thread and plan to commit
> > > > that tomorrow, assuming there isn't anything further. That should
> > > > resolve the immediate issue, but I do think there's some merit to Amit's
> > > > follow-on patches and will continue to discuss those and may commit one
> > > > or both of those later this week.
> > >
> > > This PostgreSQL 10 open item is past due for your status update. Kindly send
> > > a status update within 24 hours, and include a date for your subsequent status
> > > update. Refer to the policy on open item ownership:
> > > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> >
> > I thought I'd be able to get to this today, but other activities have
> > taken up the time I had planned to work on this. I'll be on it again
> > tomorrow morning and will provide an update (most likely a couple of
> > commits) by COB tomorrow.
>
> This PostgreSQL 10 open item is again past due for your status update. Kindly
> send a status update within 24 hours, and include a date for your subsequent
> status update. Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past due
for your status update. Please reacquaint yourself with the policy on open
item ownership[1] and then reply immediately. If I do not hear from you by
2017-04-25 00:00 UTC, I will transfer this item to release management team
ownership without further notice.
The status is simply that I've been considering Robert's comments regarding the documentation and have had a busy weekend. I'll provide an update tomorrow.
Thanks!
Stephen
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Fri, Apr 21, 2017 at 1:43 AM, Stephen Frost <sfrost@snowman.net> wrote: > >> + Once > >> + partitions exist, we do not support using <literal>ONLY</literal> to > >> + add or drop constraints on only the partitioned table. > >> > >> I wonder if the following sounds a bit more informative: Once partitions > >> exist, using <literal>ONLY</literal> will result in an error, because the > >> constraint being added or dropped must be added or dropped from the > >> partitions too. > > > > My thinking here is that we may add support later on down the line for > > using the ONLY keyword, when the constraint has already been created on > > the partitions themselves, so I would rather just clarify that we don't > > (yet) support that. Your wording, at least to me, seems to imply that > > if the constraint was added to or dropped from the partitions then the > > ONLY keyword could be used, which isn't accurate today. > > Well, I think that Amit's wording has the virtue of giving a reason > which is basically valid, whereas someone reading your text might > conceivably be left wondering what the reason for the restriction is. I wonder why the restriction is there, which is probably part of the reason that I'm thinking of phrasing the documentation that way. Beyond a matter of round to-its, is there a reason why it couldn't (or shouldn't) be supported? I'm not suggesting now, of course, but in a future release. I could certainly see someone wanting to manage the process by which a constriant is added by adding it one at a time to the individual partitions and then wishing to be sure that adding it to parent only checked that the individual partitions had the constraint and then added it to the parent (in other words, a relatively short operation, providing the locks are able to be acquired quickly). > Also, I think saying that it it will result in an error is a bit more > helpful than saying that it is is not supported, since the latter > might lead someone to believe that it could result in undefined > behavior (i.e. arbitrarily awful misbehavior) which is not the case. > So I like what he wrote, for whatever that's worth. I don't mind stating that it'll result in an error, I just don't want to imply that there's some way to get around that error if things were done differently. How about: --- Once partitions exist, using <literal>ONLY</literal> will always result in an error as adding or dropping constraints on only the partitiioned table, when partitions exist, is not supported. --- Thanks! Stephen
On Mon, Apr 24, 2017 at 9:17 AM, Stephen Frost <sfrost@snowman.net> wrote: > I wonder why the restriction is there, which is probably part of the > reason that I'm thinking of phrasing the documentation that way. > > Beyond a matter of round to-its, is there a reason why it couldn't (or > shouldn't) be supported? I'm not suggesting now, of course, but in a > future release. I don't see any particular reason, but I haven't looked into the matter deeply. One thing to consider is that you can already more or less do exactly that thing, like this: rhaas=# create table foo (a int, b text) partition by range (a); CREATE TABLE Time: 4.738 ms rhaas=# create table foo1 partition of foo for values from (0) to (100); CREATE TABLE Time: 5.162 ms rhaas=# insert into foo1 select 1, 'snarf' from generate_series(1,10000000) g; INSERT 0 10000000 Time: 12835.153 ms (00:12.835) rhaas=# alter table foo1 add constraint xyz check (b != 'nope'); ALTER TABLE Time: 1059.728 ms (00:01.060) rhaas=# alter table foo add constraint xyz check (b != 'nope'); NOTICE: merging constraint "xyz" with inherited definition ALTER TABLE Time: 1.243 ms So we may not really need another way to do it. >> Also, I think saying that it it will result in an error is a bit more >> helpful than saying that it is is not supported, since the latter >> might lead someone to believe that it could result in undefined >> behavior (i.e. arbitrarily awful misbehavior) which is not the case. >> So I like what he wrote, for whatever that's worth. > > I don't mind stating that it'll result in an error, I just don't want to > imply that there's some way to get around that error if things were done > differently. > > How about: > > --- > Once partitions exist, using <literal>ONLY</literal> will always result > in an error as adding or dropping constraints on only the partitiioned > table, when partitions exist, is not supported. > --- I still prefer Amit's language, but it's not worth to me to keep arguing about it. If you prefer what you've written there, fine, but let's get something committed and move on. I think we understand what needs to be fixed here now, and I'd like to do get that done. If you don't want to do it or don't have time to do it, I'll pick it up again, but let's not let this keep dragging out. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Amit, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > I think why getPartitions() is separate from getInherits() and then > flagPartitions() separate from flagInhTables() is because I thought > originally that mixing the two would be undesirable. In the partitioning > case, getPartitions() joins pg_inherits with pg_class so that it can also > get hold of relpartbound, which I then thought would be a bad idea to do > for all of the inheritance tables. If we're OK with always doing the > join, I don't see why we couldn't get rid of the separation. I'm not sure what you mean here. We're always going to call both getInherits() and getPartitions() and run the queries in each, with the way the code is written today. In my experience working with pg_dump and trying to not slow it down, the last thing we want to do is run two independent queries when one will do. Further, we aren't really avoiding any work when we have to go look at pg_class to exclude the partitioned tables in getInherits() anyway. I'm not seeing why we really need the join at all though, see below. > flagInhAttrs(), OTOH, seems unaffected by that concern and I think using > it for both inheritance and partitioning is fine. It affects NOT NULL > constraints and defaults, which as far as it goes, applies to both > inheritance and partitioning the same. I don't see why we need to have getPartitions(), flagPartitions(), or findPartitonParentByOid(). They all seem to be largely copies of the inheritance functions, but it doesn't seem like that's really necessary or sensible. I also don't follow why we're pulling the partbound definition in getPartitions() instead of in getTables(), where we're already pulling the rest of the data from pg_class? Or why getTablePartitionKeyInfo() exists instead of also doing that during getTables()? I looked through pg_get_partkeydef() and it didn't seem to be particularly expensive to run, though evidently it doesn't handle being passed an OID that it doesn't expect very cleanly: =# select pg_get_partkeydef(oid) from pg_class; ERROR: cache lookup failed for partition key of 52333 I don't believe that's really very appropriate of it to do though and instead it should work the same way pg_get_indexdef() and others do and return NULL in such cases, so people can use it sanely. I'm working through this but it's going to take a bit of time to clean it up and it's not going to get finished today, but I do think it needs to get done and so I'll work to make it happen this week. I didn't anticipate finding this, obviously and am a bit disappointed by it. > So, the newly added tests seem enough for now? Considering the pg_upgrade regression test didn't pass with the patch as sent, I'd say that more tests are needed. I will be working to add those also. Thanks! Stephen
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Mon, Apr 24, 2017 at 9:17 AM, Stephen Frost <sfrost@snowman.net> wrote: > > I wonder why the restriction is there, which is probably part of the > > reason that I'm thinking of phrasing the documentation that way. > > > > Beyond a matter of round to-its, is there a reason why it couldn't (or > > shouldn't) be supported? I'm not suggesting now, of course, but in a > > future release. > > I don't see any particular reason, but I haven't looked into the > matter deeply. One thing to consider is that you can already more or > less do exactly that thing, like this: > > rhaas=# create table foo (a int, b text) partition by range (a); > CREATE TABLE > Time: 4.738 ms > rhaas=# create table foo1 partition of foo for values from (0) to (100); > CREATE TABLE > Time: 5.162 ms > rhaas=# insert into foo1 select 1, 'snarf' from generate_series(1,10000000) g; > INSERT 0 10000000 > Time: 12835.153 ms (00:12.835) > rhaas=# alter table foo1 add constraint xyz check (b != 'nope'); > ALTER TABLE > Time: 1059.728 ms (00:01.060) > rhaas=# alter table foo add constraint xyz check (b != 'nope'); > NOTICE: merging constraint "xyz" with inherited definition > ALTER TABLE > Time: 1.243 ms > > So we may not really need another way to do it. Interesting. Seems like the question is really what we mean by "ONLY" here. For my 2c, at least, if we can check that all of the partitions already have the constraint enforced, such that the only thing we're changing is the partitioned table, then that's exactly what "ONLY" should do. That would require a bit of rework, presumably, of how that keyword is handled but the basics are all there. > >> Also, I think saying that it it will result in an error is a bit more > >> helpful than saying that it is is not supported, since the latter > >> might lead someone to believe that it could result in undefined > >> behavior (i.e. arbitrarily awful misbehavior) which is not the case. > >> So I like what he wrote, for whatever that's worth. > > > > I don't mind stating that it'll result in an error, I just don't want to > > imply that there's some way to get around that error if things were done > > differently. > > > > How about: > > > > --- > > Once partitions exist, using <literal>ONLY</literal> will always result > > in an error as adding or dropping constraints on only the partitiioned > > table, when partitions exist, is not supported. > > --- > > I still prefer Amit's language, but it's not worth to me to keep > arguing about it. If you prefer what you've written there, fine, but > let's get something committed and move on. I think we understand what > needs to be fixed here now, and I'd like to do get that done. If you > don't want to do it or don't have time to do it, I'll pick it up > again, but let's not let this keep dragging out. I'm planning to push just this patch to make the backend accept the ALTER TABLE ONLY when no partitions exist later this afternoon, but the work here isn't done as noted in my other email. Thanks! Stephen
Attachment
On Tue, Apr 25, 2017 at 12:26 PM, Stephen Frost <sfrost@snowman.net> wrote: > Interesting. Seems like the question is really what we mean by "ONLY" > here. For my 2c, at least, if we can check that all of the partitions > already have the constraint enforced, such that the only thing we're > changing is the partitioned table, then that's exactly what "ONLY" > should do. That would require a bit of rework, presumably, of how that > keyword is handled but the basics are all there. Well, I'm not saying that couldn't be done, but it doesn't add any capability that we don't have today, so from my point of view it seems fairly pointless. However, we can leave that for another day; for now, I think we should focus on fixing the pg_dump bugs that this thread is about. > I'm planning to push just this patch to make the backend accept the > ALTER TABLE ONLY when no partitions exist later this afternoon, ... Cool. > but the > work here isn't done as noted in my other email. Right. I'd just like to get the work done as soon as we reasonably can. Tempus fugit, and all that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi Stephen,
On 2017/04/26 0:42, Stephen Frost wrote:
> Amit,
>
> * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
>> I think why getPartitions() is separate from getInherits() and then
>> flagPartitions() separate from flagInhTables() is because I thought
>> originally that mixing the two would be undesirable. In the partitioning
>> case, getPartitions() joins pg_inherits with pg_class so that it can also
>> get hold of relpartbound, which I then thought would be a bad idea to do
>> for all of the inheritance tables. If we're OK with always doing the
>> join, I don't see why we couldn't get rid of the separation.
>
> I'm not sure what you mean here. We're always going to call both
> getInherits() and getPartitions() and run the queries in each, with the
> way the code is written today. In my experience working with pg_dump
> and trying to not slow it down, the last thing we want to do is run two
> independent queries when one will do. Further, we aren't really
> avoiding any work when we have to go look at pg_class to exclude the
> partitioned tables in getInherits() anyway. I'm not seeing why we
> really need the join at all though, see below.
You're right and I realize that we don't need lots of new code for pg_dump
to retrieve the partitioning information (including the parent-child
relationships). I propose some patches below, one per each item you
discovered could be improved.
>> flagInhAttrs(), OTOH, seems unaffected by that concern and I think using
>> it for both inheritance and partitioning is fine. It affects NOT NULL
>> constraints and defaults, which as far as it goes, applies to both
>> inheritance and partitioning the same.
>
> I don't see why we need to have getPartitions(), flagPartitions(), or
> findPartitonParentByOid(). They all seem to be largely copies of the
> inheritance functions, but it doesn't seem like that's really necessary
> or sensible.
>
> I also don't follow why we're pulling the partbound definition in
> getPartitions() instead of in getTables(), where we're already pulling
> the rest of the data from pg_class? Or why getTablePartitionKeyInfo()
> exists instead of also doing that during getTables()?
I think it had to do with wanting to avoid creating yet another copy of
the big getTables() query for >= v10, back when the original patch was
written, but doing that is not really needed.
Attached patch 0004 gets rid of that separation. It removes the struct
PartInfo, functions flagPartitions(), findPartitionParentByOid(),
getPartitions(), and getTablePartitionKeyInfo(). All necessary
partitioning-specific information is retrieved in getTables() itself.
Also, now that partitioning uses the old inheritance code, inherited NOT
NULL constraints need not be emitted separately per child. The
now-removed code that separately retrieved partitioning inheritance
information was implemented such that partition attributes didn't receive
the flagInhAttrs() treatment.
> I looked through
> pg_get_partkeydef() and it didn't seem to be particularly expensive to
> run, though evidently it doesn't handle being passed an OID that it
> doesn't expect very cleanly:
>
> =# select pg_get_partkeydef(oid) from pg_class;
> ERROR: cache lookup failed for partition key of 52333
>
> I don't believe that's really very appropriate of it to do though and
> instead it should work the same way pg_get_indexdef() and others do and
> return NULL in such cases, so people can use it sanely.
>
> I'm working through this but it's going to take a bit of time to clean
> it up and it's not going to get finished today, but I do think it needs
> to get done and so I'll work to make it happen this week. I didn't
> anticipate finding this, obviously and am a bit disappointed by it.
Yeah, that was sloppy. :-(
Attached patch 0005 fixes that and adds a test to rules.sql.
>> So, the newly added tests seem enough for now?
>
> Considering the pg_upgrade regression test didn't pass with the patch
> as sent, I'd say that more tests are needed. I will be working to add
> those also.
I think I have found an oversight in the pg_dump's --binary-upgrade code
that might have caused the error you saw (I proceeded to fix it after
seeing the error that I saw). Fix is included as patch 0003.
So to summarize what the patches do (some of these were posted earlier)
0001: Improve test coverage of partition constraints (non-inherited ones)
0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns
0003: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
INHERIT to be emitted to attach a partition instead of ALTER TABLE
ATTACH PARTITION
0004: Change the way pg_dump retrieves partitioning info (removes a lot
of unnecessary code and instead makes partitioning case use the old
inheritance code, etc.)
0005: Teach pg_get_partkeydef_worker to deal with OIDs it can't handle
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
Attachment
Amit,
* Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
> On 2017/04/26 0:42, Stephen Frost wrote:
> > I'm not sure what you mean here. We're always going to call both
> > getInherits() and getPartitions() and run the queries in each, with the
> > way the code is written today. In my experience working with pg_dump
> > and trying to not slow it down, the last thing we want to do is run two
> > independent queries when one will do. Further, we aren't really
> > avoiding any work when we have to go look at pg_class to exclude the
> > partitioned tables in getInherits() anyway. I'm not seeing why we
> > really need the join at all though, see below.
>
> You're right and I realize that we don't need lots of new code for pg_dump
> to retrieve the partitioning information (including the parent-child
> relationships). I propose some patches below, one per each item you
> discovered could be improved.
Thanks!
> Attached patch 0004 gets rid of that separation. It removes the struct
> PartInfo, functions flagPartitions(), findPartitionParentByOid(),
> getPartitions(), and getTablePartitionKeyInfo(). All necessary
> partitioning-specific information is retrieved in getTables() itself.
That certainly looks better.
> Also, now that partitioning uses the old inheritance code, inherited NOT
> NULL constraints need not be emitted separately per child. The
> now-removed code that separately retrieved partitioning inheritance
> information was implemented such that partition attributes didn't receive
> the flagInhAttrs() treatment.
Right.
> > I looked through
> > pg_get_partkeydef() and it didn't seem to be particularly expensive to
> > run, though evidently it doesn't handle being passed an OID that it
> > doesn't expect very cleanly:
> >
> > =# select pg_get_partkeydef(oid) from pg_class;
> > ERROR: cache lookup failed for partition key of 52333
> >
> > I don't believe that's really very appropriate of it to do though and
> > instead it should work the same way pg_get_indexdef() and others do and
> > return NULL in such cases, so people can use it sanely.
> >
> > I'm working through this but it's going to take a bit of time to clean
> > it up and it's not going to get finished today, but I do think it needs
> > to get done and so I'll work to make it happen this week. I didn't
> > anticipate finding this, obviously and am a bit disappointed by it.
>
> Yeah, that was sloppy. :-(
>
> Attached patch 0005 fixes that and adds a test to rules.sql.
Good, I'll commit that first, since it will make things simpler for
pg_dump.
> I think I have found an oversight in the pg_dump's --binary-upgrade code
> that might have caused the error you saw (I proceeded to fix it after
> seeing the error that I saw). Fix is included as patch 0003.
Yeah, that was what I saw also.
> So to summarize what the patches do (some of these were posted earlier)
>
> 0001: Improve test coverage of partition constraints (non-inherited ones)
Looks fine.
> 0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns
I'm trying to understand why this is also different. At least on an
initial look, there seems to be a bunch more copy-and-paste from the
existing TypedTable to Partition in gram.y, which seems to all boil down
to removing the need for WITH OPTIONS to be specified. If WITH OPTIONS
would be too much to have included for partitioning, and it isn't
actually necessary, why not just make it optional, and use the same
construct for both, removing all the duplicaiton and the need for
pg_dump to output it?
> 0003: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
> INHERIT to be emitted to attach a partition instead of ALTER TABLE
> ATTACH PARTITION
Well, worse, it was dumping out both, the INHERITs and the ATTACH
PARTITION. Given that we're now setting numParents for partitions, I
would think we'd just move the if (tbinfo->partitionOf) branches up
under the if (numParents > 0) branches, which I think would also help us
catch additional issues, like the fact that a binary-upgrade with
partitions in a different namespace from the partitioned table would
fail because the ATTACH PARTITION code doesn't account for it:
appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
fmtId(tbinfo->partitionOf->dobj.name)); appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n",
fmtId(tbinfo->dobj.name), tbinfo->partitiondef);
unlike the existing inheritance code a few lines above, which does:
appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ",
fmtId(tbinfo->dobj.name)); if (parentRel->dobj.namespace != tbinfo->dobj.namespace)
appendPQExpBuffer(q, "%s.", fmtId(parentRel->dobj.namespace->dobj.name));
appendPQExpBuffer(q, "%s;\n", fmtId(parentRel->dobj.name));
> 0004: Change the way pg_dump retrieves partitioning info (removes a lot
> of unnecessary code and instead makes partitioning case use the old
> inheritance code, etc.)
This looks pretty good, at least on first blush, probably in part
because it's mostly removing code. The CASE statement in the
getTables() query isn't needed, once pg_get_partkeydef() has been
changed to not throw an ERROR when passed a non-partitioned table OID.
> 0005: Teach pg_get_partkeydef_worker to deal with OIDs it can't handle
Looks fine, will commit this one later today.
Thanks!
Stephen
Hi Stephen,
On 2017/04/26 23:31, Stephen Frost wrote:
>>> I looked through
>>> pg_get_partkeydef() and it didn't seem to be particularly expensive to
>>> run, though evidently it doesn't handle being passed an OID that it
>>> doesn't expect very cleanly:
>>>
>>> =# select pg_get_partkeydef(oid) from pg_class;
>>> ERROR: cache lookup failed for partition key of 52333
>>>
>>> I don't believe that's really very appropriate of it to do though and
>>> instead it should work the same way pg_get_indexdef() and others do and
>>> return NULL in such cases, so people can use it sanely.
>>>
>>> I'm working through this but it's going to take a bit of time to clean
>>> it up and it's not going to get finished today, but I do think it needs
>>> to get done and so I'll work to make it happen this week. I didn't
>>> anticipate finding this, obviously and am a bit disappointed by it.
>>
>> Yeah, that was sloppy. :-(
>>
>> Attached patch 0005 fixes that and adds a test to rules.sql.
>
> Good, I'll commit that first, since it will make things simpler for
> pg_dump.
Thanks for committing this one.
>> So to summarize what the patches do (some of these were posted earlier)
>>
>> 0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns
>
> I'm trying to understand why this is also different. At least on an
> initial look, there seems to be a bunch more copy-and-paste from the
> existing TypedTable to Partition in gram.y, which seems to all boil down
> to removing the need for WITH OPTIONS to be specified. If WITH OPTIONS
> would be too much to have included for partitioning, and it isn't
> actually necessary, why not just make it optional, and use the same
> construct for both, removing all the duplicaiton and the need for
> pg_dump to output it?
OK, I think optionally allowing WITH OPTIONS keywords would be nice.
So in lieu of this patch, I propose a patch that modifies gram.y to allow
WITH OPTIONS to specified.
>> 0003: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
>> INHERIT to be emitted to attach a partition instead of ALTER TABLE
>> ATTACH PARTITION
>
> Well, worse, it was dumping out both, the INHERITs and the ATTACH
> PARTITION.
Oops, you're right. Actually meant to say that.
> Given that we're now setting numParents for partitions, I
> would think we'd just move the if (tbinfo->partitionOf) branches up
> under the if (numParents > 0) branches, which I think would also help us
> catch additional issues, like the fact that a binary-upgrade with
> partitions in a different namespace from the partitioned table would
> fail because the ATTACH PARTITION code doesn't account for it:
>
> appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
> fmtId(tbinfo->partitionOf->dobj.name));
> appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n",
> fmtId(tbinfo->dobj.name),
> tbinfo->partitiondef);
>
> unlike the existing inheritance code a few lines above, which does:
>
> appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ",
> fmtId(tbinfo->dobj.name));
> if (parentRel->dobj.namespace != tbinfo->dobj.namespace)
> appendPQExpBuffer(q, "%s.",
> fmtId(parentRel->dobj.namespace->dobj.name));
> appendPQExpBuffer(q, "%s;\n",
> fmtId(parentRel->dobj.name));
That's a good point. I put both cases under the if (numParents > 0) block
and appropriate text is emitted depending on whether the table is a
partition or plain inheritance child.
>> 0004: Change the way pg_dump retrieves partitioning info (removes a lot
>> of unnecessary code and instead makes partitioning case use the old
>> inheritance code, etc.)
>
> This looks pretty good, at least on first blush, probably in part
> because it's mostly removing code. The CASE statement in the
> getTables() query isn't needed, once pg_get_partkeydef() has been
> changed to not throw an ERROR when passed a non-partitioned table OID.
Oh yes, fixed.
I put the patches in different order this time so that the refactoring
patch appears before the --binary-upgrade bug-fix patch (0003 and 0004
have reversed their roles.) Also, 0002 is no longer a pg_dump fix.
0001: Improve test coverage of partition constraints (non-inherited ones)
0002: Allow partition columns to optionally include WITH OPTIONS keywords
0003: Change the way pg_dump retrieves partitioning info (removes a lot
of unnecessary code and instead makes partitioning case use the old
inheritance code, etc.)
0004: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
INHERIT to be emitted to attach a partition in addition to of ALTER
TABLE ATTACH PARTITION and that no schema-name was emitted where it
should have
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
Attachment
Amit, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > >> So to summarize what the patches do (some of these were posted earlier) > >> > >> 0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns > > > > I'm trying to understand why this is also different. At least on an > > initial look, there seems to be a bunch more copy-and-paste from the > > existing TypedTable to Partition in gram.y, which seems to all boil down > > to removing the need for WITH OPTIONS to be specified. If WITH OPTIONS > > would be too much to have included for partitioning, and it isn't > > actually necessary, why not just make it optional, and use the same > > construct for both, removing all the duplicaiton and the need for > > pg_dump to output it? > > OK, I think optionally allowing WITH OPTIONS keywords would be nice. > > So in lieu of this patch, I propose a patch that modifies gram.y to allow > WITH OPTIONS to specified. The point I was trying to get at was that if you make WITH OPTIONS optional for the TypedTableElement case, you can remove all of the PartitionElement-related productions and then both the OF type_name and the PARTITION OF case will accept WITH OPTIONS as noise words and also work without WITH OPTIONS being specified. This also makes the code actually match the documentation since, at least in the CREATE FOREIGN TABLE documentation, we claim that WITH OPTIONS is required. Please see a proposed patch attached to accomplish this. > > Given that we're now setting numParents for partitions, I > > would think we'd just move the if (tbinfo->partitionOf) branches up > > under the if (numParents > 0) branches, which I think would also help us > > catch additional issues, like the fact that a binary-upgrade with > > partitions in a different namespace from the partitioned table would > > fail because the ATTACH PARTITION code doesn't account for it: > > > > appendPQExpBuffer(q, "ALTER TABLE ONLY %s ", > > fmtId(tbinfo->partitionOf->dobj.name)); > > appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n", > > fmtId(tbinfo->dobj.name), > > tbinfo->partitiondef); > > > > unlike the existing inheritance code a few lines above, which does: > > > > appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ", > > fmtId(tbinfo->dobj.name)); > > if (parentRel->dobj.namespace != tbinfo->dobj.namespace) > > appendPQExpBuffer(q, "%s.", > > fmtId(parentRel->dobj.namespace->dobj.name)); > > appendPQExpBuffer(q, "%s;\n", > > fmtId(parentRel->dobj.name)); > > That's a good point. I put both cases under the if (numParents > 0) block > and appropriate text is emitted depending on whether the table is a > partition or plain inheritance child. Right, ok. > >> 0004: Change the way pg_dump retrieves partitioning info (removes a lot > >> of unnecessary code and instead makes partitioning case use the old > >> inheritance code, etc.) > > > > This looks pretty good, at least on first blush, probably in part > > because it's mostly removing code. The CASE statement in the > > getTables() query isn't needed, once pg_get_partkeydef() has been > > changed to not throw an ERROR when passed a non-partitioned table OID. > > Oh yes, fixed. Good. > 0003: Change the way pg_dump retrieves partitioning info (removes a lot > of unnecessary code and instead makes partitioning case use the old > inheritance code, etc.) This has conflicts due to my proposed patch as my patch changes pg_dump to not output the now-noise-words WITH OPTIONS at all. > 0004: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE > INHERIT to be emitted to attach a partition in addition to of ALTER > TABLE ATTACH PARTITION and that no schema-name was emitted where it > should have Given that it's touching the same places, this would really make sense to merge into 0003 now. I'm going to continue to mull over the attached for a bit and add some tests to it, but if it looks good then I'll push it this afternoon, after which you'll hopefully have time to rebase 0003 and merge in 0004 to that, which I can review and probably push tomorrow. Thanks! Stephen
Attachment
On Sun, Apr 23, 2017 at 11:58:23PM +0000, Stephen Frost wrote: > Noah, all, > > On Sun, Apr 23, 2017 at 19:52 Noah Misch <noah@leadboat.com> wrote: > > > On Sat, Apr 22, 2017 at 01:14:08PM -0700, Noah Misch wrote: > > > On Thu, Apr 20, 2017 at 09:53:28PM -0400, Stephen Frost wrote: > > > > * Noah Misch (noah@leadboat.com) wrote: > > > > > On Mon, Apr 17, 2017 at 03:41:25PM -0400, Stephen Frost wrote: > > > > > > I've put up a new patch for review on the thread and plan to commit > > > > > > that tomorrow, assuming there isn't anything further. That should > > > > > > resolve the immediate issue, but I do think there's some merit to > > Amit's > > > > > > follow-on patches and will continue to discuss those and may > > commit one > > > > > > or both of those later this week. > > > > > > > > > > This PostgreSQL 10 open item is past due for your status update. > > Kindly send > > > > > a status update within 24 hours, and include a date for your > > subsequent status > > > > > update. Refer to the policy on open item ownership: > > > > > > > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > > > > > I thought I'd be able to get to this today, but other activities have > > > > taken up the time I had planned to work on this. I'll be on it again > > > > tomorrow morning and will provide an update (most likely a couple of > > > > commits) by COB tomorrow. > > > > > > This PostgreSQL 10 open item is again past due for your status update. > > Kindly > > > send a status update within 24 hours, and include a date for your > > subsequent > > > status update. Refer to the policy on open item ownership: > > > > > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > > > IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past > > due > > for your status update. Please reacquaint yourself with the policy on open > > item ownership[1] and then reply immediately. If I do not hear from you by > > 2017-04-25 00:00 UTC, I will transfer this item to release management team > > ownership without further notice. > > > The status is simply that I've been considering Robert's comments regarding > the documentation and have had a busy weekend. I'll provide an update > tomorrow. This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
Noah, * Noah Misch (noah@leadboat.com) wrote: > On Sun, Apr 23, 2017 at 11:58:23PM +0000, Stephen Frost wrote: > > The status is simply that I've been considering Robert's comments regarding > > the documentation and have had a busy weekend. I'll provide an update > > tomorrow. > > This PostgreSQL 10 open item is past due for your status update. Kindly send > a status update within 24 hours, and include a date for your subsequent status > update. Refer to the policy on open item ownership: I committed the immediate issue regarding the ALTER TABLE ONLY commands failing when pg_dump's output was run against a new system on Tuesday, so that part is "fixed", but I continue to work through the lingering related issues with Amit and have been pushed related patches both yesterday and today. I'm anticipating a new patch (or perhaps two) from Amit tomorrow (well, later today at this point...) that I hope to push to clean up other related bugs in pg_dump's handling of partitioned tables. Frankly, I fully expect to find additional issues beyond those as we move forward, but assuming I can finish up the last couple patches from Amit tomorrow then I'll close out this open issue and open new ones as new issues are discovered. I'll commit to having this issue closed out, barring additional issues, by Sunday (2017-04-30). Thanks! Stephen
Hi Stephen, Sorry about the delay. On 2017/04/27 23:17, Stephen Frost wrote: > Amit, > > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: >>>> So to summarize what the patches do (some of these were posted earlier) >>>> >>>> 0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns >>> >>> I'm trying to understand why this is also different. At least on an >>> initial look, there seems to be a bunch more copy-and-paste from the >>> existing TypedTable to Partition in gram.y, which seems to all boil down >>> to removing the need for WITH OPTIONS to be specified. If WITH OPTIONS >>> would be too much to have included for partitioning, and it isn't >>> actually necessary, why not just make it optional, and use the same >>> construct for both, removing all the duplicaiton and the need for >>> pg_dump to output it? >> >> OK, I think optionally allowing WITH OPTIONS keywords would be nice. >> >> So in lieu of this patch, I propose a patch that modifies gram.y to allow >> WITH OPTIONS to specified. > > The point I was trying to get at was that if you make WITH OPTIONS > optional for the TypedTableElement case, you can remove all of the > PartitionElement-related productions and then both the OF type_name and > the PARTITION OF case will accept WITH OPTIONS as noise words and also > work without WITH OPTIONS being specified. > > This also makes the code actually match the documentation since, at > least in the CREATE FOREIGN TABLE documentation, we claim that WITH > OPTIONS is required. > > Please see a proposed patch attached to accomplish this. The committed patch looks good to me. Thanks. Now that WITH OPTIONS is optional even for CREATE TABLE OF, perhaps it needs to be mentioned in the release notes? >>> Given that we're now setting numParents for partitions, I >>> would think we'd just move the if (tbinfo->partitionOf) branches up >>> under the if (numParents > 0) branches, which I think would also help us >>> catch additional issues, like the fact that a binary-upgrade with >>> partitions in a different namespace from the partitioned table would >>> fail because the ATTACH PARTITION code doesn't account for it: >>> >>> appendPQExpBuffer(q, "ALTER TABLE ONLY %s ", >>> fmtId(tbinfo->partitionOf->dobj.name)); >>> appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n", >>> fmtId(tbinfo->dobj.name), >>> tbinfo->partitiondef); >>> >>> unlike the existing inheritance code a few lines above, which does: >>> >>> appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ", >>> fmtId(tbinfo->dobj.name)); >>> if (parentRel->dobj.namespace != tbinfo->dobj.namespace) >>> appendPQExpBuffer(q, "%s.", >>> fmtId(parentRel->dobj.namespace->dobj.name)); >>> appendPQExpBuffer(q, "%s;\n", >>> fmtId(parentRel->dobj.name)); >> >> That's a good point. I put both cases under the if (numParents > 0) block >> and appropriate text is emitted depending on whether the table is a >> partition or plain inheritance child. > > Right, ok. > >>>> 0004: Change the way pg_dump retrieves partitioning info (removes a lot >>>> of unnecessary code and instead makes partitioning case use the old >>>> inheritance code, etc.) >>> >>> This looks pretty good, at least on first blush, probably in part >>> because it's mostly removing code. The CASE statement in the >>> getTables() query isn't needed, once pg_get_partkeydef() has been >>> changed to not throw an ERROR when passed a non-partitioned table OID. >> >> Oh yes, fixed. > > Good. > >> 0003: Change the way pg_dump retrieves partitioning info (removes a lot >> of unnecessary code and instead makes partitioning case use the old >> inheritance code, etc.) > > This has conflicts due to my proposed patch as my patch changes pg_dump > to not output the now-noise-words WITH OPTIONS at all. Rebased. >> 0004: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE >> INHERIT to be emitted to attach a partition in addition to of ALTER >> TABLE ATTACH PARTITION and that no schema-name was emitted where it >> should have > > Given that it's touching the same places, this would really make sense > to merge into 0003 now. OK, done. > I'm going to continue to mull over the attached for a bit and add some > tests to it, but if it looks good then I'll push it this afternoon, > after which you'll hopefully have time to rebase 0003 and merge in 0004 > to that, which I can review and probably push tomorrow. Attached updated patches. 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
Attachment
Amit, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > Sorry about the delay. No worries, I'm just back from being in NY and will take a look at this tomorrow (wrt the open item, I'll provide a status tomorrow). Thanks! Stephen
Amit, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > Now that WITH OPTIONS is optional even for CREATE TABLE OF, perhaps it > needs to be mentioned in the release notes? Doesn't strike me as rising to the level of needing to go into the release notes, but I won't object if people feel that it needs mentioning. > Attached updated patches. Thanks, but aren't the changes for handling pg_dump --binary-upgrade when dealing with partitions whose parents are in another schema backwards? The source schema selected is for the partition, so we don't need to schema-qualify the partition, but we do need to schema-qualify the parent because it could be in another schema. I think the approach to use is to decide, first, if we need to schema-qualify the parent or not, then set up a string which has the qualified (if necessary) name of the parent, and then just use that when appropriate while building the ALTER TABLE command. Remember, we select the source schema of the table in tbinfo at the top of dumpTableSchema() (see 'selectSourceSchema()'), so it shouldn't ever be necessary to schema-qualify the table in tbinfo. I've somehow managed to run out of time again today (it's gotten quite busy lately), but I'll try to find time late tonight to continue working on this, or I'll be working on it again tomorrow. This *really* needs some tests that actually cover this case, as it's clearly not hard to get confused about what needs to be qualified and what doesn't. Thanks! Stephen
Amit, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > Attached updated patches. Please find an updated version which corrects the issue with binary-upgrade of partitioned tables having partitions in other schemas, along with a few other minor improvements. If you could take a look at it, I'd appreciate it. We already had a test case in the pg_dump TAP tests for partitions existing in a schema different from the partitioned table, but we weren't checking the binary-upgrade case, so I've added a check to do that now. I'm sure additional tests would be good to add and will take a look at doing that tomorrow, but this hopefully closes at least the latest issue. Assuming this looks good to you, I'll push it tomorrow, possibly with other minor adjustments and perhaps a few more tests. Thanks! Stephen
Attachment
Hi Stephen, On Wed, May 3, 2017 at 12:05 PM, Stephen Frost <sfrost@snowman.net> wrote: > Amit, > > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: >> Attached updated patches. > > Please find an updated version which corrects the issue with > binary-upgrade of partitioned tables having partitions in other schemas, > along with a few other minor improvements. > > If you could take a look at it, I'd appreciate it. We already had a > test case in the pg_dump TAP tests for partitions existing in a schema > different from the partitioned table, but we weren't checking the > binary-upgrade case, so I've added a check to do that now. I'm sure > additional tests would be good to add and will take a look at doing that > tomorrow, but this hopefully closes at least the latest issue. > > Assuming this looks good to you, I'll push it tomorrow, possibly with > other minor adjustments and perhaps a few more tests. Your latest patch looks good to me. Thanks, Amit
Amit, * Amit Langote (amitlangote09@gmail.com) wrote: > On Wed, May 3, 2017 at 12:05 PM, Stephen Frost <sfrost@snowman.net> wrote: > > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > >> Attached updated patches. > > > > Please find an updated version which corrects the issue with > > binary-upgrade of partitioned tables having partitions in other schemas, > > along with a few other minor improvements. > > > > If you could take a look at it, I'd appreciate it. We already had a > > test case in the pg_dump TAP tests for partitions existing in a schema > > different from the partitioned table, but we weren't checking the > > binary-upgrade case, so I've added a check to do that now. I'm sure > > additional tests would be good to add and will take a look at doing that > > tomorrow, but this hopefully closes at least the latest issue. > > > > Assuming this looks good to you, I'll push it tomorrow, possibly with > > other minor adjustments and perhaps a few more tests. > > Your latest patch looks good to me. Ok, great, thanks for taking a look at it. Too late for me to push it tonight, so I'll do so tomorrow. Thanks again! Stephen
Amit, * Amit Langote (amitlangote09@gmail.com) wrote: > On Wed, May 3, 2017 at 12:05 PM, Stephen Frost <sfrost@snowman.net> wrote: > > Assuming this looks good to you, I'll push it tomorrow, possibly with > > other minor adjustments and perhaps a few more tests. > > Your latest patch looks good to me. Found a few more issues (like pg_dump not working against older versions of PG, because the queries for older versions hadn't been adjusted) and did a bit more tidying. I'll push this in a couple of hours. Thanks! Stephen
Attachment
On Thu, May 04, 2017 at 05:47:02PM -0400, Stephen Frost wrote: > * Amit Langote (amitlangote09@gmail.com) wrote: > > On Wed, May 3, 2017 at 12:05 PM, Stephen Frost <sfrost@snowman.net> wrote: > > > Assuming this looks good to you, I'll push it tomorrow, possibly with > > > other minor adjustments and perhaps a few more tests. > > > > Your latest patch looks good to me. > > Found a few more issues (like pg_dump not working against older versions > of PG, because the queries for older versions hadn't been adjusted) and > did a bit more tidying. > > I'll push this in a couple of hours. This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com I see you did commit a related patch. Is this item done?
Noah,
On Fri, May 5, 2017 at 23:19 Noah Misch <noah@leadboat.com> wrote:
On Thu, May 04, 2017 at 05:47:02PM -0400, Stephen Frost wrote:
> * Amit Langote (amitlangote09@gmail.com) wrote:
> > On Wed, May 3, 2017 at 12:05 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > > Assuming this looks good to you, I'll push it tomorrow, possibly with
> > > other minor adjustments and perhaps a few more tests.
> >
> > Your latest patch looks good to me.
>
> Found a few more issues (like pg_dump not working against older versions
> of PG, because the queries for older versions hadn't been adjusted) and
> did a bit more tidying.
>
> I'll push this in a couple of hours.
This PostgreSQL 10 open item is past due for your status update. Kindly send
a status update within 24 hours, and include a date for your subsequent status
update. Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
I see you did commit a related patch. Is this item done?
Yes, I believe it to be. I anticipate reviewing the code in this area more in the coming weeks, but this specific issue can be marked as resolved.
Thanks!
Stephen
Hi Stephen, On 2017/05/06 12:28, Stephen Frost wrote: > Noah, > > On Fri, May 5, 2017 at 23:19 Noah Misch <noah@leadboat.com> wrote: > >> On Thu, May 04, 2017 at 05:47:02PM -0400, Stephen Frost wrote: >>> * Amit Langote (amitlangote09@gmail.com) wrote: >>>> On Wed, May 3, 2017 at 12:05 PM, Stephen Frost <sfrost@snowman.net> >> wrote: >>>>> Assuming this looks good to you, I'll push it tomorrow, possibly with >>>>> other minor adjustments and perhaps a few more tests. >>>> >>>> Your latest patch looks good to me. >>> >>> Found a few more issues (like pg_dump not working against older versions >>> of PG, because the queries for older versions hadn't been adjusted) and >>> did a bit more tidying. >>> >>> I'll push this in a couple of hours. >> >> This PostgreSQL 10 open item is past due for your status update. Kindly >> send >> a status update within 24 hours, and include a date for your subsequent >> status >> update. Refer to the policy on open item ownership: >> >> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com >> >> I see you did commit a related patch. Is this item done? > > > Yes, I believe it to be. I anticipate reviewing the code in this area more > in the coming weeks, but this specific issue can be marked as resolved. Thanks for committing the patch after improving it quite a bit, and sorry that I couldn't reply promptly during the last week due to vacation. Regards, Amit
Amit, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > Thanks for committing the patch after improving it quite a bit, and sorry > that I couldn't reply promptly during the last week due to vacation. No worries, hopefully you have an opportunity to review the additional changes I made and understand why they were necessary. Certainly, feel free to reach out if you have any questions or notice anything else which should be improved. Thanks! Stephen
On 2017/05/08 12:42, Stephen Frost wrote: > Amit, > > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: >> Thanks for committing the patch after improving it quite a bit, and sorry >> that I couldn't reply promptly during the last week due to vacation. > > No worries, hopefully you have an opportunity to review the additional > changes I made and understand why they were necessary. Certainly, feel > free to reach out if you have any questions or notice anything else > which should be improved. Do you intend to push the other patch to add regression tests for the non-inherited constraints? Here it is attached again for you to look over. 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
Attachment
Greetings, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > On 2017/05/08 12:42, Stephen Frost wrote: > > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > >> Thanks for committing the patch after improving it quite a bit, and sorry > >> that I couldn't reply promptly during the last week due to vacation. > > > > No worries, hopefully you have an opportunity to review the additional > > changes I made and understand why they were necessary. Certainly, feel > > free to reach out if you have any questions or notice anything else > > which should be improved. > > Do you intend to push the other patch to add regression tests for the > non-inherited constraints? Here it is attached again for you to look over. In a blast from the past, I had a long-lost note about trying to push this that I just stumbled over. Apologies for it getting lost in the cracks. I've rebased it and will plan to push it later this weekend, barring objections. Thanks! Stephen
Attachment
Greetings, * Stephen Frost (sfrost@snowman.net) wrote: > Greetings, > > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > > On 2017/05/08 12:42, Stephen Frost wrote: > > > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > > >> Thanks for committing the patch after improving it quite a bit, and sorry > > >> that I couldn't reply promptly during the last week due to vacation. > > > > > > No worries, hopefully you have an opportunity to review the additional > > > changes I made and understand why they were necessary. Certainly, feel > > > free to reach out if you have any questions or notice anything else > > > which should be improved. > > > > Do you intend to push the other patch to add regression tests for the > > non-inherited constraints? Here it is attached again for you to look over. > > In a blast from the past, I had a long-lost note about trying to push > this that I just stumbled over. > > Apologies for it getting lost in the cracks. I've rebased it and will > plan to push it later this weekend, barring objections. This has now been pushed. Thanks! Stephen
Attachment
On Tue, Dec 11, 2018 at 3:20 AM Stephen Frost <sfrost@snowman.net> wrote: > * Stephen Frost (sfrost@snowman.net) wrote: > > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > > > Do you intend to push the other patch to add regression tests for the > > > non-inherited constraints? Here it is attached again for you to look over. > > > > In a blast from the past, I had a long-lost note about trying to push > > this that I just stumbled over. > > > > Apologies for it getting lost in the cracks. I've rebased it and will > > plan to push it later this weekend, barring objections. > > This has now been pushed. Thank you Stephen for keeping track of this for so long and committing. Regards, Amit