public inbox for [email protected]
help / color / mirror / Atom feedFrom: Mahendra Singh Thalor <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: 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: Tue, 17 Mar 2026 14:51:56 +0530
Message-ID: <CAKYtNArN7Tyn7n0wp8BxcpkfBn3gi01QZJS55sbj=qmdyoNO4A@mail.gmail.com> (raw)
In-Reply-To: <abhbSpewJZz0H999@nathan>
References: <CAKYtNApkh=Vy2DpNRCnEJmPpxNuksbAh_QBav=2fLmVjBhGwFw@mail.gmail.com>
<CAKYtNApxyeeQbjxjFCgHwuv0T2TJvFaU6QSQaMU_yr1nP4Y8Fg@mail.gmail.com>
<[email protected]>
<CAKYtNArQ-c8xfBaWOKBB6TPVM5Fd_d0v2UEpxk9f5BuKyrQYxQ@mail.gmail.com>
<abhbSpewJZz0H999@nathan>
Thanks Nathan for the review and feedback.
On Tue, 17 Mar 2026 at 01:04, Nathan Bossart <[email protected]>
wrote:
>
> On Sun, Mar 15, 2026 at 10:32:11PM +0530, Mahendra Singh Thalor wrote:
> > 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.
>
> Looks generally reasonable to me.
>
> > I think we don't need any test cases for host and port.
>
> Why not?
I did some more testing and found that if there is an empty string with
--port/--host, then we don't report any error because we don't validate
empty strings. This looks weird to me but this is happening with all tools
so I was not adding any test case for --host and --port.
Please see the test cases.
*test case1: ./psql -d postgres --port*
./psql: option '--port' requires an argument
psql: hint: Try "psql --help" for more information.
*test case 2: ./psql -d postgres --port=*
psql (19devel)
Type "help" for help.
postgres=# \q
*test case 3: ./psql -d postgres --port= 9*
psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed:
FATAL: role "9" does not exist
In case 2 and 3, empty string is considered as valid value for --port and
the same is happening with --host and other options also. I think we should
add checks for all the tools to validate empty strings (tools should report
errors as value is required with these arguments.)
Please add your opinion.
>
> > 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.
>
> -1 for back-patching. --format seems to have been broken since pg_restore
> was first committed in 2000 (commit 500b62b057), so I don't sense any
> urgency here. Not to mention that someone might be relying on the current
> behavior.
Okay. I agree with you.
> > +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');
>
> How does this test relate to this change?
>
> --
> nathan
Fixed. I removed this test case from the current patch.
Here, I am attaching an updated patch.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
Attachments:
[text/x-patch] v04-pg_restore-format-host-port-options-should-validate-all-values.patch (2.8K, 3-v04-pg_restore-format-host-port-options-should-validate-all-values.patch)
download | inline diff:
From e88baae1e8831beda36cb0e6f561816e15658862 Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <[email protected]>
Date: Tue, 17 Mar 2026 14:35:15 +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 | 5 +++++
2 files changed, 8 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 91efe305650..f016b336308 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 86eec1b0064..fb1ade48f1d
--- a/src/bin/pg_dump/t/001_basic.pl
+++ b/src/bin/pg_dump/t/001_basic.pl
@@ -206,6 +206,11 @@ 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_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], [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: <CAKYtNArN7Tyn7n0wp8BxcpkfBn3gi01QZJS55sbj=qmdyoNO4A@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