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

On Sun, 13 Apr 2025 at 18:32, Mahendra Singh Thalor <mahi6run@gmail.com> 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

On Sun, Mar 15, 2026 at 9:48 AM Mahendra Singh Thalor <mahi6run@gmail.com>
wrote:

On Sun, 13 Apr 2025 at 18:32, Mahendra Singh Thalor <mahi6run@gmail.com>
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/

On 2026-03-15 Su 12:18 AM, Mahendra Singh Thalor wrote:

On Sun, 13 Apr 2025 at 18:32, Mahendra Singh Thalor<mahi6run@gmail.com> 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

On Sun, 15 Mar 2026 at 22:01, Andrew Dunstan <andrew@dunslane.net> wrote:

On 2026-03-15 Su 12:18 AM, Mahendra Singh Thalor wrote:

On Sun, 13 Apr 2025 at 18:32, Mahendra Singh Thalor <mahi6run@gmail.com> 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

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

Thanks Nathan for the review and feedback.

On Tue, 17 Mar 2026 at 01:04, Nathan Bossart <nathandbossart@gmail.com>
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

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

Committed.

--
nathan

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 <nathandbossart@gmail.com>
wrote:

On Fri, 20 Mar 2026 at 01:31, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:

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 <nathandbossart@gmail.com> wrote:

Committed.

--
nathan

Hi Nathan,

This fix was committed as ec80215c033, but it got reverted along with
7ca548f23a6 (the revert of pg_dumpall's non-text output format support) as
collateral damage: the option-parsing block it touched had been restructured
by that feature, so the revert took this change down with it even though it
has nothing to do with non-text formats.

Re-attaching the same fix, unchanged, rebased on the current master.
We can back-port till v19.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

On 14 Jul 2026, at 14:19, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:

Re-attaching the same fix, unchanged, rebased on the current master.
We can back-port till v19.

I agree that this change got accidentally reverted and should be re-applied,
but at this point in the cycle I think it's too late to target 19 and will
apply to master only unless there are objections to this plan.

--
Daniel Gustafsson