Обсуждение: 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
isreporting 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
patchesfor 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, 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
isreporting 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
patchesfor 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

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