Thanks Fujii Masao for the review and feedback. On Fri, 17 Jul 2026 at 16:43, Fujii Masao <[email protected]> wrote: > > On Tue, Jul 14, 2026 at 5:31 PM Mahendra Singh Thalor > <[email protected]> wrote: > > Attached are two small, independent patches that redo that conversion on > > top of current master: > > Thanks for the patches! > > > > 0001 - pg_restore.c: converts its *-only / --no-* / --statistics / > > --clean / --single-transaction conflict checks to > > check_mut_excl_opts(). > > Commit 7c8280eeb58 converted the checks for -d/--dbname vs. -f/--file and > -d/--dbname vs. --restrict-key to use check_mut_excl_opts(). But this patch > doesn't seem to restore those conversions. Is that intentional?
I missed this. Fixed. > > > > 0002 - pg_dumpall.c: same conversion for --exclude-database, the *-only > > options, their --no-* counterparts, --statistics, and --clean. > > Also tracks -s/--schema-only locally (schema_only), which > > pg_dumpall previously just passed through to pg_dump without > > recording, since check_mut_excl_opts() needs it. Updates the > > expected error text in the TAP tests to match > > check_mut_excl_opts()'s "options X and Y cannot be used together" > > wording, and adds coverage for a couple of option pairs that > > previously had no dedicated test. > > + /* --exclude-database is incompatible with global *-only options */ > + check_mut_excl_opts(database_exclude_patterns.head, "--exclude-database", > + globals_only, "-g/--globals-only", Agreed. Fixed. > > database_exclude_patterns.head is a pointer, but > check_mut_excl_opts_internal() > reads the corresponding varargs value as an int. Passing a pointer and reading > it as an int doesn't seem correct. How about passing a boolean instead, e.g.: > > check_mut_excl_opts(database_exclude_patterns.head != NULL, > > There seems to be a similar existing call in pg_dump.c using > foreign_servers_include_patterns.head, so it would be good to fix > that as well. Fixed. > Regards, > > -- > Fujii Masao Here, I am attaching 3 patches for the review. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
From ea9736f5209ec800ec8ff576f1251e5e5bf1a22b Mon Sep 17 00:00:00 2001 From: Mahendra Singh Thalor <[email protected]> Date: Mon, 20 Jul 2026 15:27:00 +0530 Subject: [PATCH v2] pg_restore: Use check_mut_excl_opts() for mutually-exclusive options Replace the ad hoc pairwise if/pg_fatal() and if/pg_log_error() checks for conflicting command-line options with calls to the shared check_mut_excl_opts() helper, matching the style already used in pg_dump.c. This covers -d/--dbname vs -f/--file, -d/--dbname vs --restrict-key, the *-only options, their --no-* counterparts, --statistics vs --no-statistics, --statistics vs *-only, --clean vs --data-only, --single-transaction vs --transaction-size, and -C vs --single-transaction. No behavior change; error message wording and option-reporting order are preserved exactly. --- src/bin/pg_dump/pg_restore.c | 94 ++++++++++++++---------------------- 1 file changed, 36 insertions(+), 58 deletions(-) diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c index 84b8d410c9e..c9c358a3657 100644 --- a/src/bin/pg_dump/pg_restore.c +++ b/src/bin/pg_dump/pg_restore.c @@ -348,22 +348,15 @@ main(int argc, char **argv) pg_fatal("one of -d/--dbname and -f/--file must be specified"); /* Should get at most one of -d and -f, else user is confused */ - if (opts->cparams.dbname) - { - if (opts->filename) - { - pg_log_error("options %s and %s cannot be used together", - "-d/--dbname", "-f/--file"); - pg_log_error_hint("Try \"%s --help\" for more information.", progname); - exit_nicely(1); - } + check_mut_excl_opts(opts->cparams.dbname, "-d/--dbname", + opts->filename, "-f/--file"); - if (opts->restrict_key) - pg_fatal("options %s and %s cannot be used together", - "-d/--dbname", "--restrict-key"); + /* --dbname and --restrict-key are incompatible */ + check_mut_excl_opts(opts->cparams.dbname, "-d/--dbname", + opts->restrict_key, "--restrict-key"); + if (opts->cparams.dbname) opts->useDB = 1; - } else { /* @@ -377,56 +370,41 @@ main(int argc, char **argv) pg_fatal("invalid restrict key"); } - /* reject conflicting "-only" options */ - if (data_only && schema_only) - pg_fatal("options %s and %s cannot be used together", - "-s/--schema-only", "-a/--data-only"); - if (schema_only && statistics_only) - pg_fatal("options %s and %s cannot be used together", - "-s/--schema-only", "--statistics-only"); - if (data_only && statistics_only) - pg_fatal("options %s and %s cannot be used together", - "-a/--data-only", "--statistics-only"); - - /* reject conflicting "-only" and "no-" options */ - if (data_only && no_data) - pg_fatal("options %s and %s cannot be used together", - "-a/--data-only", "--no-data"); - if (schema_only && no_schema) - pg_fatal("options %s and %s cannot be used together", - "-s/--schema-only", "--no-schema"); - if (statistics_only && no_statistics) - pg_fatal("options %s and %s cannot be used together", - "--statistics-only", "--no-statistics"); - - /* reject conflicting "no-" options */ - if (with_statistics && no_statistics) - pg_fatal("options %s and %s cannot be used together", - "--statistics", "--no-statistics"); - - /* reject conflicting "only-" options */ - if (data_only && with_statistics) - pg_fatal("options %s and %s cannot be used together", - "-a/--data-only", "--statistics"); - if (schema_only && with_statistics) - pg_fatal("options %s and %s cannot be used together", - "-s/--schema-only", "--statistics"); - - if (data_only && opts->dropSchema) - pg_fatal("options %s and %s cannot be used together", - "-c/--clean", "-a/--data-only"); - - if (opts->single_txn && opts->txn_size > 0) - pg_fatal("options %s and %s cannot be used together", - "-1/--single-transaction", "--transaction-size"); + /* *-only options are incompatible with each other */ + check_mut_excl_opts(schema_only, "-s/--schema-only", + data_only, "-a/--data-only", + statistics_only, "--statistics-only"); + + /* --no-* and *-only for same thing are incompatible */ + check_mut_excl_opts(data_only, "-a/--data-only", + no_data, "--no-data"); + check_mut_excl_opts(schema_only, "-s/--schema-only", + no_schema, "--no-schema"); + check_mut_excl_opts(statistics_only, "--statistics-only", + no_statistics, "--no-statistics"); + + /* --statistics and --no-statistics are incompatible */ + check_mut_excl_opts(with_statistics, "--statistics", + no_statistics, "--no-statistics"); + + /* --statistics is incompatible with *-only (except --statistics-only) */ + check_mut_excl_opts(data_only, "-a/--data-only", + schema_only, "-s/--schema-only", + with_statistics, "--statistics"); + + /* --clean is incompatible with --data-only */ + check_mut_excl_opts(opts->dropSchema, "-c/--clean", + data_only, "-a/--data-only"); + + check_mut_excl_opts(opts->single_txn, "-1/--single-transaction", + opts->txn_size > 0, "--transaction-size"); /* * -C is not compatible with -1, because we can't create a database inside * a transaction block. */ - if (opts->createDB && opts->single_txn) - pg_fatal("options %s and %s cannot be used together", - "-C/--create", "-1/--single-transaction"); + check_mut_excl_opts(opts->createDB, "-C/--create", + opts->single_txn, "-1/--single-transaction"); /* Can't do single-txn mode with multiple connections */ if (opts->single_txn && numWorkers > 1) -- 2.52.0
From eab87a8f4d86cda107927c4c4f633bfbd7f325c4 Mon Sep 17 00:00:00 2001 From: Mahendra Singh Thalor <[email protected]> Date: Mon, 20 Jul 2026 15:31:16 +0530 Subject: [PATCH v2] pg_dumpall: Use check_mut_excl_opts() for mutually-exclusive options Replace the ad hoc if/pg_log_error() and if/pg_fatal() checks for conflicting command-line options with calls to the shared check_mut_excl_opts() helper, matching the style already used in pg_dump.c and pg_restore.c. This covers --exclude-database vs the global *-only options, the *-only options against each other, their --no-* counterparts, --statistics vs --no-statistics, --statistics vs *-only, and --clean vs --data-only. Track -s/--schema-only locally (as schema_only) so it can participate in these checks, matching how the other *-only options are already tracked. Update the expected error text in the TAP tests to match check_mut_excl_opts()'s "options X and Y cannot be used together" wording, and add coverage for the option pairs that previously had no dedicated test. --- src/bin/pg_dump/pg_dumpall.c | 77 +++++++++++---------- src/bin/pg_dump/t/001_basic.pl | 34 ++++++++- src/bin/pg_dump/t/005_pg_dump_filterfile.pl | 4 +- 3 files changed, 73 insertions(+), 42 deletions(-) diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index c53e77c2878..aee76bb4761 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -27,6 +27,7 @@ #include "common/string.h" #include "connectdb.h" #include "dumputils.h" +#include "fe_utils/option_utils.h" #include "fe_utils/string_utils.h" #include "filter.h" #include "getopt_long.h" @@ -201,6 +202,7 @@ main(int argc, char *argv[]) bool data_only = false; bool globals_only = false; bool roles_only = false; + bool schema_only = false; bool tablespaces_only = false; PGconn *conn; int encoding; @@ -299,6 +301,7 @@ main(int argc, char *argv[]) break; case 's': + schema_only = true; appendPQExpBufferStr(pgdumpopts, " -s"); break; @@ -395,50 +398,48 @@ main(int argc, char *argv[]) exit_nicely(1); } - if (database_exclude_patterns.head != NULL && - (globals_only || roles_only || tablespaces_only)) - { - pg_log_error("option %s cannot be used together with %s, %s, or %s", - "--exclude-database", - "-g/--globals-only", "-r/--roles-only", "-t/--tablespaces-only"); - pg_log_error_hint("Try \"%s --help\" for more information.", progname); - exit_nicely(1); - } + /* --exclude-database is incompatible with global *-only options */ + check_mut_excl_opts(database_exclude_patterns.head != NULL, "--exclude-database", + globals_only, "-g/--globals-only", + roles_only, "-r/--roles-only", + tablespaces_only, "-t/--tablespaces-only"); + + /* *-only options are incompatible with each other */ + check_mut_excl_opts(data_only, "-a/--data-only", + globals_only, "-g/--globals-only", + roles_only, "-r/--roles-only", + schema_only, "-s/--schema-only", + statistics_only, "--statistics-only", + tablespaces_only, "-t/--tablespaces-only"); + + /* --no-* and *-only for same thing are incompatible */ + check_mut_excl_opts(data_only, "-a/--data-only", + no_data, "--no-data"); + check_mut_excl_opts(schema_only, "-s/--schema-only", + no_schema, "--no-schema"); + check_mut_excl_opts(statistics_only, "--statistics-only", + no_statistics, "--no-statistics"); + + /* --statistics and --no-statistics are incompatible */ + check_mut_excl_opts(with_statistics, "--statistics", + no_statistics, "--no-statistics"); + + /* --statistics is incompatible with *-only (except --statistics-only) */ + check_mut_excl_opts(with_statistics, "--statistics", + data_only, "-a/--data-only", + globals_only, "-g/--globals-only", + roles_only, "-r/--roles-only", + schema_only, "-s/--schema-only", + tablespaces_only, "-t/--tablespaces-only"); - /* Make sure the user hasn't specified a mix of globals-only options */ - if (globals_only && roles_only) - { - pg_log_error("options %s and %s cannot be used together", - "-g/--globals-only", "-r/--roles-only"); - pg_log_error_hint("Try \"%s --help\" for more information.", progname); - exit_nicely(1); - } - - if (globals_only && tablespaces_only) - { - pg_log_error("options %s and %s cannot be used together", - "-g/--globals-only", "-t/--tablespaces-only"); - pg_log_error_hint("Try \"%s --help\" for more information.", progname); - exit_nicely(1); - } + /* --clean and --data-only are incompatible */ + check_mut_excl_opts(output_clean, "-c/--clean", + data_only, "-a/--data-only"); if (if_exists && !output_clean) pg_fatal("option %s requires option %s", "--if-exists", "-c/--clean"); - /* --clean and --data-only are incompatible */ - if (output_clean && data_only) - pg_fatal("options %s and %s cannot be used together", - "-c/--clean", "-a/--data-only"); - - if (roles_only && tablespaces_only) - { - pg_log_error("options %s and %s cannot be used together", - "-r/--roles-only", "-t/--tablespaces-only"); - pg_log_error_hint("Try \"%s --help\" for more information.", progname); - exit_nicely(1); - } - /* * If password values are not required in the dump, switch to using * pg_roles which is equally useful, just more likely to have unrestricted diff --git a/src/bin/pg_dump/t/001_basic.pl b/src/bin/pg_dump/t/001_basic.pl index b2558046224..d89ba4a4ceb 100644 --- a/src/bin/pg_dump/t/001_basic.pl +++ b/src/bin/pg_dump/t/001_basic.pl @@ -246,8 +246,38 @@ command_fails_like( # also fails for -r and -t, but it seems pointless to add more tests for those. command_fails_like( [ 'pg_dumpall', '--exclude-database=foo', '--globals-only' ], - qr/\Qpg_dumpall: error: option --exclude-database cannot be used together with -g\/--globals-only\E/, - 'pg_dumpall: option --exclude-database cannot be used together with -g/--globals-only' + qr/\Qpg_dumpall: error: options --exclude-database and -g\/--globals-only cannot be used together\E/, + 'pg_dumpall: options --exclude-database and -g/--globals-only cannot be used together' +); + +command_fails_like( + [ 'pg_dumpall', '-a', '--no-data' ], + qr/\Qpg_dumpall: error: options -a\/--data-only and --no-data cannot be used together\E/, + 'pg_dumpall: options -a\/--data-only and --no-data cannot be used together' +); + +command_fails_like( + [ 'pg_dumpall', '-s', '--no-schema' ], + qr/\Qpg_dumpall: error: options -s\/--schema-only and --no-schema cannot be used together\E/, + 'pg_dumpall: options -s\/--schema-only and --no-schema cannot be used together' +); + +command_fails_like( + [ 'pg_dumpall', '--statistics-only', '--no-statistics' ], + qr/\Qpg_dumpall: error: options --statistics-only and --no-statistics cannot be used together\E/, + 'pg_dumpall: options --statistics-only and --no-statistics cannot be used together' +); + +command_fails_like( + [ 'pg_dumpall', '--statistics', '--no-statistics' ], + qr/\Qpg_dumpall: error: options --statistics and --no-statistics cannot be used together\E/, + 'pg_dumpall: options --statistics-only and --no-statistics cannot be used together' +); + +command_fails_like( + [ 'pg_dumpall', '--statistics', '--tablespaces-only' ], + qr/\Qpg_dumpall: error: options --statistics and -t\/--tablespaces-only cannot be used together\E/, + 'pg_dumpall: options --statistics and -t\/--tablespaces-only cannot be used together' ); done_testing(); diff --git a/src/bin/pg_dump/t/005_pg_dump_filterfile.pl b/src/bin/pg_dump/t/005_pg_dump_filterfile.pl index 72a7c90c6af..cecf0442088 100644 --- a/src/bin/pg_dump/t/005_pg_dump_filterfile.pl +++ b/src/bin/pg_dump/t/005_pg_dump_filterfile.pl @@ -570,8 +570,8 @@ command_fails_like( '--filter' => "$tempdir/inputfile.txt", '--globals-only' ], - qr/\Qpg_dumpall: error: option --exclude-database cannot be used together with -g\/--globals-only\E/, - 'pg_dumpall: option --exclude-database cannot be used together with -g/--globals-only' + qr/\Qpg_dumpall: error: options --exclude-database and -g\/--globals-only cannot be used together\E/, + 'pg_dumpall: options --exclude-database and -g/--globals-only cannot be used together' ); # Test invalid filter command -- 2.52.0
From ead4bf997d079fa20050c61f17b5348714e02d19 Mon Sep 17 00:00:00 2001 From: Mahendra Singh Thalor <[email protected]> Date: Mon, 20 Jul 2026 15:31:00 +0530 Subject: [PATCH v2] pg_dump: Fix argument type passed to check_mut_excl_opts() check_mut_excl_opts()'s internal varargs reader retrieves each "is this option set" flag via va_arg(args, int). The --include-foreign-data check passed foreign_servers_include_patterns.head (a pointer) directly for that slot, which is not guaranteed to be read back correctly through an int-typed va_arg. Pass a boolean derived from a NULL comparison instead. --- src/bin/pg_dump/pg_dump.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 4948e6d80c7..890479e51eb 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -849,7 +849,7 @@ main(int argc, char **argv) schema_only, "-s/--schema-only"); /* --include-foreign-data is incompatible with --schema-only */ - check_mut_excl_opts(foreign_servers_include_patterns.head, "--include-foreign-data", + check_mut_excl_opts(foreign_servers_include_patterns.head != NULL, "--include-foreign-data", schema_only, "-s/--schema-only"); if (numWorkers > 1 && foreign_servers_include_patterns.head != NULL) -- 2.52.0
