Quantcast

pg_dump emits ALTER TABLE ONLY partitioned_table

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
49 messages Options
123
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

pg_dump emits ALTER TABLE ONLY partitioned_table

Amit Langote-2
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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

0001-pg_dump-do-not-emit-ALTER-TABLE-ONLY-for-partitioned.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Stephen Frost
Amit,

* Amit Langote ([hidden email]) 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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Amit Langote-2
Hi Stephen,

On 2017/02/17 22:32, Stephen Frost wrote:

> Amit,
>
> * Amit Langote ([hidden email]) 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




--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Stephen Frost
Amit,

* Amit Langote ([hidden email]) wrote:

> On 2017/02/17 22:32, Stephen Frost wrote:
> > * Amit Langote ([hidden email]) 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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Robert Haas
In reply to this post by Amit Langote-2
On Fri, Feb 17, 2017 at 3:23 AM, Amit Langote
<[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Amit Langote-2
In reply to this post by Stephen Frost
Hi Stephen,

On 2017/03/21 1:40, Stephen Frost wrote:

> Amit,
>
> * Amit Langote ([hidden email]) wrote:
>> On 2017/02/17 22:32, Stephen Frost wrote:
>>> * Amit Langote ([hidden email]) 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




--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Amit Langote-2
In reply to this post by Robert Haas
On 2017/03/27 23:30, Robert Haas wrote:

> On Fri, Feb 17, 2017 at 3:23 AM, Amit Langote
> <[hidden email]> 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




--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Robert Haas
On Tue, Mar 28, 2017 at 6:50 AM, Amit Langote
<[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Amit Langote-2
On 2017/03/29 0:39, Robert Haas wrote:

> On Tue, Mar 28, 2017 at 6:50 AM, Amit Langote
> <[hidden email]> 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




--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Noah Misch-2
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
> > <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Robert Haas
On Sun, Apr 9, 2017 at 7:50 PM, Noah Misch <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Tom Lane-2
Robert Haas <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Stephen Frost
Tom, Robert,

* Tom Lane ([hidden email]) wrote:
> Robert Haas <[hidden email]> 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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Tom Lane-2
Stephen Frost <[hidden email]> writes:
> * Tom Lane ([hidden email]) 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Robert Haas
In reply to this post by Tom Lane-2
On Sun, Apr 9, 2017 at 10:10 PM, Tom Lane <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Amit Langote-2
On 2017/04/11 0:26, Robert Haas wrote:

> On Sun, Apr 9, 2017 at 10:10 PM, Tom Lane <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

0001-Fix-how-pg_dump-emits-partition-attributes.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Stephen Frost
Amit,

* Amit Langote ([hidden email]) 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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Amit Langote-2
Hi Stephen,

On 2017/04/11 22:12, Stephen Frost wrote:

> Amit,
>
> * Amit Langote ([hidden email]) 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

0001-Fix-ALTER-TABLE-ONLY-to-avoid-unnecessarily-failures.patch (10K) Download Attachment
0002-Fix-pg_dump-to-handle-partition-inheritance-sanely.patch (7K) Download Attachment
0003-Do-not-emit-WITH-OPTIONS-for-partition-s-columns.patch (903 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Robert Haas
On Wed, Apr 12, 2017 at 6:29 AM, Amit Langote
<[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Stephen Frost
Robert,

* Robert Haas ([hidden email]) wrote:

> On Wed, Apr 12, 2017 at 6:29 AM, Amit Langote
> <[hidden email]> 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

signature.asc (836 bytes) Download Attachment
123
Previous Thread Next Thread
Loading...