Обсуждение: 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/