public inbox for [email protected]
help / color / mirror / Atom feedpg_restore --format= option(without any value) should report an error as pg_dump is reporting an error
9+ messages / 3 participants
[nested] [flat]
* pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error
@ 2025-04-13 13:02 Mahendra Singh Thalor <[email protected]>
2026-03-15 04:18 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Mahendra Singh Thalor @ 2025-04-13 13:02 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
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
Attachments:
[application/octet-stream] v01-pg_restore-format-option-should-validate-all-values.patch (2.2K, 3-v01-pg_restore-format-option-should-validate-all-values.patch)
download | inline diff:
From b553405e50b25ffcf9d408e21d1f926c6452748d Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <[email protected]>
Date: Sun, 13 Apr 2025 19:17:57 +0530
Subject: [PATCH] pg_restore --format option should validate all values
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: (after this patch)-- 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"
---
src/bin/pg_dump/pg_restore.c | 3 +--
src/bin/pg_dump/t/001_basic.pl | 10 ++++++++++
2 files changed, 11 insertions(+), 2 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 ff4bb320fc9..c86d593df92 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -217,8 +217,7 @@ 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.dat file from directory */
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 84ca25e17d6..67b97ec5b92
--- 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_dump: 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_dump: 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.39.3
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error
2025-04-13 13:02 pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
@ 2026-03-15 04:18 ` Mahendra Singh Thalor <[email protected]>
2026-03-15 13:44 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Srinath Reddy Sadipiralla <[email protected]>
2026-03-15 17:02 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
0 siblings, 2 replies; 9+ messages in thread
From: Mahendra Singh Thalor @ 2026-03-15 04:18 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
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.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
Attachments:
[text/x-patch] v02-pg_restore-format-option-should-validate-all-values.patch (2.3K, 2-v02-pg_restore-format-option-should-validate-all-values.patch)
download | inline diff:
From 8fa9f056d567ce350e88503c872e0267592100a0 Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <[email protected]>
Date: Sun, 15 Mar 2026 09:42:52 +0530
Subject: [PATCH] pg_restore --format option should validate all values
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 is already reporting an error in above case.
---
src/bin/pg_dump/pg_restore.c | 3 +--
src/bin/pg_dump/t/001_basic.pl | 10 ++++++++++
2 files changed, 11 insertions(+), 2 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 fb44c0cfdfe..d755d3c72dc 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -225,8 +225,7 @@ 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. */
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..bf661910c66
--- 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_dump: 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_dump: 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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error
2025-04-13 13:02 pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 04:18 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
@ 2026-03-15 13:44 ` Srinath Reddy Sadipiralla <[email protected]>
1 sibling, 0 replies; 9+ messages in thread
From: Srinath Reddy Sadipiralla @ 2026-03-15 13:44 UTC (permalink / raw)
To: Mahendra Singh Thalor <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Sun, Mar 15, 2026 at 9:48 AM Mahendra Singh Thalor <[email protected]>
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.
>
+1 , patch LGTM, i think this also needs backpatching,
but i think in the TAP test, change the test_name from pg_dump to
pg_restore.
diff --git a/src/bin/pg_dump/t/001_basic.pl b/src/bin/pg_dump/t/001_basic.pl
index bf661910c66..3914fb158c2 100755
--- a/src/bin/pg_dump/t/001_basic.pl
+++ b/src/bin/pg_dump/t/001_basic.pl
@@ -204,12 +204,12 @@ command_fails_like(
command_fails_like(
[ 'pg_restore', '-f -', '--format='],
qr/\Qpg_restore: error: unrecognized archive format "";\E/,
- 'pg_dump: unrecognized archive format empty string');
+ '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_dump: unrecognized archive format p|plain');
+ 'pg_restore: unrecognized archive format p|plain');
command_fails_like(
[ 'pg_dump', '--on-conflict-do-nothing' ],
--
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error
2025-04-13 13:02 pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 04:18 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
@ 2026-03-15 17:02 ` Mahendra Singh Thalor <[email protected]>
2026-03-16 19:34 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Nathan Bossart <[email protected]>
1 sibling, 1 reply; 9+ messages in thread
From: Mahendra Singh Thalor @ 2026-03-15 17:02 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; +Cc: PostgreSQL Hackers <[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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error
2025-04-13 13:02 pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 04:18 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 17:02 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
@ 2026-03-16 19:34 ` Nathan Bossart <[email protected]>
2026-03-17 09:21 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Nathan Bossart @ 2026-03-16 19:34 UTC (permalink / raw)
To: Mahendra Singh Thalor <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; PostgreSQL Hackers <[email protected]>
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?
> 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.
> +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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error
2025-04-13 13:02 pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 04:18 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 17:02 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-16 19:34 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Nathan Bossart <[email protected]>
@ 2026-03-17 09:21 ` Mahendra Singh Thalor <[email protected]>
2026-03-17 16:45 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Nathan Bossart <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Mahendra Singh Thalor @ 2026-03-17 09:21 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; PostgreSQL Hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error
2025-04-13 13:02 pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 04:18 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 17:02 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-16 19:34 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Nathan Bossart <[email protected]>
2026-03-17 09:21 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
@ 2026-03-17 16:45 ` Nathan Bossart <[email protected]>
2026-03-18 19:24 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Nathan Bossart <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Nathan Bossart @ 2026-03-17 16:45 UTC (permalink / raw)
To: Mahendra Singh Thalor <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Mar 17, 2026 at 02:51:56PM +0530, Mahendra Singh Thalor wrote:
> 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.
Oh, weird.
Unless new feedback materializes, I'll proceed with committing your patch
within the next day or so.
--
nathan
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error
2025-04-13 13:02 pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 04:18 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 17:02 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-16 19:34 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Nathan Bossart <[email protected]>
2026-03-17 09:21 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-17 16:45 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Nathan Bossart <[email protected]>
@ 2026-03-18 19:24 ` Nathan Bossart <[email protected]>
2026-03-19 20:01 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Nathan Bossart @ 2026-03-18 19:24 UTC (permalink / raw)
To: Mahendra Singh Thalor <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; PostgreSQL Hackers <[email protected]>
Committed.
--
nathan
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error
2025-04-13 13:02 pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 04:18 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 17:02 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-16 19:34 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Nathan Bossart <[email protected]>
2026-03-17 09:21 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-17 16:45 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Nathan Bossart <[email protected]>
2026-03-18 19:24 ` Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Nathan Bossart <[email protected]>
@ 2026-03-19 20:01 ` Mahendra Singh Thalor <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Mahendra Singh Thalor @ 2026-03-19 20:01 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; PostgreSQL Hackers <[email protected]>
Thanks Nathan for committing this.
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
On Thu, 19 Mar 2026 at 12:54 AM, Nathan Bossart <[email protected]>
wrote:
> Committed.
>
> --
> nathan
>
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2026-03-19 20:01 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-04-13 13:02 pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error Mahendra Singh Thalor <[email protected]>
2026-03-15 04:18 ` Mahendra Singh Thalor <[email protected]>
2026-03-15 13:44 ` Srinath Reddy Sadipiralla <[email protected]>
2026-03-15 17:02 ` Mahendra Singh Thalor <[email protected]>
2026-03-16 19:34 ` Nathan Bossart <[email protected]>
2026-03-17 09:21 ` Mahendra Singh Thalor <[email protected]>
2026-03-17 16:45 ` Nathan Bossart <[email protected]>
2026-03-18 19:24 ` Nathan Bossart <[email protected]>
2026-03-19 20:01 ` Mahendra Singh Thalor <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox