From e702603beba8943efcda66c9b751d5ee56ad9084 Mon Sep 17 00:00:00 2001 From: Nitin Motiani 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 +#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