]> granicus.if.org Git - postgresql/commitdiff
Revert "Make pg_dump emit ATTACH PARTITION instead of PARTITION OF"
authorAlvaro Herrera <alvherre@alvh.no-ip.org>
Mon, 6 May 2019 16:23:49 +0000 (12:23 -0400)
committerAlvaro Herrera <alvherre@alvh.no-ip.org>
Mon, 6 May 2019 16:23:49 +0000 (12:23 -0400)
... and fallout (from branches 10, 11 and master).  The change was
ill-considered, and it broke a few normal use cases; since we don't have
time to fix it, we'll try again after this week's minor releases.

Reported-by: Rushabh Lathia
Discussion: https://postgr.es/m/CAGPqQf0iQV=PPOv2Btog9J9AwOQp6HmuVd6SbGTR_v3Zp2XT1w@mail.gmail.com

doc/src/sgml/release-10.sgml
src/bin/pg_dump/pg_dump.c
src/bin/pg_dump/t/002_pg_dump.pl

index 80df50ce08b141236180d74fb1f2b7cfdd14cc43..fc90f873d1ac4ed0f5ad82fd3403d2ffd0ba7dac 100644 (file)
      </para>
     </listitem>
 
-    <listitem>
-     <para>
-      Make <application>pg_dump</application> recreate table partitions
-      using <command>ATTACH PARTITION</command> instead
-      of <command>CREATE TABLE ... PARTITION OF</command> (David Rowley)
-     </para>
-
-     <para>
-      This avoids various corner-case problems, notably that dump and
-      restore might unexpectedly alter a partition's column ordering.
-      It also means that a selective restore of the partition can succeed
-      even if its parent partitioned table isn't restored.
-      (The <command>ATTACH PARTITION</command> will fail of course, but
-      the partition table itself can be created and populated.)
-     </para>
-    </listitem>
-
     <listitem>
      <para>
       Avoid crash in <filename>contrib/postgres_fdw</filename> when a
index 5e28556c62488602f96fd5056360c1ceb4683e22..376b8c7db7811893df2fe88057a541e3f2eb19e7 100644 (file)
@@ -8188,12 +8188,9 @@ 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.)
- * 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.
+ * 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.
  *
  * This function exists because there are scattered nonobvious places that
  * must be kept in sync with this decision.
@@ -8203,9 +8200,7 @@ shouldPrintColumn(DumpOptions *dopt, TableInfo *tbinfo, int colno)
 {
        if (dopt->binary_upgrade)
                return true;
-       if (tbinfo->attisdropped[colno])
-               return false;
-       return (tbinfo->attislocal[colno] || tbinfo->ispartition);
+       return (tbinfo->attislocal[colno] && !tbinfo->attisdropped[colno]);
 }
 
 
@@ -14968,6 +14963,27 @@ 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)
+                               exit_horribly(NULL, "invalid number of parents %d for table \"%s\"\n",
+                                                         tbinfo->numParents, tbinfo->dobj.name);
+
+                       appendPQExpBuffer(q, " PARTITION OF %s",
+                                                         fmtQualifiedDumpable(parentRel));
+               }
+
                if (tbinfo->relkind != RELKIND_MATVIEW)
                {
                        /* Dump the attributes */
@@ -14996,9 +15012,12 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
                                                                                           (!tbinfo->inhNotNull[j] ||
                                                                                                dopt->binary_upgrade));
 
-                                       /* Skip column if fully defined by reloftype */
-                                       if (tbinfo->reloftype && !has_default && !has_notnull &&
-                                               !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)
                                                continue;
 
                                        /* Format properly if not first attr */
@@ -15021,16 +15040,20 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
                                                 * clean things up later.
                                                 */
                                                appendPQExpBufferStr(q, " INTEGER /* dummy */");
-                                               /* and skip to the next column */
+                                               /* Skip all the rest, too */
                                                continue;
                                        }
 
                                        /*
-                                        * Attribute type; print it except when creating a typed
-                                        * table ('OF type_name'), but in binary-upgrade mode,
-                                        * print it in that case too.
+                                        * 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.
                                         */
-                                       if (dopt->binary_upgrade || !tbinfo->reloftype)
+                                       if (dopt->binary_upgrade || (!tbinfo->reloftype && !tbinfo->ispartition))
                                        {
                                                appendPQExpBuffer(q, " %s",
                                                                                  tbinfo->atttypnames[j]);
@@ -15080,20 +15103,25 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
 
                        if (actual_atts)
                                appendPQExpBufferStr(q, "\n)");
-                       else if (!(tbinfo->reloftype && !dopt->binary_upgrade))
+                       else if (!((tbinfo->reloftype || tbinfo->ispartition) &&
+                                          !dopt->binary_upgrade))
                        {
                                /*
-                                * No attributes? we must have a parenthesized attribute list,
-                                * even though empty, when not using the OF TYPE syntax.
+                                * We must have a parenthesized attribute list, even though
+                                * empty, when not using the OF TYPE or PARTITION OF syntax.
                                 */
                                appendPQExpBufferStr(q, " (\n)");
                        }
 
-                       /*
-                        * Emit the INHERITS clause (not for partitions), except in
-                        * binary-upgrade mode.
-                        */
-                       if (numParents > 0 && !tbinfo->ispartition &&
+                       if (tbinfo->ispartition && !dopt->binary_upgrade)
+                       {
+                               appendPQExpBufferStr(q, "\n");
+                               appendPQExpBufferStr(q, tbinfo->partbound);
+                       }
+
+                       /* Emit the INHERITS clause, except if this is a partition. */
+                       if (numParents > 0 &&
+                               !tbinfo->ispartition &&
                                !dopt->binary_upgrade)
                        {
                                appendPQExpBufferStr(q, "\nINHERITS (");
@@ -15243,16 +15271,30 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
                                appendPQExpBufferStr(q, "::pg_catalog.regclass;\n");
                        }
 
-                       if (numParents > 0 && !tbinfo->ispartition)
+                       if (numParents > 0)
                        {
-                               appendPQExpBufferStr(q, "\n-- For binary upgrade, set up inheritance this way.\n");
+                               appendPQExpBufferStr(q, "\n-- For binary upgrade, set up inheritance and partitioning this way.\n");
                                for (k = 0; k < numParents; k++)
                                {
                                        TableInfo  *parentRel = parents[k];
 
-                                       appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT %s;\n",
-                                                                         qualrelname,
-                                                                         fmtQualifiedDumpable(parentRel));
+                                       /* 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));
                                }
                        }
 
@@ -15265,27 +15307,6 @@ 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)
-                               exit_horribly(NULL, "invalid number of parents %d for table \"%s\"\n",
-                                         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
index c1654cf0155fb3ddbc238e6a7bb5079f0d79219d..447f96b66d919ffc55112c2948b2c341687c1700 100644 (file)
@@ -1047,8 +1047,8 @@ 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   => { binary_upgrade => 1, },
+               unlike => {
                        clean                    => 1,
                        clean_if_exists          => 1,
                        createdb                 => 1,
@@ -1057,14 +1057,13 @@ my %tests = (
                        exclude_test_table       => 1,
                        exclude_test_table_data  => 1,
                        no_blobs                 => 1,
-                       no_owner                 => 1,
                        no_privs                 => 1,
+                       no_owner                 => 1,
                        pg_dumpall_dbprivs       => 1,
                        role                     => 1,
                        schema_only              => 1,
                        section_pre_data         => 1,
                        with_oids                => 1,
-               }, unlike => {
                        only_dump_test_schema    => 1,
                        only_dump_test_table     => 1,
                        pg_dumpall_globals       => 1,
@@ -4834,8 +4833,7 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
                        \QCREATE TABLE dump_test_second_schema.measurement_y2006m2 PARTITION OF dump_test.measurement\E\n
                        \QFOR VALUES FROM ('2006-02-01') TO ('2006-03-01');\E\n
                        /xm,
-               like => {},
-               unlike => {
+               like => {
                        clean                    => 1,
                        clean_if_exists          => 1,
                        createdb                 => 1,
@@ -4850,7 +4848,8 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
                        role                     => 1,
                        schema_only              => 1,
                        section_pre_data         => 1,
-                       with_oids                => 1,
+                       with_oids                => 1, },
+               unlike => {
                        binary_upgrade           => 1,
                        only_dump_test_schema    => 1,
                        only_dump_test_table     => 1,