v10-0005-POC-Add-test-in-pg_dump.pl.patch

From e702603beba8943efcda66c9b751d5ee56ad9084 Mon Sep 17 00:00:00 2001
From: Nitin Motiani <nitinmotiani@google.com>
Date: Fri, 1 May 2026 07:49:14 +0000
Subject: [PATCH v10 5/5] [POC] Add test in pg_dump.pl * Make a couple of code
 changes to fix the test. If they pass, we'll rebase.

---
 src/bin/pg_dump/compress_none.c      | 10 ++++++----
 src/bin/pg_dump/pg_backup_archiver.c | 27 +++++++++++++++++++++++++--
 src/bin/pg_dump/pg_dump.c            | 20 ++++++++++----------
 src/bin/pg_dump/t/002_pg_dump.pl     | 19 +++++++++++++++++++
 4 files changed, 60 insertions(+), 16 deletions(-)

diff --git a/src/bin/pg_dump/compress_none.c b/src/bin/pg_dump/compress_none.c
index 8c2f95b520d..9ecdf18eec4 100644
--- a/src/bin/pg_dump/compress_none.c
+++ b/src/bin/pg_dump/compress_none.c
@@ -14,6 +14,7 @@
 #include "postgres_fe.h"
 #include <unistd.h>
 
+#include "port.h"
 #include "compress_none.h"
 #include "pg_backup_utils.h"
 
@@ -210,13 +211,14 @@ close_none(CompressFileHandle *CFH)
 
 	if (fp)
 	{
-		errno = 0;
 		if (CFH->path_is_pipe_command)
-			ret = pclose(fp);
+			ret = pclose_check(fp);
 		else
+		{
 			ret = fclose(fp);
-		if (ret != 0)
-			pg_log_error("could not close file: %m");
+			if (ret != 0)
+				pg_log_error("could not close file: %m");
+		}
 	}
 
 	return ret == 0;
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 4ef9dae49ed..8358bd97df5 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -1744,7 +1744,19 @@ SetOutput(ArchiveHandle *AH, const char *filename,
 	else
 		mode = PG_BINARY_W;
 
-	CFH = InitCompressFileHandle(compression_spec, AH->fSpecIsPipe);
+	/*
+	 * The output handle (usually stdout) should never be a pipe command
+	 * managed by our popen logic, even if the archive itself is a pipe.  Our
+	 * pipe command implementation for directory mode is a template for the
+	 * data files, not for this primary output stream.
+	 *
+	 * Furthermore, marking this as a pipe command would cause it to be closed
+	 * with pclose() instead of fclose().  Since this handle is opened via
+	 * fdopen() (for stdout) or fopen() (for a regular file), using pclose()
+	 * on it is a bug that causes failures on BSD-based systems (like FreeBSD
+	 * or macOS).
+	 */
+	CFH = InitCompressFileHandle(compression_spec, false);
 
 	if (!CFH->open_func(filename, fn, mode, CFH))
 	{
@@ -2463,7 +2475,18 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
 
 	/* Open stdout with no compression for AH output handle */
 	out_compress_spec.algorithm = PG_COMPRESSION_NONE;
-	CFH = InitCompressFileHandle(out_compress_spec, AH->fSpecIsPipe);
+	/*
+	 * The output handle (usually stdout) should never be a pipe command
+	 * managed by our popen logic, even if the archive itself is a pipe.  Our
+	 * pipe command implementation for directory mode is a template for the
+	 * data files, not for this primary output stream.
+	 *
+	 * Furthermore, marking this as a pipe command would cause it to be closed
+	 * with pclose() instead of fclose().  Since this handle is opened via
+	 * fdopen() (for stdout), using pclose() on it is a bug that causes
+	 * failures on BSD-based systems (like FreeBSD or macOS).
+	 */
+	CFH = InitCompressFileHandle(out_compress_spec, false);
 	if (!CFH->open_func(NULL, fileno(stdout), PG_BINARY_A, CFH))
 		pg_fatal("could not open stdout for appending: %m");
 	AH->OF = CFH;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7345e6c7a4b..e4ae64c1495 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -420,6 +420,8 @@ main(int argc, char **argv)
 	int			c;
 	const char *filename = NULL;
 	bool		filename_is_pipe = false;
+	bool		file_specified = false;
+	bool		pipe_specified = false;
 	const char *format = "p";
 	TableInfo  *tblinfo;
 	int			numTables;
@@ -608,11 +610,7 @@ main(int argc, char **argv)
 				break;
 
 			case 'f':
-				if (filename != NULL)
-				{
-					pg_log_error_hint("Only one of [--file, --pipe-command] allowed");
-					exit_nicely(1);
-				}
+				file_specified = true;
 				filename = pg_strdup(optarg);
 				filename_is_pipe = false;	/* it already is, setting again
 											 * here just for clarity */
@@ -809,11 +807,7 @@ main(int argc, char **argv)
 				break;
 
 			case 26:			/* pipe command */
-				if (filename != NULL)
-				{
-					pg_log_error_hint("Only one of [--file, --pipe-command] allowed");
-					exit_nicely(1);
-				}
+				pipe_specified = true;
 				filename = pg_strdup(optarg);
 				filename_is_pipe = true;
 				break;
@@ -825,6 +819,12 @@ main(int argc, char **argv)
 		}
 	}
 
+	if (file_specified && pipe_specified)
+	{
+		pg_log_error_hint("Only one of [--file, --pipe-command] allowed");
+		exit_nicely(1);
+	}
+
 	/*
 	 * Non-option argument specifies database name as long as it wasn't
 	 * already specified with -d / --dbname
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 3bc8e51561d..06b2f2619e2 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -223,6 +223,25 @@ my %pgdump_runs = (
 		],
 	},
 
+	# This test kept failing.
+	defaults_dir_format_pipe => {
+		test_key => 'defaults',
+		dump_cmd => [
+			'pg_dump',
+			'--format' => 'directory',
+			'--pipe-command' => "cat > $tempdir/defaults_dir_format/%f",
+			'--statistics',
+			'postgres',
+		],
+	restore_cmd => [
+			'pg_restore',
+			'--format' => 'directory',
+			'--file' => "$tempdir/defaults_dir_format_pipe.sql",
+			'--statistics',
+			"$tempdir/defaults_dir_format",
+		],
+	},
+
 	# Do not use --no-sync to give test coverage for data sync.
 	defaults_parallel => {
 		test_key => 'defaults',
-- 
2.54.0.545.g6539524ca2-goog