Horton

Thread

Commits

  1. Make pg_dump emit ATTACH PARTITION instead of PARTITION OF (reprise)

  2. Revert "Make pg_dump emit ATTACH PARTITION instead of PARTITION OF"

  3. Make pg_dump emit ATTACH PARTITION instead of PARTITION OF

  4. Allow a partitioned table to have a default partition.

  5. Give partitioned table "p" in regression tests a less generic name.

  1. pg_dump: fail to restore partition table with serial type

    Rushabh Lathia <rushabh.lathia@gmail.com> — 2019-05-06T05:43:51Z

    Hi,

Consider the following test scenario:

create table test ( c1 serial, c2 int not null ) partition by list (c2);
create table test_p1 partition of test for values in ( 1);
create table test_p2 partition of test for values in ( 2);

rushabh@rushabh:postgresql$ ./db/bin/pg_dump db1  > dump.sql

While restoring above dump it's throwing a below error:

CREATE TABLE
psql:dump.sql:66: ERROR:  column "c1" in child table must be marked NOT NULL
ALTER TABLE
CREATE TABLE
psql:dump.sql:79: ERROR:  column "c1" in child table must be marked NOT NULL
ALTER TABLE

Problem got introduced with below commit:

commit 3b23552ad8bbb1384381b67f860019d14d5b680e
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date:   Wed Apr 24 15:30:37 2019 -0400

    Make pg_dump emit ATTACH PARTITION instead of PARTITION OF

Above commit use ATTACH PARTITION instead of PARTITION OF, that
means CREATE TABLE get build with attributes for each child.  I found
that NOT NULL constraints not getting dump for the child table and that
is the reason restore end up with above error.

Looking at code found the below code which skip the NULL NULL
constraint for the inherited table - and which is the reason it also
it end up not emitting the NOT NULL constraint for child table:

                    /*
                     * Not Null constraint --- suppress if inherited, except
                     * in binary-upgrade case where that won't work.
                     */
                    bool        has_notnull = (tbinfo->notnull[j] &&
                                               (!tbinfo->inhNotNull[j] ||
                                                dopt->binary_upgrade));

PFA patch to fix the issue, which allow to dump the NOT NULL
for partition table.

PS: we also need to backport this to v11.

Thanks,
-- 
Rushabh Lathia
www.EnterpriseDB.com

  2. Re: pg_dump: fail to restore partition table with serial type

    Rushabh Lathia <rushabh.lathia@gmail.com> — 2019-05-06T08:55:39Z

    Found another scenario where check constraint is not getting
dump for the child table.

Testcase:

create table test ( c1 serial, c2 int not null, c3 integer CHECK (c3 > 0))
partition by list (c2);
create table test_p1 partition of test for values in ( 1);
create table test_p2 partition of test for values in ( 2);

In the above test, check constraint for column c3 is not getting
dump with CREATE TABLE, and that is the reason ATTACH
PARTITION is failing.

Seems like need to handle NOT NULL and CHECK CONSTRAINT
differently than the inheritance table.




On Mon, May 6, 2019 at 11:13 AM Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:

> Hi,
>
> Consider the following test scenario:
>
> create table test ( c1 serial, c2 int not null ) partition by list (c2);
> create table test_p1 partition of test for values in ( 1);
> create table test_p2 partition of test for values in ( 2);
>
> rushabh@rushabh:postgresql$ ./db/bin/pg_dump db1  > dump.sql
>
> While restoring above dump it's throwing a below error:
>
> CREATE TABLE
> psql:dump.sql:66: ERROR:  column "c1" in child table must be marked NOT
> NULL
> ALTER TABLE
> CREATE TABLE
> psql:dump.sql:79: ERROR:  column "c1" in child table must be marked NOT
> NULL
> ALTER TABLE
>
> Problem got introduced with below commit:
>
> commit 3b23552ad8bbb1384381b67f860019d14d5b680e
> Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
> Date:   Wed Apr 24 15:30:37 2019 -0400
>
>     Make pg_dump emit ATTACH PARTITION instead of PARTITION OF
>
> Above commit use ATTACH PARTITION instead of PARTITION OF, that
> means CREATE TABLE get build with attributes for each child.  I found
> that NOT NULL constraints not getting dump for the child table and that
> is the reason restore end up with above error.
>
> Looking at code found the below code which skip the NULL NULL
> constraint for the inherited table - and which is the reason it also
> it end up not emitting the NOT NULL constraint for child table:
>
>                     /*
>                      * Not Null constraint --- suppress if inherited,
> except
>                      * in binary-upgrade case where that won't work.
>                      */
>                     bool        has_notnull = (tbinfo->notnull[j] &&
>                                                (!tbinfo->inhNotNull[j] ||
>                                                 dopt->binary_upgrade));
>
> PFA patch to fix the issue, which allow to dump the NOT NULL
> for partition table.
>
> PS: we also need to backport this to v11.
>
> Thanks,
> --
> Rushabh Lathia
> www.EnterpriseDB.com
>
>

-- 
Rushabh Lathia

  3. Re: pg_dump: fail to restore partition table with serial type

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-06T16:13:31Z

    On 2019-May-06, Rushabh Lathia wrote:

> Found another scenario where check constraint is not getting
> dump for the child table.

You're right, the patched code is bogus; I'm reverting it all for
today's minors.  Thanks for reporting.

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

  4. Re: pg_dump: fail to restore partition table with serial type

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-06-07T18:36:41Z

    On 2019-May-06, Alvaro Herrera wrote:

> On 2019-May-06, Rushabh Lathia wrote:
> 
> > Found another scenario where check constraint is not getting
> > dump for the child table.
> 
> You're right, the patched code is bogus; I'm reverting it all for
> today's minors.  Thanks for reporting.

Here's another version of this patch.  This time, I added some real
tests in pg_dump's suite, including a SERIAL column and NOT NULL
constraints.  The improved test verifies that the partition is created
separately and later attached, and it includes constraints from the
parent as well as some locally defined ones.  I also added tests with
legacy inheritance, which was not considered previously in pg_dump tests
as far as I could see.

I looked for other cases that could have been broken by changing the
partition creation methodology in pg_dump, and didn't find anything.
That part of pg_dump (dumpTableSchema) is pretty spaghettish, though;
the fact that shouldPrintColumn makes some partitioned-related decisions
and then dumpTableSchema make them again is notoriously confusing.  I
could have easily missed something.


One weird thing about pg_dump's output of the serial column in a
partitioned table is that it emits the parent table itself first without
a DEFAULT clause, then the sequence and marks it as owned by the column;
then it emits the partition *with* the default clause, and finally it
alters the parent table's column to set the default.  Now there is some
method in this madness (the OWNED BY clause for the sequence is mingled
together with the sequence itself), but I think this arrangement makes
a partial restore of the partition fail.

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

  5. Re: pg_dump: fail to restore partition table with serial type

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-06-10T23:07:52Z

    On 2019-Jun-07, Alvaro Herrera wrote:

> I looked for other cases that could have been broken by changing the
> partition creation methodology in pg_dump, and didn't find anything.
> That part of pg_dump (dumpTableSchema) is pretty spaghettish, though;
> the fact that shouldPrintColumn makes some partitioned-related decisions
> and then dumpTableSchema make them again is notoriously confusing.  I
> could have easily missed something.

There was indeed one more problem, that only the pg10 pg_upgrade test
detected.  Namely, binary-upgrade dump didn't restore for locally
defined constraints: they were dumped twice, first in the table
definition and later by the ALTER TABLE ADD CONSTRAINT bit for binary
upgrade that I had failed to notice.  Ooops.  The reason pg10 detected
it and the other branches didn't, is that the only constraint of this
ilk that remained after running regress was removed by 05bd889904e0 :-(

Pushed to the three branches.  Hopefully it won't explode as
spectacularly this time ...

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

  6. Re: pg_dump: fail to restore partition table with serial type

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-13T00:46:57Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> There was indeed one more problem, that only the pg10 pg_upgrade test
> detected.  Namely, binary-upgrade dump didn't restore for locally
> defined constraints: they were dumped twice, first in the table
> definition and later by the ALTER TABLE ADD CONSTRAINT bit for binary
> upgrade that I had failed to notice.  Ooops.  The reason pg10 detected
> it and the other branches didn't, is that the only constraint of this
> ilk that remained after running regress was removed by 05bd889904e0 :-(

Seems like we'd better put back some coverage for that case, no?
But I'm confused by your reference to 05bd889904e0.  It looks like
that didn't change anything about tables that weren't getting dropped
anyhow.

			regards, tom lane

  7. Re: pg_dump: fail to restore partition table with serial type

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-06-13T03:26:43Z

    On 2019-Jun-12, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > There was indeed one more problem, that only the pg10 pg_upgrade test
> > detected.  Namely, binary-upgrade dump didn't restore for locally
> > defined constraints: they were dumped twice, first in the table
> > definition and later by the ALTER TABLE ADD CONSTRAINT bit for binary
> > upgrade that I had failed to notice.  Ooops.  The reason pg10 detected
> > it and the other branches didn't, is that the only constraint of this
> > ilk that remained after running regress was removed by 05bd889904e0 :-(
> 
> Seems like we'd better put back some coverage for that case, no?

I'll work on that.

> But I'm confused by your reference to 05bd889904e0.  It looks like
> that didn't change anything about tables that weren't getting dropped
> anyhow.

Ah ... yeah, I pasted the wrong commit ID.  That commit indeed removed
one occurrence of constraint check_b, but it wasn't the one that
detected the failure -- the one that did (also named check_b) was
removed by commit 6f6b99d1335b (pg11 only).

Commit aa56671836e6 (in pg10, two months after 05bd889904e0) changed
those tables afterwards so that they wouldn't be dropped. 

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