Thread: pg_dump: fail to restore partition table with serial type
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
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
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));
* 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,
Attachment
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.sqlWhile 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 TABLEProblem 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, thatmeans CREATE TABLE get build with attributes for each child. I foundthat NOT NULL constraints not getting dump for the child table and thatis the reason restore end up with above error.Looking at code found the below code which skip the NULL NULLconstraint for the inherited table - and which is the reason it alsoit 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 NULLfor partition table.PS: we also need to backport this to v11.Thanks,
--
Rushabh Lathia
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
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
Attachment
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
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
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