Subject: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error

From: Mahendra Singh Thalor <[email-masked]>
Date: Sun, 13 Apr 2025 18:32:01 +0530

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

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Sun, 15 Mar 2026 09:48:06 +0530

On Sun, 13 Apr 2025 at 18:32, Mahendra Singh Thalor <[email-masked]> 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: 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

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
From 8fa9f056d567ce350e88503c872e0267592100a0 Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <[email-masked]>
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

---

From: Srinath Reddy Sadipiralla <[email-masked]>
Date: Sun, 15 Mar 2026 19:14:57 +0530

On Sun, Mar 15, 2026 at 9:48 AM Mahendra Singh Thalor <[email-masked]>
wrote:

> > 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.
> >
> > 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

+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(

-- 
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Sun, 15 Mar 2026 22:32:11 +0530

On Sun, 15 Mar 2026 at 22:01, Andrew Dunstan <[email-masked]> wrote:
>
>
> On 2026-03-15 Su 12:18 AM, Mahendra Singh Thalor wrote:
>
> 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
From ce5e45b744556b77028f9dfc398b330e92f0f4ae Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <[email-masked]>
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

after patch:

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(

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 16 Mar 2026 14:34:34 -0500

On Sun, Mar 15, 2026 at 10:32:11PM +0530, Mahendra Singh Thalor wrote:
> 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?

> 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.

> +	'pg_restore: unrecognized archive format p|plain');

How does this test relate to this change?

-- 
nathan

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Tue, 17 Mar 2026 14:51:56 +0530

Thanks Nathan for the review and feedback.

> > I think we don't need any test cases for host and port.

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.

> urgency here.  Not to mention that someone might be relying on the current

Okay. I agree with you.

> How does this test relate to this change?

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
From e88baae1e8831beda36cb0e6f561816e15658862 Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <[email-masked]>
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

after patch:

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

index 91efe305650..f016b336308 100644
 
 			case 'p':
index 86eec1b0064..fb1ade48f1d
@@ -206,6 +206,11 @@ command_fails_like(
 
+command_fails_like(

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 17 Mar 2026 11:45:41 -0500

> 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

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 18 Mar 2026 14:24:42 -0500

Committed.

-- 
nathan

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Fri, 20 Mar 2026 01:31:08 +0530

Thanks Nathan for committing this.

Thanks and Regards
Mahendra Singh Thalor