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

... 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

src/bin/pg_dump/pg_dump.c

@@ -8602,12 +8602,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
+ * However, in binary_upgrade mode, we must print all such columns anyway and
- * created independently and ATTACH PARTITION used afterwards.
+ * 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.
  *
  * This function exists because there are scattered nonobvious places that
  * must be kept in sync with this decision.
@@ -8617,9 +8614,7 @@ shouldPrintColumn(DumpOptions *dopt, TableInfo *tbinfo, int colno)
 {
 	if (dopt->binary_upgrade)
 		return true;
-	if (tbinfo->attisdropped[colno])
+	return (tbinfo->attislocal[colno] && !tbinfo->attisdropped[colno]);
-		return false;
-	return (tbinfo->attislocal[colno] || tbinfo->ispartition);
 }
 
 
@@ -15570,6 +15565,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)
+				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 */
@@ -15598,9 +15614,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 &&
+					 * Skip column if fully defined by reloftype or the
-						!dopt->binary_upgrade)
+					 * partition parent.
+					 */
+					if ((tbinfo->reloftype || tbinfo->ispartition) &&
+						!has_default && !has_notnull && !dopt->binary_upgrade)
 						continue;
 
 					/* Format properly if not first attr */
@@ -15623,16 +15642,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
+					 * Attribute type
-					 * table ('OF type_name'), but in binary-upgrade mode,
+					 *
-					 * print it in that case too.
+					 * 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]);
@@ -15689,20 +15712,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,
+				 * We must have a parenthesized attribute list, even though
-				 * even though empty, when not using the OF TYPE syntax.
+				 * empty, when not using the OF TYPE or PARTITION OF syntax.
 				 */
 				appendPQExpBufferStr(q, " (\n)");
 			}
 
-			/*
+			if (tbinfo->ispartition && !dopt->binary_upgrade)
-			 * Emit the INHERITS clause (not for partitions), except in
+			{
-			 * binary-upgrade mode.
+				appendPQExpBufferChar(q, '\n');
-			 */
+				appendPQExpBufferStr(q, tbinfo->partbound);
-			if (numParents > 0 && !tbinfo->ispartition &&
+			}
+
+			/* Emit the INHERITS clause, except if this is a partition. */
+			if (numParents > 0 &&
+				!tbinfo->ispartition &&
 				!dopt->binary_upgrade)
 			{
 				appendPQExpBufferStr(q, "\nINHERITS (");
@@ -15875,16 +15903,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",
+					/* In the partitioning case, we alter the parent */
-									  qualrelname,
+					if (tbinfo->ispartition)
-									  fmtQualifiedDumpable(parentRel));
+						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));
 				}
 			}
 
@@ -15897,27 +15939,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)
-				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

src/bin/pg_dump/t/002_pg_dump.pl

@@ -733,12 +733,7 @@ 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 => {
+		like => { binary_upgrade => 1, },
-			%full_runs,
-			role             => 1,
-			section_pre_data => 1,
-			binary_upgrade   => 1,
-		},
 	  },
 
 	'ALTER TABLE test_table CLUSTER ON test_table_pkey' => {
@@ -2327,13 +2322,12 @@ 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' => {