pg_dumpall: Fix handling of conflicting options.

pg_dumpall is missing checks for some conflicting options,
including those passed through to pg_dump.  To fix, introduce a
new function that checks whether mutually exclusive options are
set, and use that in pg_dumpall.  A similar change could likely be
made for pg_dump and pg_restore, but that is left as a future
exercise.

This is arguably a bug fix, but since this might break existing
scripts, no back-patch for now.

Author: Jian He <jian.universality@gmail.com>
Co-authored-by: Nathan Bossart <nathandbossart@gmail.com>
Reviewed-by: Wang Peng <215722532@qq.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CACJufxFf5%3DwSv2MsuO8iZOvpLZQ1-meAMwhw7JX5gNvWo5PDug%40mail.gmail.com

diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 56e25f9b1562627d5ff035025f1bcec575708569..4ded902095288b607eb1cba5436355273dca29f5 100644 (file)
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -34,6 +34,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"
@@ -219,6 +220,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;
@@ -321,6 +323,7 @@ main(int argc, char *argv[])
                                break;
 
                        case 's':
+                               schema_only = true;
                                appendPQExpBufferStr(pgdumpopts, " -s");
                                break;
 
@@ -418,45 +421,44 @@ 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);
-       }
-
-       /* 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);
-       }
+       /* --exclude_database is incompatible with global *-only options */
+       check_mut_excl_opts(database_exclude_patterns.head, "--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");
 
        if (if_exists && !output_clean)
                pg_fatal("option %s requires option %s",
                                 "--if-exists", "-c/--clean");
 
-       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);
-       }
-
        /* Get format for dump. */
        archDumpFormat = parseDumpFormat(format_name);
 

diff --git a/src/bin/pg_dump/t/001_basic.pl b/src/bin/pg_dump/t/001_basic.pl
index 930ef4915c5280f3de2f7752bf9d80fb93a32716..67131a674f4466c5faf3480cec57815b5fd3fd58 100644 (file)
--- a/src/bin/pg_dump/t/001_basic.pl
+++ b/src/bin/pg_dump/t/001_basic.pl
@@ -240,8 +240,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'
 );
 
 command_fails_like(

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 588891a62bd6b6a307100e9f5e79ab4f647e2257..e7cc998cfbad273005c2d30665ec53c5cbb244e6 100644 (file)
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -322,7 +322,6 @@ my %pgdump_runs = (
                        '--file' => "$tempdir/pg_dumpall_globals.sql",
                        '--globals-only',
                        '--no-sync',
-                       '--statistics',
                ],
        },
        pg_dumpall_globals_clean => {
@@ -332,7 +331,6 @@ my %pgdump_runs = (
                        '--globals-only',
                        '--clean',
                        '--no-sync',
-                       '--statistics',
                ],
        },
        pg_dumpall_dbprivs => {

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 9c9f2baa733d669d30f0930f8e10b33e2b4673fd..b2630ef289733bf9aea0f4e40b969af15b88214b 100644 (file)
--- a/src/bin/pg_dump/t/005_pg_dump_filterfile.pl
+++ b/src/bin/pg_dump/t/005_pg_dump_filterfile.pl
@@ -571,8 +571,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

diff --git a/src/fe_utils/option_utils.c b/src/fe_utils/option_utils.c
index cc483ae176c72a728807770bdc46b8a05234c214..8d0659c1164d3a1f8516ee18fa99f1c11e6f2192 100644 (file)
--- a/src/fe_utils/option_utils.c
+++ b/src/fe_utils/option_utils.c
@@ -109,3 +109,38 @@ parse_sync_method(const char *optarg, DataDirSyncMethod *sync_method)
 
        return true;
 }
+
+/*
+ * Fail with appropriate error if 2 or more of the specified options are set.
+ * The first parameter is the number of arguments.  Following that is an
+ * arbitrary number of bool/string pairs.  The bool indicates whether the
+ * option is set, and the string is used for the error message.  Note that only
+ * the first pair of enabled options we find are reported.
+ *
+ * Don't call this directly.  Use the check_mut_excl_opts() macro in
+ * option_utils.h, which is the exact same except it doesn't take the first
+ * parameter (it discovers the number of arguments automagically).
+ */
+void
+check_mut_excl_opts_internal(int n,...)
+{
+       char       *first = NULL;
+       va_list         args;
+
+       Assert(n % 2 == 0);
+
+       va_start(args, n);
+       for (int i = 0; i < n; i += 2)
+       {
+               bool            set = va_arg(args, int);
+               char       *opt = va_arg(args, char *);
+
+               if (set && first)
+                       pg_fatal("options %s and %s cannot be used together",
+                                        first, opt);
+
+               if (set)
+                       first = opt;
+       }
+       va_end(args);
+}

diff --git a/src/include/fe_utils/option_utils.h b/src/include/fe_utils/option_utils.h
index 0db6e3b6e91c3ef868e2470d3a66df150b5de9db..d975db77af2228dad0b9e22312d5dad544076ae9 100644 (file)
--- a/src/include/fe_utils/option_utils.h
+++ b/src/include/fe_utils/option_utils.h
@@ -24,5 +24,11 @@ extern bool option_parse_int(const char *optarg, const char *optname,
                                                         int *result);
 extern bool parse_sync_method(const char *optarg,
                                                          DataDirSyncMethod *sync_method);
+extern void check_mut_excl_opts_internal(int n,...);
+
+/* see comment for check_mut_excl_opts_internal() in option_utils.c for info */
+#define check_mut_excl_opts(set, opt, ...) \
+       check_mut_excl_opts_internal(VA_ARGS_NARGS(__VA_ARGS__) + 2, \
+                                                                set, opt, __VA_ARGS__)
 
 #endif                                                 /* OPTION_UTILS_H */