Re: Restore check_mut_excl_opts, usage in pg_restore and pg_dumpall
Re: Restore check_mut_excl_opts, usage in pg_restore and pg_dumpall
От:
Fujii Masao <masao.fujii@gmail.com>
Дата:
On Mon, Jul 20, 2026 at 7:12 PM Mahendra Singh Thalor
wrote:
> Here, I am attaching 3 patches for the review.
Thanks for updating the patches!
+ check_mut_excl_opts(opts->cparams.dbname, "-d/--dbname",
+ opts->filename, "-f/--file");
...
+ /* --dbname and --restrict-key are incompatible */
+ check_mut_excl_opts(opts->cparams.dbname, "-d/--dbname",
+ opts->restrict_key, "--restrict-key");
opts->cparams.dbname, opts->filename, and opts->restrict_key are pointers.
Passing them to check_mut_excl_opts() and reading them back as int is
undefined behavior. Shouldn't these calls instead be written as:
check_mut_excl_opts(opts->cparams.dbname != NULL, "-d/--dbname",
opts->filename != NULL, "-f/--file");
check_mut_excl_opts(opts->cparams.dbname != NULL, "-d/--dbname",
opts->restrict_key != NULL, "--restrict-key");
I'm wondering about backpatching. Patch 0003 fixes undefined behavior
introduced by check_mut_excl_opts(), so it seems appropriate to
backpatch it to v19. Patches 0001 and 0002 are mostly refactoring,
but they restore the v19-era cleanup that was lost when the pg_dumpall
non-text-format support was reverted, so backpatching those to v19 also
seems OK to me. Thoughts?
If we commit these, I think it would make sense to merge them into
a single commit.
Regards,
--
Fujii Masao
Re: Restore check_mut_excl_opts, usage in pg_restore and pg_dumpall
От:
Fujii Masao <masao.fujii@gmail.com>
Дата:
On Tue, Jul 14, 2026 at 5:31 PM Mahendra Singh Thalor
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?
> 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",
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.
Regards,
--
Fujii Masao
Restore check_mut_excl_opts, usage in pg_restore and pg_dumpall
От:
Mahendra Singh Thalor <mahi6run@gmail.com>
Дата:
Hi,
Commit 7ca548f23a6 reverted pg_dumpall's non-text output format support
per Noah Misch's post-commit review. That revert was scoped carefully to
remove only the feature itself, but its commit message notes one piece of
unavoidable collateral damage:
Because the feature restructured pg_dumpall.c and pg_restore.c (pg_restore's
main() was split into restore_one_database() plus a dispatcher) and
interleaved its option checks with the conflicting-option refactor in the
same regions, the cosmetic check_mut_excl_opts() reflow of those two files'
option blocks is inseparable from the feature and comes out with it; the
behavior is unchanged.
In other words, pg_restore.c and pg_dumpall.c both went back to their old,
ad hoc pairwise "if (a && b) pg_fatal(...)" style option-conflict checks,
even though converting them to use the shared check_mut_excl_opts() helper
(added by b2898baaf7e, and already used by pg_dump.c) is a purely mechanical,
behavior-preserving cleanup with no dependency on the reverted feature's
design.
Attached are two small, independent patches that redo that conversion on
top of current master:
0001 - pg_restore.c: converts its *-only / --no-* / --statistics /
--clean / --single-transaction conflict checks to
check_mut_excl_opts().
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.
Both patches preserve the exact pre-existing error-message wording and
option-reporting order for every combination that already had test
coverage; I verified this with the full src/bin/pg_dump TAP suite
(make check), which passes cleanly with both patches applied.
They're independent of each other and can be applied/reviewed in either
order or separately.
I think we can commit those into v19 and master. Please review and let
me know feedback.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
Re: Restore check_mut_excl_opts, usage in pg_restore and pg_dumpall
От:
Mahendra Singh Thalor <mahi6run@gmail.com>
Дата:
Thanks Fujii Masao for the review and feedback. On Fri, 17 Jul 2026 at 16:43, Fujii Masao wrote: > > On Tue, Jul 14, 2026 at 5:31 PM Mahendra Singh Thalor > 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
