From 0f889151e56a3b6ce7e522799bbbb8d644ca54c4 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Mon, 8 Apr 2019 16:40:05 -0400 Subject: [PATCH v2] Make pg_dump emit ATTACH PARTITION instead of PARTITION OF ca41030 modified how the TABLESPACE option worked with partitioned tables so that it was possible to specify and store a TABLESPACE for a partitioned table. This setting served only to mark what the default tablespace should be for new partitions which were created with the CREATE TABLE PARTITION OF syntax. The problem with this was that pg_dump used the PARTITION OF syntax which meant that if a partition had been explicitly defined to have pg_default as the tablespace then since reltablespace is set to 0 in this case, pg_dump would emit a CREATE TABLE .. PARTITION OF statement and cause the partition to default to the partitioned table's tablespace rather than the one it was meant to be located in. We can work around this problem by simply performing the CREATE TABLE first then perform an ATTACH PARTITION later. This bypasses the check of the parent partitioned table's tablespace in DefineRelation() as the table is not a partition when it is defined. Doing this also means that when restoring partitions they now maintain their original column ordering rather than switch to their partitioned table's column ordering. This is perhaps minor, but it is noteworthy pg_dump in binary upgrade mode already worked this way, so it turns out this commit removes more code than it adds. --- src/bin/pg_dump/pg_dump.c | 118 ++++++++++++++----------------- src/bin/pg_dump/t/002_pg_dump.pl | 12 +++- 2 files changed, 61 insertions(+), 69 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 452f30760bb..a7987790df5 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -8618,9 +8618,11 @@ 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. + * + * 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. We also do this for partitions, for a different reason: we want + * them to be created independently, and ATTACH PARTITION used afterwards. * * This function exists because there are scattered nonobvious places that * must be kept in sync with this decision. @@ -8630,7 +8632,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); } @@ -15599,27 +15603,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 */ @@ -15648,12 +15631,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 */ @@ -15676,7 +15656,7 @@ 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; } @@ -15685,11 +15665,9 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo) * * 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. + * when creating a typed table ('OF type_name'). */ - if (dopt->binary_upgrade || (!tbinfo->reloftype && !tbinfo->ispartition)) + if (dopt->binary_upgrade || !tbinfo->reloftype) { appendPQExpBuffer(q, " %s", tbinfo->atttypnames[j]); @@ -15746,23 +15724,19 @@ 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. */ + /* + * Emit the INHERITS clause, except if this is a partition. When + * in binary upgrade mode we'll do this later in this function. + */ if (numParents > 0 && !tbinfo->ispartition && !dopt->binary_upgrade) @@ -15937,30 +15911,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)); } } @@ -15973,6 +15933,32 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo) } } + /* + * For partitioned tables, emit the ATTACH PARTITION clause. Note + * that we always want to create partitions in this way instead using + * CREATE TABLE .. PARTITION OF, because doing it the other way would + * result in an incorrect tablespace setting for the partition, if the + * partitioned table has one and the partitioned table is created in + * a different one. + */ + if (tbinfo->ispartition) + { + /* + * 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); + + /* Perform ALTER TABLE on the parent */ + appendPQExpBuffer(q, "ALTER TABLE ONLY %s ATTACH PARTITION ", + fmtQualifiedDumpable(parents[0])); + + /* specify the bounds */ + appendPQExpBuffer(q, "%s %s;\n", 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 5721882b3b2..5c1acd014bf 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' => { @@ -2322,12 +2327,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