Fix pg_dump for hash partitioning on enum columns.

Hash partitioning on an enum is problematic because the hash codes are
derived from the OIDs assigned to the enum values, which will almost
certainly be different after a dump-and-reload than they were before.
This means that some rows probably end up in different partitions than
before, causing restore to fail because of partition constraint
violations.  (pg_upgrade dodges this problem by using hacks to force
the enum values to keep the same OIDs, but that's not possible nor
desirable for pg_dump.)

Users can work around that by specifying --load-via-partition-root,
but since that's a dump-time not restore-time decision, one might
find out the need for it far too late.  Instead, teach pg_dump to
apply that option automatically when dealing with a partitioned
table that has hash-on-enum partitioning.

Also deal with a pre-existing issue for --load-via-partition-root
mode: in a parallel restore, we try to TRUNCATE target tables just
before loading them, in order to enable some backend optimizations.
This is bad when using --load-via-partition-root because (a) we're
likely to suffer deadlocks from restore jobs trying to restore rows
into other partitions than they came from, and (b) if we miss getting
a deadlock we might still lose data due to a TRUNCATE removing rows
from some already-completed restore job.

The fix for this is conceptually simple: just don't TRUNCATE if we're
dealing with a --load-via-partition-root case.  The tricky bit is for
pg_restore to identify those cases.  In dumps using COPY commands we
can inspect each COPY command to see if it targets the nominal target
table or some ancestor.  However, in dumps using INSERT commands it's
pretty impractical to examine the INSERTs in advance.  To provide a
solution for that going forward, modify pg_dump to mark TABLE DATA
items that are using --load-via-partition-root with a comment.
(This change also responds to a complaint from Robert Haas that
the dump output for --load-via-partition-root is pretty confusing.)
pg_restore checks for the special comment as well as checking the
COPY command if present.  This will fail to identify the combination
of --load-via-partition-root and --inserts in pre-existing dump files,
but that should be a pretty rare case in the field.  If it does
happen you will probably get a deadlock failure that you can work
around by not using parallel restore, which is the same as before
this bug fix.

Having done this, there seems no remaining reason for the alarmism
in the pg_dump man page about combining --load-via-partition-root
with parallel restore, so remove that warning.

Patch by me; thanks to Julien Rouhaud for review.  Back-patch to
v11 where hash partitioning was introduced.

Discussion: https://postgr.es/m/[email protected]

Author: tglsfdc

doc/src/sgml/ref/pg_dump.sgml

@@ -863,16 +863,6 @@ PostgreSQL documentation
         and the two systems have different definitions of the collation used
         to sort the partitioning column.
        </para>
-
-       <para>
-        It is best not to use parallelism when restoring from an archive made
-        with this option, because <application>pg_restore</application> will
-        not know exactly which partition(s) a given archive data item will
-        load data into.  This could result in inefficiency due to lock
-        conflicts between parallel jobs, or perhaps even reload failures due
-        to foreign key constraints being set up before all the relevant data
-        is loaded.
-       </para>
       </listitem>
      </varlistentry>
 

doc/src/sgml/ref/pg_dumpall.sgml

@@ -395,10 +395,6 @@ PostgreSQL documentation
         and the two systems have different definitions of the collation used
         to sort the partitioning column.
        </para>
-
-       <!-- Currently, we don't need pg_dump's warning about parallelism here,
-        since parallel restore from a pg_dumpall script is impossible.
-       -->
       </listitem>
      </varlistentry>
 

src/bin/pg_dump/common.c

@@ -250,6 +250,9 @@ getSchemaData(Archive *fout, int *numTablesPtr)
 	pg_log_info("flagging inherited columns in subtables");
 	flagInhAttrs(fout->dopt, tblinfo, numTables);
 
+	pg_log_info("reading partitioning data");
+	getPartitioningInfo(fout);
+
 	pg_log_info("reading indexes");
 	getIndexes(fout, tblinfo, numTables);
 
@@ -302,7 +305,6 @@ static void
 flagInhTables(Archive *fout, TableInfo *tblinfo, int numTables,
 			  InhInfo *inhinfo, int numInherits)
 {
-	DumpOptions *dopt = fout->dopt;
 	int			i,
 				j;
 
@@ -318,26 +320,18 @@ flagInhTables(Archive *fout, TableInfo *tblinfo, int numTables,
 			continue;
 
 		/*
-		 * FIXME: In PostgreSQL, foreign tables can be inherited. But
-		 * pg_dump chokes on external tables, if an external table is
-		 * used as a partition, and a column has attislocal=false.
-		 */
-		if (tblinfo[i].relstorage == 'x' /* RELSTORAGE_EXTERNAL */)
-			continue;
-
-		/*
-		 * Normally, we don't bother computing anything for non-target tables,
-		 * but if load-via-partition-root is specified, we gather information
-		 * on every partition in the system so that getRootTableInfo can trace
-		 * from any given to leaf partition all the way up to the root.  (We
-		 * don't need to mark them as interesting for getTableAttrs, though.)
+		 * Normally, we don't bother computing anything for non-target tables.
+		 * However, we must find the parents of non-root partitioned tables in
+		 * any case, so that we can trace from leaf partitions up to the root
+		 * (in case a leaf is to be dumped but its parents are not).  We need
+		 * not mark such parents interesting for getTableAttrs, though.
 		 */
 		if (!tblinfo[i].dobj.dump)
 		{
 			mark_parents = false;
 
-			if (!dopt->load_via_partition_root ||
-				!tblinfo[i].ispartition)
+			if (!(tblinfo[i].relkind == RELKIND_PARTITIONED_TABLE &&
+				  tblinfo[i].ispartition))
 				find_parents = false;
 		}
 

src/bin/pg_dump/meson.build

@@ -0,0 +1,105 @@
+# Copyright (c) 2022-2023, PostgreSQL Global Development Group
+
+pg_dump_common_sources = files(
+  'compress_gzip.c',
+  'compress_io.c',
+  'compress_lz4.c',
+  'compress_none.c',
+  'dumputils.c',
+  'parallel.c',
+  'pg_backup_archiver.c',
+  'pg_backup_custom.c',
+  'pg_backup_db.c',
+  'pg_backup_directory.c',
+  'pg_backup_null.c',
+  'pg_backup_tar.c',
+  'pg_backup_utils.c',
+)
+
+pg_dump_common = static_library('libpgdump_common',
+  pg_dump_common_sources,
+  c_pch: pch_postgres_fe_h,
+  dependencies: [frontend_code, libpq, lz4, zlib],
+  kwargs: internal_lib_args,
+)
+
+
+pg_dump_sources = files(
+  'common.c',
+  'pg_dump.c',
+  'pg_dump_sort.c',
+)
+
+if host_system == 'windows'
+  pg_dump_sources += rc_bin_gen.process(win32ver_rc, extra_args: [
+    '--NAME', 'pg_dump',
+    '--FILEDESC', 'pg_dump - backup one PostgreSQL database',])
+endif
+
+pg_dump = executable('pg_dump',
+  pg_dump_sources,
+  link_with: [pg_dump_common],
+  dependencies: [frontend_code, libpq, zlib],
+  kwargs: default_bin_args,
+)
+bin_targets += pg_dump
+
+
+pg_dumpall_sources = files(
+  'pg_dumpall.c',
+)
+
+if host_system == 'windows'
+  pg_dumpall_sources += rc_bin_gen.process(win32ver_rc, extra_args: [
+    '--NAME', 'pg_dumpall',
+    '--FILEDESC', 'pg_dumpall - backup PostgreSQL databases'])
+endif
+
+pg_dumpall = executable('pg_dumpall',
+  pg_dumpall_sources,
+  link_with: [pg_dump_common],
+  dependencies: [frontend_code, libpq, zlib],
+  kwargs: default_bin_args,
+)
+bin_targets += pg_dumpall
+
+
+pg_restore_sources = files(
+  'pg_restore.c',
+)
+
+if host_system == 'windows'
+  pg_restore_sources += rc_bin_gen.process(win32ver_rc, extra_args: [
+    '--NAME', 'pg_restore',
+    '--FILEDESC', 'pg_restore - restore PostgreSQL databases'])
+endif
+
+pg_restore = executable('pg_restore',
+  pg_restore_sources,
+  link_with: [pg_dump_common],
+  dependencies: [frontend_code, libpq, zlib],
+  kwargs: default_bin_args,
+)
+bin_targets += pg_restore
+
+tests += {
+  'name': 'pg_dump',
+  'sd': meson.current_source_dir(),
+  'bd': meson.current_build_dir(),
+  'tap': {
+    'env': {
+      'GZIP_PROGRAM': gzip.path(),
+      'LZ4': program_lz4.found() ? program_lz4.path() : '',
+      'with_icu': icu.found() ? 'yes' : 'no',
+    },
+    'tests': [
+      't/001_basic.pl',
+      't/002_pg_dump.pl',
+      't/003_pg_dump_with_server.pl',
+      't/004_pg_dump_parallel.pl',
+      't/010_dump_connstr.pl',
+    ],
+  },
+}
+
+subdir('po', if_found: libintl)

src/bin/pg_dump/pg_backup_archiver.c

@@ -93,6 +93,7 @@ static RestorePass _tocEntryRestorePass(TocEntry *te);
 static bool _tocEntryIsACL(TocEntry *te);
 static void _disableTriggersIfNecessary(ArchiveHandle *AH, TocEntry *te);
 static void _enableTriggersIfNecessary(ArchiveHandle *AH, TocEntry *te);
+static bool is_load_via_partition_root(TocEntry *te);
 static void buildTocEntryArrays(ArchiveHandle *AH);
 static void _moveBefore(TocEntry *pos, TocEntry *te);
 static int	_discoverArchiveFormat(ArchiveHandle *AH);
@@ -892,6 +893,8 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
 				}
 				else
 				{
+					bool		use_truncate;
+
 					_disableTriggersIfNecessary(AH, te);
 
 					/* Select owner and schema as necessary */
@@ -903,13 +906,24 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
 
 					/*
 					 * In parallel restore, if we created the table earlier in
-					 * the run then we wrap the COPY in a transaction and
-					 * precede it with a TRUNCATE.  If archiving is not on
-					 * this prevents WAL-logging the COPY.  This obtains a
-					 * speedup similar to that from using single_txn mode in
-					 * non-parallel restores.
+					 * this run (so that we know it is empty) and we are not
+					 * restoring a load-via-partition-root data item then we
+					 * wrap the COPY in a transaction and precede it with a
+					 * TRUNCATE.  If wal_level is set to minimal this prevents
+					 * WAL-logging the COPY.  This obtains a speedup similar
+					 * to that from using single_txn mode in non-parallel
+					 * restores.
+					 *
+					 * We mustn't do this for load-via-partition-root cases
+					 * because some data might get moved across partition
+					 * boundaries, risking deadlock and/or loss of previously
+					 * loaded data.  (We assume that all partitions of a
+					 * partitioned table will be treated the same way.)
 					 */
-					if (is_parallel && te->created)
+					use_truncate = is_parallel && te->created &&
+						!is_load_via_partition_root(te);
+
+					if (use_truncate)
 					{
 						/*
 						 * Parallel restore is always talking directly to a
@@ -950,7 +964,7 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
 					AH->outputKind = OUTPUT_SQLCMDS;
 
 					/* close out the transaction started above */
-					if (is_parallel && te->created)
+					if (use_truncate)
 						CommitTransaction(&AH->public);
 
 					_enableTriggersIfNecessary(AH, te);
@@ -1048,6 +1062,43 @@ _enableTriggersIfNecessary(ArchiveHandle *AH, TocEntry *te)
 			 fmtQualifiedId(te->namespace, te->tag));
 }
 
+/*
+ * Detect whether a TABLE DATA TOC item is performing "load via partition
+ * root", that is the target table is an ancestor partition rather than the
+ * table the TOC item is nominally for.
+ *
+ * In newer archive files this can be detected by checking for a special
+ * comment placed in te->defn.  In older files we have to fall back to seeing
+ * if the COPY statement targets the named table or some other one.  This
+ * will not work for data dumped as INSERT commands, so we could give a false
+ * negative in that case; fortunately, that's a rarely-used option.
+ */
+static bool
+is_load_via_partition_root(TocEntry *te)
+{
+	if (te->defn &&
+		strncmp(te->defn, "-- load via partition root ", 27) == 0)
+		return true;
+	if (te->copyStmt && *te->copyStmt)
+	{
+		PQExpBuffer copyStmt = createPQExpBuffer();
+		bool		result;
+
+		/*
+		 * Build the initial part of the COPY as it would appear if the
+		 * nominal target table is the actual target.  If we see anything
+		 * else, it must be a load-via-partition-root case.
+		 */
+		appendPQExpBuffer(copyStmt, "COPY %s ",
+						  fmtQualifiedId(te->namespace, te->tag));
+		result = strncmp(te->copyStmt, copyStmt->data, copyStmt->len) != 0;
+		destroyPQExpBuffer(copyStmt);
+		return result;
+	}
+	/* Assume it's not load-via-partition-root */
+	return false;
+}
+
 /*
  * This is a routine that is part of the dumper interface, hence the 'Archive*' parameter.
  */
@@ -3020,8 +3071,12 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
 			res = res & ~REQ_DATA;
 	}
 
-	/* If there's no definition command, there's no schema component */
-	if (!te->defn || !te->defn[0])
+	/*
+	 * If there's no definition command, there's no schema component.  Treat
+	 * "load via partition root" comments as not schema.
+	 */
+	if (!te->defn || !te->defn[0] ||
+		strncmp(te->defn, "-- load via partition root ", 27) == 0)
 		res = res & ~REQ_SCHEMA;
 
 	/*