Обсуждение: pg_dumpall --roles-only interact with other options
hi. pg_dumpall --verbose --roles-only --no-schema --file=1.sql pg_dumpall --verbose --roles-only --no-data --file=2.sql pg_dumpall --verbose --roles-only --no-statistics --file=3.sql pg_dumpall --verbose --roles-only --statistics-only --file=4.sql pg_dumpall --verbose --roles-only --data-only --file=5.sql pg_dumpall --verbose --roles-only --schema-only --file=6.sql What should we expect for the above commands? the current behavior is not good, i think, some even do not dump the roles command. I would expect the last three commands to raise errors, while the first three should simply ignore the options (--no-schema, --no-data, --no-statistics). This situation also happens to another pg_duampall option: --tablespaces-only. what do you think? -- jian https://www.enterprisedb.com/
On Sat, Jan 31, 2026 at 11:50:50AM +0800, jian he wrote: > pg_dumpall --verbose --roles-only --no-schema --file=1.sql > pg_dumpall --verbose --roles-only --no-data --file=2.sql > pg_dumpall --verbose --roles-only --no-statistics --file=3.sql These seem permissible to me. The --no-* options are redundant, but the user intent seems clear. > pg_dumpall --verbose --roles-only --statistics-only --file=4.sql > pg_dumpall --verbose --roles-only --data-only --file=5.sql > pg_dumpall --verbose --roles-only --schema-only --file=6.sql > > [...] > > This situation also happens to another pg_duampall option: > --tablespaces-only. Yeah, conflicting --*-only options should probably cause errors, like we do for pg_dump. -- nathan
hi. please check the attached. pg_dumpall --roles-only --statistics-only pg_dumpall --roles-only --data-only pg_dumpall --roles-only --schema-only pg_dumpall --roles-only --statistics pg_dumpall --tablespaces-only --statistics-only pg_dumpall --tablespaces-only --data-only pg_dumpall --tablespaces-only --schema-only pg_dumpall --tablespaces-only --statistics pg_dumpall --globals-only --statistics the above will all error out. ``pg_dumpall --globals-only --statistics`` should error, the HEAD behavior does not respect "--statistics", maybe we can make it not error out, but that would contradict the meaning of "--globals-only", i think. pg_dumpall --roles-only --no-schema --file=1.sql pg_dumpall --roles-only --no-data --file=2.sql pg_dumpall --roles-only --no-statistics --file=3.sql pg_dumpall --tablespaces-only --no-schema --file=1.sql pg_dumpall --tablespaces-only --no-data --file=2.sql pg_dumpall --tablespaces-only --no-statistics --file=3.sql The items listed above respect the 'only' option but ignore the 'no' option." -- jian https://www.enterprisedb.com/
Вложения
On Tue, Feb 03, 2026 at 10:15:35AM +0800, jian he wrote:
> please check the attached.
Thanks.
> - {"statistics", no_argument, &with_statistics, 1},
> - {"statistics-only", no_argument, &statistics_only, 1},
> + {"statistics", no_argument, NULL, 10},
> + {"statistics-only", no_argument, NULL, 11},
nitpick: I don't totally disagree with these changes, but they are
unrelated to the patch at hand, so I think we'd better leave them out.
> + /* reject conflicting "-only" options */
> + if (globals_only && with_statistics)
> + pg_fatal("options %s and %s cannot be used together",
> + "-g/--globals-only", "--statistics");
> +
> + if (data_only && roles_only)
> + pg_fatal("options %s and %s cannot be used together",
> + "-a/--data-only", "-r/--roles-only");
> [...]
> +
> + if (data_only && tablespaces_only)
> + pg_fatal("options %s and %s cannot be used together",
> + "-a/--data-only", "-t/--tablespaces-only");
> [...]
Could we integrate this into the existing handling for conflicting options
a few lines above this point?
> - if (!data_only && !statistics_only && !no_schema)
I wonder if we ought to create "derivative flags" like we did for pg_dump
in commit 96a81c1be9. That could make some of this stuff easier to
maintain and to follow.
> diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
> index a8dcc2b5c75..340cf953a60 100644
> --- 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 => {
Could you add some new tests for the conflicting options?
--
nathan
jian he 写于 2026/2/3 10:15: > hi. > > please check the attached. > > pg_dumpall --roles-only --statistics-only > pg_dumpall --roles-only --data-only > pg_dumpall --roles-only --schema-only > pg_dumpall --roles-only --statistics > pg_dumpall --tablespaces-only --statistics-only > pg_dumpall --tablespaces-only --data-only > pg_dumpall --tablespaces-only --schema-only > pg_dumpall --tablespaces-only --statistics > pg_dumpall --globals-only --statistics > > the above will all error out. > ``pg_dumpall --globals-only --statistics`` should error, > the HEAD behavior does not respect "--statistics", maybe we can make > it not error out, but > that would contradict the meaning of "--globals-only", i think. > > pg_dumpall --roles-only --no-schema --file=1.sql > pg_dumpall --roles-only --no-data --file=2.sql > pg_dumpall --roles-only --no-statistics --file=3.sql > pg_dumpall --tablespaces-only --no-schema --file=1.sql > pg_dumpall --tablespaces-only --no-data --file=2.sql > pg_dumpall --tablespaces-only --no-statistics --file=3.sql > > The items listed above respect the 'only' option but ignore the 'no' option." > > > -- > jian > https://www.enterprisedb.com/ Hi, I reviewed and tested this patch. I noticed that: pg_dumpall --globals-only --statistics ----> error pg_dumpall --globals-only --statistics-only ----> pass maybe there is inconsistent for *-only options is that intentional? Best regards, -- wangpeng
On Wed, Feb 4, 2026 at 9:56 AM wangpeng <215722532@qq.com> wrote: > > Hi, > I reviewed and tested this patch. I noticed that: > pg_dumpall --globals-only --statistics ----> error > pg_dumpall --globals-only --statistics-only ----> pass > maybe there is inconsistent for *-only options > is that intentional? > Thanks for pointing this out. It should fail too. I missed this combination. The attached v2 should be bullet-proof. On Wed, Feb 4, 2026 at 5:25 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > I wonder if we ought to create "derivative flags" like we did for pg_dump > in commit 96a81c1be9. That could make some of this stuff easier to > maintain and to follow. https://git.postgresql.org/cgit/postgresql.git/commit/?id=96a81c1be929d122719bd289f6e24824f37e1ff6 added new fields to RestoreOptions and DumpOptions. These global objects dump(roles, tablespaces) are not directly related to pg_restore for now, pg_restore does not support options like --roles-only or --tablespaces-only. Creating "derivative flags" requires careful consideration of their default values, which adds complexity for relatively little benefit. Overall we don't need to implement similar logic now, i think. commitgest entry: https://commitfest.postgresql.org/patch/6459 -- jian https://www.enterprisedb.com/
Вложения
On Wed, Feb 04, 2026 at 04:14:59PM +0800, jian he wrote:
> These global objects dump(roles, tablespaces) are not directly related to
> pg_restore for now, pg_restore does not support options like --roles-only
> or --tablespaces-only.
I'm suggesting adding derivative flags to pg_dumpall, not pg_restore.
> Creating "derivative flags" requires careful
> consideration of their default values, which adds complexity for relatively
> little benefit. Overall we don't need to implement similar logic now, i think.
I'm not following your objection here. If anything, such a change would
reduce complexity. For example, we currently use the following check in
multiple places to decide whether to drop/drump databases:
if (!globals_only && !roles_only && !tablespaces_only)
If we created a derivative flag like this:
shouldDumpDBs = !globals_only && !roles_only && !tablespaces_only;
We could then decide whether to do database things like this:
if (shouldDumpDBs)
dumpDatabases(conn);
This has the added benefit of simplifying future patches that add new -only
options. If/when that happens, we'd just add it to the line that sets
shouldDumpDBs, whereas today we'd need to go through the rest of the code
and update multiple conditions. Not to mention the readability
improvements...
+ /* reject conflicting "-only" options */
+ if (globals_only && with_statistics)
+ pg_fatal("options %s and %s cannot be used together",
+ "-g/--globals-only", "--statistics");
+ if (globals_only && statistics_only)
+ pg_fatal("options %s and %s cannot be used together",
+ "-g/--globals-only", "--statistics-only");
As before, I think we should integrate the new conflicting option handling
into the existing section that does this sort of thing. We should also
make sure the handling is the same. The existing code uses pg_log_error(),
pg_log_error_hint(), and exit_nicely(), while the patch uses pg_fatal().
--
nathan
Hello! Should these work? (currently these don't result in errors, but doesn't seem to be useful in practice) pg_dumpall --globals-only --no-schema pg_dumpall --globals-only --data-only Also previously the code had a check that certain flags (--statistics-only, --data-only, --no-schema) didn't dump roles and tablespaces. With the current patch, this is no longer true, and that doesn't seem to be an intended change, at least it's not explained in the commit message. The removed condition that causes this was already mentioned previously, but without explicitly stating that this results in a behavior change.
On Thu, Feb 5, 2026 at 2:33 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I'm not following your objection here. If anything, such a change would
> reduce complexity. For example, we currently use the following check in
> multiple places to decide whether to drop/drump databases:
>
> if (!globals_only && !roles_only && !tablespaces_only)
>
> If we created a derivative flag like this:
>
> shouldDumpDBs = !globals_only && !roles_only && !tablespaces_only;
>
> We could then decide whether to do database things like this:
>
> if (shouldDumpDBs)
> dumpDatabases(conn);
>
> This has the added benefit of simplifying future patches that add new -only
> options. If/when that happens, we'd just add it to the line that sets
> shouldDumpDBs, whereas today we'd need to go through the rest of the code
> and update multiple conditions. Not to mention the readability
> improvements...
I thought you meant to add a new field to DumpOptions.
I've added 3 bool variables: shouldDumpDBs, shouldDumpTablespaces,
shouldDumpRoles.
shouldDumpDBs = !globals_only && !roles_only && !tablespaces_only;
shouldDumpTablespaces = !roles_only && !no_tablespaces &&
!data_only && !schema_only && !statistics_only;
shouldDumpRoles = !tablespaces_only && !data_only && !schema_only
&& !statistics_only;
pg_dumpall --statistics
will dump global objects, data, schema, and statistics.
Which is correct, I think.
>
> + /* reject conflicting "-only" options */
> + if (globals_only && with_statistics)
> + pg_fatal("options %s and %s cannot be used together",
> + "-g/--globals-only", "--statistics");
> + if (globals_only && statistics_only)
> + pg_fatal("options %s and %s cannot be used together",
> + "-g/--globals-only", "--statistics-only");
>
> As before, I think we should integrate the new conflicting option handling
> into the existing section that does this sort of thing. We should also
> make sure the handling is the same. The existing code uses pg_log_error(),
> pg_log_error_hint(), and exit_nicely(), while the patch uses pg_fatal().
>
Adding a pg_log_error_hint would likely be helpful, since the reason
for the failure is not very intuitive,
The attached patch also addresses the points mentioned by Zsolt Parragi.
I just found out
pg_dumpall --no-data --data-only
will not immediately fail, it will fail during pg_dumpall call pg_dump.
not sure if this is ok or not.
--
jian
https://www.enterprisedb.com/
Вложения
> The attached patch also addresses the points mentioned by Zsolt Parragi.
old:
- if (!data_only && !statistics_only && !no_schema)
new:
+ shouldDumpTablespaces = !roles_only && !no_tablespaces && !data_only
&& !schema_only && !statistics_only;
+ shouldDumpRoles = !tablespaces_only && !data_only && !schema_only &&
!statistics_only;
This is still a user visible change: no_schema -> schema_only
And I don't think this change is good, roles and tablespaces are part
of the schema, without them, DDL statements later can fail.
The variables also should use under_score names, not camelCase.
And these two produce empty dumps, maybe they could result in errors instead:
pg_dumpall --globals-only --schema-only
pg_dumpall --globals-only --data-only
These produce an empty dump with a pg_dump error message, as pg_dump
blocks them, so they could use early errors:
pg_dumpall --data-only --statistics-only
pg_dumpall --schema-only --statistics-only
pg_dumpall --data-only --schema-only
I also wonder if it would be better to use a simple static array with
a helper struct to describe the blocked pairs, iterating it with a
simple for loop and generating error messages dynamically, instead of
manual copy-paste and editing. This if list is already getting quite
long, and it doesn't contain all combinations that should be blocked
yet.
E.g.
incompatible_options[] = { { &globals_only, &roles_only,
"-g/--globals-only", "-r/--roles-only" }, ... }
On Fri, Feb 6, 2026 at 4:35 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
>
> > The attached patch also addresses the points mentioned by Zsolt Parragi.
>
> old:
>
> - if (!data_only && !statistics_only && !no_schema)
>
> new:
>
> + shouldDumpTablespaces = !roles_only && !no_tablespaces && !data_only
> && !schema_only && !statistics_only;
> + shouldDumpRoles = !tablespaces_only && !data_only && !schema_only &&
> !statistics_only;
>
> This is still a user visible change: no_schema -> schema_only
>
> And I don't think this change is good, roles and tablespaces are part
> of the schema, without them, DDL statements later can fail.
>
hi.
I believe "schema" generally refers to object definitions, excluding
things like roles and tablespaces.
I tend to think that once "only" is specified, the "no" option meaning
is not applied,
thus I'm ok with
pg_dumpall --globals-only --no-schema
is equivalent to
pg_dumpall --globals-only
For all these pg_dumpall combination
--roles-only
--tablespaces-only
--statistics-only
--schema-only
--globals-only
--data-only
--statistics
The only allowed combination is --statistics --statistics-only.
since pg_dump also supports it, and these two option meanings do not contradict.
please check v4, it looks very neat, IMHO.
for example:
+ if (schema_only && (with_statistics || statistics_only))
+ {
+ pg_log_error("options %s and %s cannot be used together",
+ "-s/--schema-only",
+ statistics_only ? "--statistics-only" :
+ "--statistics");
+
src/bin/pg_dump/t/001_basic.pl tests are well aligned with pg_dumpall.c code, so
it's quite easy to review.
--
jian
https://www.enterprisedb.com/
Вложения
Hello! > I believe "schema" generally refers to object definitions, excluding > things like roles and tablespaces. Please see the attached tap test case. It works with the current master branch, and fails with the patch. Basically my expectation is that if we use dumpall --schema-only, we should be able to restore it without errors, except for one error for the already existing postgres/current user which it ignores. I also found one more issue/behavior change: --no-schema --clean now generates DROP statements (roles, tablespaces, databases), while previously it didn't.
Вложения
On Fri, Feb 6, 2026 at 5:36 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote: > > Please see the attached tap test case. It works with the current > master branch, and fails with the patch. Basically my expectation is > that if we use dumpall --schema-only, we should be able to restore it > without errors, except for one error for the already existing > postgres/current user which it ignores. > > I also found one more issue/behavior change: --no-schema --clean now > generates DROP statements (roles, tablespaces, databases), while > previously it didn't. hi. It would be better to simply reject the CONFLICT ONLY option and keep the rest of the logic same as the HEAD. That way, we avoid any surprising behavior. IMHO. Please check attached v5. -- jian https://www.enterprisedb.com/
Вложения
> It would be better to simply reject the CONFLICT ONLY option and keep the rest > of the logic same as the HEAD. I agree, that's why I showed that test case that failed. This version is definitely better, it's also more readable than the previous version with the many ifs. But it is again missing a few cases that should error out before pg_dump, as currently it fails after pg_dump errors out: --schema-only --no-schema --data-only --no-data --statistics-only --no-statistics /* Make sure the user hasn't specified a mix of globals-only options */ - if (globals_only && roles_only) + if (globals_only && + (roles_only || tablespaces_only || with_statistics || statistics_only || schema_only || data_only)) The comment above the checks also seems stale, as we have more checks with the patch
On Mon, Feb 09, 2026 at 07:09:54PM +0000, Zsolt Parragi wrote: > But it is again missing a few cases that should error out before > pg_dump, as currently it fails after pg_dump errors out: > > --schema-only --no-schema > --data-only --no-data > --statistics-only --no-statistics Is there a reason we need to duplicate these checks in pg_dumpall when they are already handled by pg_dump? -- nathan
> Is there a reason we need to duplicate these checks in pg_dumpall when they > are already handled by pg_dump? Mainly I think it would be a nicer user experience to fail early without generating additional output other than the error message (currently it writes out 26 lines before the error), but there are also two specific reasons why it would be an improvement: * "--schema-only --no-schema" is already a contradiction before pg_dumpall calls pg_dump: should it print out roles/tablespaces or not? (it doesn't) * if you specify "pg_dumpall --data-only -no-data -f dump.sql", or redirect stdout to a file, it writes out a partial dump before failing, and leaves it there. Users should check error messages and exit codes, but the file is still there and could cause accidents. 3 simple checks could prevent this.
On Tue, Feb 10, 2026 at 3:47 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote: > > > Is there a reason we need to duplicate these checks in pg_dumpall when they > > are already handled by pg_dump? > > Mainly I think it would be a nicer user experience to fail early > without generating additional output other than the error message > (currently it writes out 26 lines before the error), but there are > also two specific reasons why it would be an improvement: > > * "--schema-only --no-schema" is already a contradiction before > pg_dumpall calls pg_dump: should it print out roles/tablespaces or > not? (it doesn't) > * if you specify "pg_dumpall --data-only -no-data -f dump.sql", or > redirect stdout to a file, it writes out a partial dump before > failing, and leaves it there. Users should check error messages and > exit codes, but the file is still there and could cause accidents. 3 > simple checks could prevent this. OK. The attached v6 added these 3 "--only" and "--no" checks, along with related tests. -- jian https://www.enterprisedb.com/
Вложения
Shouldn't these also use pg_log_error + pg_log_error_hint + exit_nicely? And there's a commit message typo: "there 3 combination should fail immediately" there -> these Otherwise it looks good to me.
On Wed, Feb 25, 2026 at 4:09 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote: > > Shouldn't these also use pg_log_error + pg_log_error_hint + exit_nicely? > Sure. > Otherwise it looks good to me. While rebasing, I found that I missed the combination: --statistics and --no-statistics. Since pg_dump will error out on this combination, pg_dumpall should too. -- jian https://www.enterprisedb.com/
Вложения
New version looks good!
On Mon, Mar 02, 2026 at 10:23:17PM +0000, Zsolt Parragi wrote: > New version looks good! I'm not thrilled about the long list of checks. What if we added a function that could check an arbitrary number of mutually exclusive options, a bit like the attached? -- nathan
Вложения
On Tue, Mar 3, 2026 at 6:41 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > I'm not thrilled about the long list of checks. What if we added a > function that could check an arbitrary number of mutually exclusive > options, a bit like the attached? > hi. This is neater than my version, and overall it looks good to me. Once this is committed, we can use CheckMutuallyExclusiveOpts in both pg_dump.c and pg_restore.c. -- jian https://www.enterprisedb.com/
> On Mar 3, 2026, at 06:41, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Mon, Mar 02, 2026 at 10:23:17PM +0000, Zsolt Parragi wrote:
>> New version looks good!
>
> I'm not thrilled about the long list of checks. What if we added a
> function that could check an arbitrary number of mutually exclusive
> options, a bit like the attached?
>
> --
> nathan
> <v8-0001-pg_dumpall-error-out-conflict-options.patch>
Yeah, the new helper function makes the code much cleaner, good job.
A few comments on v8:
1 - dumputils.c
```
+void
+CheckMutuallyExclusiveOpts(int n,...)
+{
+ char *first = NULL;
+ va_list args;
+
+ va_start(args, n);
+ for (int i = 0; i < n; i += 2)
```
I think we can Assert(n % 2 == 0).
If a future code author mistakenly forgets an option name, the compiler won’t detect that, and runtime will only show
“null”for the option name.
I tried to delete the last option name from the first CheckMutuallyExclusiveOpts call, then I got:
```
% pg_dumpall -a --no-data
pg_dumpall: error: options -a/--data-only and (null) cannot be used together
```
2 - dumputils.c
```
+ pg_fatal("options %s and %s cannot be used together",
+ first, opt);
```
The current code also shows a hint upon the error, do we want to retain that?
```
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
```
3 - 001_basic.pl
```
+ 'pg_dumpall: error: options /-s\/--schema-only and --statistics-only cannot be used together'
```
“/“ before “-s” seems not needed.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
This is definitely better. I suggested an array based approach back in
my first email to the thread, and that didn't get any positive
feedback, so I thought people prefer the simple ifs.
- pg_log_error_hint("Try \"%s --help\" for more information.", progname);
This error hint comment is now missing, was that intentional?
+ CheckMutuallyExclusiveOpts(12, globals_only, "-g/--globals-only",
+ roles_only, "-r/--roles-only",
+ tablespaces_only, "-t/--tablespaces-only",
+ data_only, "-a/--data-only",
+ schema_only, "-s/--schema-only",
+ statistics_only, "--statistics-only");
+ CheckMutuallyExclusiveOpts(12,
+ globals_only, "-g/--globals-only",
+ roles_only, "-r/--roles-only",
+ tablespaces_only, "-t/--tablespaces-only",
+ schema_only, "-s/--schema-only",
+ data_only, "-a/--data-only",
+ with_statistics, "--statistics");
This was confusing it at first. data_only schema_only are present on
both but in different order, the only real difference is
with_statistics/statistics_only. I would fix the order and add a
comment that only the last item differs.
On Tue, Mar 03, 2026 at 10:46:51AM +0800, Chao Li wrote:
> ```
> + pg_fatal("options %s and %s cannot be used together",
> + first, opt);
> ```
>
> The current code also shows a hint upon the error, do we want to retain that?
IMHO the hint is kind of silly because --help doesn't actually provide any
useful information about which options conflict with each other. Not to
mention that pg_dump doesn't provide a hint for conflicting options.
--
nathan
Here's what I have staged for commit. A few notes: * I moved the new function to option_utils.c and added a macro that takes care of figuring out the number of arguments. * I removed the derivative flags. I know I lobbied for those, but they seemed out of place for this patch. Maybe we can add them separately. * I limited the number of new tests to one per call to check_mut_excl_opts(). I didn't see much benefit from testing everything exhaustively. -- nathan
Вложения
Committed. Jian He, would you mind putting together similar patches for pg_dump and pg_restore? -- nathan