| From: | Mahendra Singh Thalor <mahi6run(at)gmail(dot)com> |
|---|---|
| To: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andrew Dunstan <andrew(at)dunslane(dot)net>, nathan(at)postgresql(dot)org |
| Subject: | Re: Restore check_mut_excl_opts, usage in pg_restore and pg_dumpall |
| Date: | 2026-07-20 10:12:41 |
| Message-ID: | CAKYtNArryDL=KZ=FCcdsk3GsS4JaoUjgSG209qm+1hWMke+21Q@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Thanks Fujii Masao for the review and feedback.
On Fri, 17 Jul 2026 at 16:43, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> On Tue, Jul 14, 2026 at 5:31 PM Mahendra Singh Thalor
> <mahi6run(at)gmail(dot)com> 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
| Attachment | Content-Type | Size |
|---|---|---|
| v02-0001-pg_restore-Use-check_mut_excl_opts-for-mutually-excl.patch | text/x-patch | 5.3 KB |
| v02-0002-pg_dumpall-Use-check_mut_excl_opts-for-mutually-excl.patch | text/x-patch | 8.1 KB |
| v02-0003-pg_dump-Fix-argument-type-passed-to-check_mut_excl_opts.patch | text/x-patch | 1.3 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Amit Kapila | 2026-07-20 10:14:34 | Re: Proposal: Conflict log history table for Logical Replication |
| Previous Message | Rafia Sabih | 2026-07-20 09:16:32 | Re: postgres_fdw: Emit message when batch_size is reduced |