public inbox for [email protected]
help / color / mirror / Atom feedFrom: Mahendra Singh Thalor <[email protected]>
To: Andrew Dunstan <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error
Date: Sun, 15 Mar 2026 22:32:11 +0530
Message-ID: <CAKYtNArQ-c8xfBaWOKBB6TPVM5Fd_d0v2UEpxk9f5BuKyrQYxQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAKYtNApkh=Vy2DpNRCnEJmPpxNuksbAh_QBav=2fLmVjBhGwFw@mail.gmail.com>
<CAKYtNApxyeeQbjxjFCgHwuv0T2TJvFaU6QSQaMU_yr1nP4Y8Fg@mail.gmail.com>
<[email protected]>
On Sun, 15 Mar 2026 at 22:01, Andrew Dunstan <[email protected]> wrote:
>
>
> On 2026-03-15 Su 12:18 AM, Mahendra Singh Thalor wrote:
>
> On Sun, 13 Apr 2025 at 18:32, Mahendra Singh Thalor <[email protected]> wrote:
>
> Hi,
> With "pg_restore --format=", we are not giving any error because in code, we are checking length of arg but pg_dump is reporting an error for the same option.
>
> For the consistency purpose, pg_dump and pg_restore both should report an error for the test case below.
>
> Ex: (output after this patch)but before this patch, below command is passing.
> /pg_restore x1 -d postgres -j 10 -C --verbose --format=
> pg_restore: error: unrecognized archive format ""; please specify "c", "d", or "t"
>
> Here, I am attaching a patch which is fixing the same. I added 2 TAP tests also for invalid options.
>
> Note: We have 2 more options in pg_restore code which validate the option if arg has non zero length. I will prepare patches for both(--host and --port). We need to add some validate function also for both these options.
>
> --
> Thanks and Regards
> Mahendra Singh Thalor
> EnterpriseDB: http://www.enterprisedb.com
>
> Hi all,
> Here I am attaching a re-based patch.
>
> I think we should sync behaviour with pg_dump and pg_restore. Please
> review this patch and let me know feedback.
>
>
>
> Let's try to deal with all these in one hit, instead if piecemeal.
>
>
> cheers
>
>
> andrew
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
Thanks Srinath and Andrew for the review and feedback.
Here, I am attaching an updated patch for the review. I removed the
length check for host, port and format in pg_restore as we don't have
check in pg_dump also. I think we don't need any test cases for host
and port.
If we want to backpatch, then I can make patches for back branches but
as of now, I am uploading a patch for master only.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
Attachments:
[text/x-patch] v03-pg_restore-format-host-port-options-should-validate-all-values.patch (3.0K, 2-v03-pg_restore-format-host-port-options-should-validate-all-values.patch)
download | inline diff:
From ce5e45b744556b77028f9dfc398b330e92f0f4ae Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <[email protected]>
Date: Sun, 15 Mar 2026 22:21:57 +0530
Subject: [PATCH] pg_restore -F/--format, -h/--host, -p/--port options should
validate values in all cases
With "pg_restore --format=", we are not giving any error because in code, we are checking
length of arg but pg_dump is troughing an error for the same option.
Ex: before this patch, below command was passing.
/pg_restore x1 -d postgres -j 10 -C --verbose --format=
after patch:
/pg_restore x1 -d postgres -j 10 -C --verbose --format=
pg_restore: error: unrecognized archive format ""; please specify "c", "d", or "t"
Note: pg_dump and pg_dumpall are already reporting an error in above case.
same fix is needed for -h/--host, -p/--port options also.
---
src/bin/pg_dump/pg_restore.c | 9 +++------
src/bin/pg_dump/t/001_basic.pl | 10 ++++++++++
2 files changed, 13 insertions(+), 6 deletions(-)
mode change 100644 => 100755 src/bin/pg_dump/t/001_basic.pl
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 9b4b151b318..fa5e1355e71 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -225,16 +225,14 @@ main(int argc, char **argv)
opts->filename = pg_strdup(optarg);
break;
case 'F':
- if (strlen(optarg) != 0)
- opts->formatName = pg_strdup(optarg);
+ opts->formatName = pg_strdup(optarg);
break;
case 'g':
/* restore only global sql commands. */
globals_only = true;
break;
case 'h':
- if (strlen(optarg) != 0)
- opts->cparams.pghost = pg_strdup(optarg);
+ opts->cparams.pghost = pg_strdup(optarg);
break;
case 'j': /* number of restore jobs */
if (!option_parse_int(optarg, "-j/--jobs", 1,
@@ -264,8 +262,7 @@ main(int argc, char **argv)
break;
case 'p':
- if (strlen(optarg) != 0)
- opts->cparams.pgport = pg_strdup(optarg);
+ opts->cparams.pgport = pg_strdup(optarg);
break;
case 'R':
/* no-op, still accepted for backwards compatibility */
diff --git a/src/bin/pg_dump/t/001_basic.pl b/src/bin/pg_dump/t/001_basic.pl
old mode 100644
new mode 100755
index 2f5eb48e7b8..3914fb158c2
--- a/src/bin/pg_dump/t/001_basic.pl
+++ b/src/bin/pg_dump/t/001_basic.pl
@@ -201,6 +201,16 @@ command_fails_like(
qr/\Qpg_restore: error: unrecognized archive format "garbage";\E/,
'pg_dump: unrecognized archive format');
+command_fails_like(
+ [ 'pg_restore', '-f -', '--format='],
+ qr/\Qpg_restore: error: unrecognized archive format "";\E/,
+ 'pg_restore: unrecognized archive format empty string');
+
+command_fails_like(
+ [ 'pg_restore', '-f -', '-F', 'p' ],
+ qr/\Qpg_restore: error: archive format "p" is not supported; please use psql\E/,
+ 'pg_restore: unrecognized archive format p|plain');
+
command_fails_like(
[ 'pg_dump', '--on-conflict-do-nothing' ],
qr/pg_dump: error: option --on-conflict-do-nothing requires option --inserts, --rows-per-insert, or --column-inserts/,
--
2.52.0
view thread (9+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error
In-Reply-To: <CAKYtNArQ-c8xfBaWOKBB6TPVM5Fd_d0v2UEpxk9f5BuKyrQYxQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox