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
>From 72ad894d79b6f26d24a8857e5f33c6d51bd65051 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 24 Apr 2019 15:30:37 -0400
Subject: [PATCH v1 1/2] Make pg_dump emit ATTACH PARTITION instead of
PARTITION OF
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Using PARTITION OF can result in column ordering being changed from the
database being dumped, if the partition uses a column layout different
from the parent's. It's not pg_dump's job to editorialize on table
definitions, so this is not acceptable; back-patch all the way back to
pg10, where partitioned tables where introduced.
This change also ensures that partitions end up in the correct
tablespace, if different from the parent's; this is an oversight in
ca4103025dfe (in pg12 only). Partitioned indexes (in pg11) don't have
this problem, because they're already created as independent indexes and
attached to their parents afterwards.
This change also has the advantage that the partition is restorable from
the dump (as a standalone table) even if its parent table isn't
restored.
Author: David Rowley
Reviewed-by: Álvaro Herrera
Discussion: https://postgr.es/m/cakjs1f_1c260not_vbj067az3jxptxvrohdvmlebmudx1ye...@mail.gmail.com
Discussion: https://postgr.es/m/20190423185007.GA27954@alvherre.pgsql
---
src/bin/pg_dump/pg_dump.c | 123 +++++++++++++------------------
src/bin/pg_dump/t/002_pg_dump.pl | 12 ++-
2 files changed, 60 insertions(+), 75 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 9f59cc74ee..408eee9119 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8631,9 +8631,12 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
* Normally this is always true, but it's false for dropped columns, as well
* as those that were inherited without any local definition. (If we print
* such a column it will mistakenly get pg_attribute.attislocal set to true.)
- * However, in binary_upgrade mode, we must print all such columns anyway and
- * fix the attislocal/attisdropped state later, so as to keep control of the
- * physical column order.
+ * For partitions, it's always true, because we want the partitions to be
+ * created independently and ATTACH PARTITION used afterwards.
+ *
+ * In binary_upgrade mode, we must print all columns and fix the attislocal/
+ * attisdropped state later, so as to keep control of the physical column
+ * order.
*
* This function exists because there are scattered nonobvious places that
* must be kept in sync with this decision.
@@ -8643,7 +8646,9 @@ shouldPrintColumn(DumpOptions *dopt, TableInfo *tbinfo, int colno)
{
if (dopt->binary_upgrade)
return true;
- return (tbinfo->attislocal[colno] && !tbinfo->attisdropped[colno]);
+ if (tbinfo->attisdropped[colno])
+ return false;
+ return (tbinfo->attislocal[colno] || tbinfo->ispartition);
}
@@ -15594,27 +15599,6 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
if (tbinfo->reloftype && !dopt->binary_upgrade)
appendPQExpBuffer(q, " OF %s", tbinfo->reloftype);
- /*
- * If the table is a partition, dump it as such; except in the case of
- * a binary upgrade, we dump the table normally and attach it to the
- * parent afterward.
- */
- if (tbinfo->ispartition && !dopt->binary_upgrade)
- {
- TableInfo *parentRel = tbinfo->parents[0];
-
- /*
- * With partitions, unlike inheritance, there can only be one
- * parent.
- */
- if (tbinfo->numParents != 1)
- fatal("invalid number of parents %d for table \"%s\"",
- tbinfo->numParents, tbinfo->dobj.name);
-
- appendPQExpBuffer(q, " PARTITION OF %s",
- fmtQualifiedDumpable(parentRel));
- }
-
if (tbinfo->relkind != RELKIND_MATVIEW)
{
/* Dump the attributes */
@@ -15643,12 +15627,9 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
(!tbinfo->inhNotNull[j] ||
dopt->binary_upgrade));
- /*
- * Skip column if fully defined by reloftype or the
- * partition parent.
- */
- if ((tbinfo->reloftype || tbinfo->ispartition) &&
- !has_default && !has_notnull && !dopt->binary_upgrade)
+ /* Skip column if fully defined by reloftype */
+ if (tbinfo->reloftype && !has_default && !has_notnull &&
+ !dopt->binary_upgrade)
continue;
/* Format properly if not first attr */
@@ -15671,20 +15652,16 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
* clean things up later.
*/
appendPQExpBufferStr(q, " INTEGER /* dummy */");
- /* Skip all the rest, too */
+ /* and skip to the next column */
continue;
}
/*
- * Attribute type
- *
- * In binary-upgrade mode, we always include the type. If
- * we aren't in binary-upgrade mode, then we skip the type
- * when creating a typed table ('OF type_name') or a
- * partition ('PARTITION OF'), since the type comes from
- * the parent/partitioned table.
+ * Attribute type; print it except when creating a typed
+ * table ('OF type_name'), but in binary-upgrade mode,
+ * print it in that case too.
*/
- if (dopt->binary_upgrade || (!tbinfo->reloftype && !tbinfo->ispartition))
+ if (dopt->binary_upgrade || !tbinfo->reloftype)
{
appendPQExpBuffer(q, " %s",
tbinfo->atttypnames[j]);
@@ -15741,25 +15718,20 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
if (actual_atts)
appendPQExpBufferStr(q, "\n)");
- else if (!((tbinfo->reloftype || tbinfo->ispartition) &&
- !dopt->binary_upgrade))
+ else if (!(tbinfo->reloftype && !dopt->binary_upgrade))
{
/*
- * We must have a parenthesized attribute list, even though
- * empty, when not using the OF TYPE or PARTITION OF syntax.
+ * No attributes? we must have a parenthesized attribute list,
+ * even though empty, when not using the OF TYPE syntax.
*/
appendPQExpBufferStr(q, " (\n)");
}
- if (tbinfo->ispartition && !dopt->binary_upgrade)
- {
- appendPQExpBufferChar(q, '\n');
- appendPQExpBufferStr(q, tbinfo->partbound);
- }
-
- /* Emit the INHERITS clause, except if this is a partition. */
- if (numParents > 0 &&
- !tbinfo->ispartition &&
+ /*
+ * Emit the INHERITS clause (not for partitions), except in
+ * binary-upgrade mode.
+ */
+ if (numParents > 0 && !tbinfo->ispartition &&
!dopt->binary_upgrade)
{
appendPQExpBufferStr(q, "\nINHERITS (");
@@ -15932,30 +15904,16 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
appendPQExpBufferStr(q, "::pg_catalog.regclass;\n");
}
- if (numParents > 0)
+ if (numParents > 0 && !tbinfo->ispartition)
{
- appendPQExpBufferStr(q, "\n-- For binary upgrade, set up inheritance and partitioning this way.\n");
+ appendPQExpBufferStr(q, "\n-- For binary upgrade, set up inheritance this way.\n");
for (k = 0; k < numParents; k++)
{
TableInfo *parentRel = parents[k];
- /* In the partitioning case, we alter the parent */
- if (tbinfo->ispartition)
- appendPQExpBuffer(q,
- "ALTER TABLE ONLY %s ATTACH PARTITION ",
- fmtQualifiedDumpable(parentRel));
- else
- appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ",
- qualrelname);
-
- /* Partition needs specifying the bounds */
- if (tbinfo->ispartition)
- appendPQExpBuffer(q, "%s %s;\n",
- qualrelname,
- tbinfo->partbound);
- else
- appendPQExpBuffer(q, "%s;\n",
- fmtQualifiedDumpable(parentRel));
+ appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT %s;\n",
+ qualrelname,
+ fmtQualifiedDumpable(parentRel));
}
}
@@ -15968,6 +15926,27 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
}
}
+ /*
+ * For partitioned tables, emit the ATTACH PARTITION clause. Note
+ * that we always want to create partitions this way instead of using
+ * CREATE TABLE .. PARTITION OF, mainly to preserve a possible column
+ * layout discrepancy with the parent, but also to ensure it gets the
+ * correct tablespace setting if it differs from the parent's.
+ */
+ if (tbinfo->ispartition)
+ {
+ /* With partitions there can only be one parent */
+ if (tbinfo->numParents != 1)
+ fatal("invalid number of parents %d for table \"%s\"",
+ tbinfo->numParents, tbinfo->dobj.name);
+
+ /* Perform ALTER TABLE on the parent */
+ appendPQExpBuffer(q,
+ "ALTER TABLE ONLY %s ATTACH PARTITION %s %s;\n",
+ fmtQualifiedDumpable(parents[0]),
+ qualrelname, tbinfo->partbound);
+ }
+
/*
* In binary_upgrade mode, arrange to restore the old relfrozenxid and
* relminmxid of all vacuumable relations. (While vacuum.c processes
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index ea6283c51a..54253c42d7 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -733,7 +733,12 @@ my %tests = (
\QALTER TABLE ONLY dump_test.measurement ATTACH PARTITION dump_test_second_schema.measurement_y2006m2 \E
\QFOR VALUES FROM ('2006-02-01') TO ('2006-03-01');\E\n
/xm,
- like => { binary_upgrade => 1, },
+ like => {
+ %full_runs,
+ role => 1,
+ section_pre_data => 1,
+ binary_upgrade => 1,
+ },
},
'ALTER TABLE test_table CLUSTER ON test_table_pkey' => {
@@ -2323,12 +2328,13 @@ my %tests = (
\)\n
\QFOR VALUES FROM ('2006-02-01') TO ('2006-03-01');\E\n
/xm,
- like => {
+ like => {},
+ unlike => {
%full_runs,
role => 1,
section_pre_data => 1,
+ binary_upgrade => 1,
},
- unlike => { binary_upgrade => 1, },
},
'CREATE TABLE test_fourth_table_zero_col' => {
--
2.17.1
>From abc43ad7eaf1a4178c696cc88d59618b5e43c24e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 7 May 2019 09:52:35 -0400
Subject: [PATCH v1 2/2] Fix code to consider NOT NULL, add tests
---
src/bin/pg_dump/pg_dump.c | 9 ++--
src/bin/pg_dump/t/002_pg_dump.pl | 80 +++++++++++++++++++++++++++-----
2 files changed, 74 insertions(+), 15 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 408eee9119..df128e4508 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15621,10 +15621,12 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
/*
* Not Null constraint --- suppress if inherited, except
- * in binary-upgrade case where that won't work.
+ * if partition, or in binary-upgrade case where that
+ * won't work.
*/
bool has_notnull = (tbinfo->notnull[j] &&
- (!tbinfo->inhNotNull[j] ||
+ (tbinfo->ispartition ||
+ !tbinfo->inhNotNull[j] ||
dopt->binary_upgrade));
/* Skip column if fully defined by reloftype */
@@ -15701,7 +15703,8 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
{
ConstraintInfo *constr = &(tbinfo->checkexprs[j]);
- if (constr->separate || !constr->conislocal)
+ if (constr->separate ||
+ (!constr->conislocal && !tbinfo->ispartition))
continue;
if (actual_atts == 0)
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 54253c42d7..e631c919e3 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -2288,9 +2288,9 @@ my %tests = (
'CREATE TABLE measurement PARTITIONED BY' => {
create_order => 90,
create_sql => 'CREATE TABLE dump_test.measurement (
- city_id int not null,
+ city_id serial not null,
logdate date not null,
- peaktemp int,
+ peaktemp int CHECK (peaktemp >= -460),
unitsales int
) PARTITION BY RANGE (logdate);',
regexp => qr/^
@@ -2300,7 +2300,8 @@ my %tests = (
\s+\Qcity_id integer NOT NULL,\E\n
\s+\Qlogdate date NOT NULL,\E\n
\s+\Qpeaktemp integer,\E\n
- \s+\Qunitsales integer\E\n
+ \s+\Qunitsales integer,\E\n
+ \s+\QCONSTRAINT measurement_peaktemp_check CHECK ((peaktemp >= '-460'::integer))\E\n
\)\n
\QPARTITION BY RANGE (logdate);\E\n
/xm,
@@ -2312,7 +2313,7 @@ my %tests = (
},
},
- 'CREATE TABLE measurement_y2006m2 PARTITION OF' => {
+ 'Partition measurement_y2006m2 creation' => {
create_order => 91,
create_sql =>
'CREATE TABLE dump_test_second_schema.measurement_y2006m2
@@ -2321,18 +2322,31 @@ my %tests = (
)
FOR VALUES FROM (\'2006-02-01\') TO (\'2006-03-01\');',
regexp => qr/^
- \Q-- Name: measurement_y2006m2;\E.*\n
- \Q--\E\n\n
- \QCREATE TABLE dump_test_second_schema.measurement_y2006m2 PARTITION OF dump_test.measurement (\E\n
+ \QCREATE TABLE dump_test_second_schema.measurement_y2006m2 (\E\n
+ \s+\Qcity_id integer DEFAULT nextval('dump_test.measurement_city_id_seq'::regclass) NOT NULL,\E\n
+ \s+\Qlogdate date NOT NULL,\E\n
+ \s+\Qpeaktemp integer,\E\n
+ \s+\Qunitsales integer DEFAULT 0,\E\n
+ \s+\QCONSTRAINT measurement_peaktemp_check CHECK ((peaktemp >= '-460'::integer)),\E\n
\s+\QCONSTRAINT measurement_y2006m2_unitsales_check CHECK ((unitsales >= 0))\E\n
- \)\n
- \QFOR VALUES FROM ('2006-02-01') TO ('2006-03-01');\E\n
+ \);\n
/xm,
- like => {},
- unlike => {
+ like => {
%full_runs,
- role => 1,
section_pre_data => 1,
+ role => 1,
+ binary_upgrade => 1,
+ },
+ },
+
+ 'Partition measurement_y2006m2 attach' => {
+ regexp => qr/^
+ \QALTER TABLE ONLY dump_test.measurement ATTACH PARTITION dump_test_second_schema.measurement_y2006m2 FOR VALUES FROM ('2006-02-01') TO ('2006-03-01');\E\n
+ /xm,
+ like => {
+ %full_runs,
+ section_pre_data => 1,
+ role => 1,
binary_upgrade => 1,
},
},
@@ -2438,6 +2452,48 @@ my %tests = (
unlike => { exclude_dump_test_schema => 1, },
},
+ 'CREATE TABLE test_inheritance_parent' => {
+ create_order => 90,
+ create_sql => 'CREATE TABLE dump_test.test_inheritance_parent (
+ col1 int NOT NULL,
+ col2 int CHECK (col2 >= 42)
+ );',
+ regexp => qr/^
+ \QCREATE TABLE dump_test.test_inheritance_parent (\E\n
+ \s+\Qcol1 integer NOT NULL,\E\n
+ \s+\Qcol2 integer,\E\n
+ \s+\QCONSTRAINT test_inheritance_parent_col2_check CHECK ((col2 >= 42))\E\n
+ \Q);\E\n
+ /xm,
+ like =>
+ { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
+ unlike => { exclude_dump_test_schema => 1, },
+ },
+
+ 'CREATE TABLE test_inheritance_child' => {
+ create_order => 91,
+ create_sql => 'CREATE TABLE dump_test.test_inheritance_child (
+ col1 int NOT NULL,
+ CONSTRAINT test_inheritance_child CHECK (col2 >= 142857)
+ ) INHERITS (dump_test.test_inheritance_parent);',
+ regexp => qr/^
+ \QCREATE TABLE dump_test.test_inheritance_child (\E\n
+ \s+\Qcol1 integer,\E\n
+ \s+\QCONSTRAINT test_inheritance_child CHECK ((col2 >= 142857))\E\n
+ \)\n
+ \QINHERITS (dump_test.test_inheritance_parent);\E\n
+ /xm,
+ like => {
+ %full_runs,
+ %dump_test_schema_runs,
+ section_pre_data => 1,
+ },
+ unlike => {
+ binary_upgrade => 1,
+ exclude_dump_test_schema => 1,
+ },
+ },
+
'CREATE STATISTICS extended_stats_no_options' => {
create_order => 97,
create_sql => 'CREATE STATISTICS dump_test.test_ext_stats_no_options
--
2.17.1
