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.
