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

alvherre · alvherre · commit 40353bcc67cd · 2019-05-06T12:23:49.000-04:00
... 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
diff --git a/doc/src/sgml/release-10.sgml b/doc/src/sgml/release-10.sgml
@@ -441,23 +441,6 @@
      </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
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
@@ -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
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -1047,8 +1047,8 @@
 			\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 @@
 			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 @@
 			\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 @@
 			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,
