MIME-Version: 1.0
References: 
 <CAKYtNApkh=Vy2DpNRCnEJmPpxNuksbAh_QBav=2fLmVjBhGwFw@mail.gmail.com>
 <CAKYtNApxyeeQbjxjFCgHwuv0T2TJvFaU6QSQaMU_yr1nP4Y8Fg@mail.gmail.com>
 <1d66a1e5-ce44-448b-8407-31d5ed9bc10d@dunslane.net>
 <CAKYtNArQ-c8xfBaWOKBB6TPVM5Fd_d0v2UEpxk9f5BuKyrQYxQ@mail.gmail.com>
 <abhbSpewJZz0H999@nathan>
In-Reply-To: <abhbSpewJZz0H999@nathan>
From: Mahendra Singh Thalor <mahi6run@gmail.com>
Date: Tue, 17 Mar 2026 14:51:56 +0530
Message-ID: 
 <CAKYtNArN7Tyn7n0wp8BxcpkfBn3gi01QZJS55sbj=qmdyoNO4A@mail.gmail.com>
Subject: Re: pg_restore --format= option(without any value) should report an
 error as pg_dump is reporting an error
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Andrew Dunstan <andrew@dunslane.net>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="00000000000092d8b0064d34de3f"
Archived-At: 
 <https://www.postgresql.org/message-id/CAKYtNArN7Tyn7n0wp8BxcpkfBn3gi01QZJS55sbj%3DqmdyoNO4A%40mail.gmail.com>
Precedence: bulk

--00000000000092d8b0064d34de3f
Content-Type: multipart/alternative; boundary="00000000000092d8ae064d34de3d"

--00000000000092d8ae064d34de3d
Content-Type: text/plain; charset="UTF-8"

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

--00000000000092d8ae064d34de3d
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Thanks Nathan for the review and feedback.<br><br>On Tue, =
17 Mar 2026 at 01:04, Nathan Bossart &lt;<a href=3D"mailto:nathandbossart@g=
mail.com">nathandbossart@gmail.com</a>&gt; wrote:<br>&gt;<br>&gt; On Sun, M=
ar 15, 2026 at 10:32:11PM +0530, Mahendra Singh Thalor wrote:<br>&gt; &gt; =
Here, I am attaching an updated patch for the review. I removed the<br>&gt;=
 &gt; length check for host, port and format in pg_restore as we don&#39;t =
have<br>&gt; &gt; check =C2=A0in pg_dump also.<br>&gt;<br>&gt; Looks genera=
lly reasonable to me.<br>&gt;<br>&gt; &gt; I think we don&#39;t need any te=
st cases for host and port.<br>&gt;<br>&gt; Why not?<br><br>I did some more=
 testing and found that if there is an empty string with --port/--host, the=
n we don&#39;t report any error because we don&#39;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.<br><br>Please see the test case=
s.<br><b>test case1: ./psql -d postgres --port</b><br>./psql: option &#39;-=
-port&#39; requires an argument<br><div>psql: hint: Try &quot;psql --help&q=
uot; for more information.</div><div><br></div><b>test case 2:=C2=A0 ./psql=
 -d postgres --port=3D</b><br><div>psql (19devel)<br>Type &quot;help&quot; =
for help.<br>postgres=3D# \q<br><br><b>test case 3:=C2=A0 ./psql -d postgre=
s --port=3D 9</b><br>psql: error: connection to server on socket &quot;/tmp=
/.s.PGSQL.5432&quot; failed: FATAL: =C2=A0role &quot;9&quot; does not exist=
</div><div><br></div><div>In case 2 and 3, empty string is considered as va=
lid value for --port and the same is happening=C2=A0with --host and other o=
ptions also. I think we should add checks for all the tools to validate emp=
ty strings (tools should report errors as value is required with these argu=
ments.)</div><div><br></div><div>Please add your opinion.</div><div><br></d=
iv><div>&gt;<br>&gt; &gt; If we want to backpatch, then I can make patches =
for back branches but<br>&gt; &gt; as of now, I am uploading a patch for ma=
ster only.<br>&gt;<br>&gt; -1 for back-patching. =C2=A0--format seems to ha=
ve been broken since pg_restore<br>&gt; was first committed in 2000 (commit=
 500b62b057), so I don&#39;t sense any<br>&gt; urgency here.=C2=A0 Not to m=
ention that someone might be relying on the current<br>&gt; behavior.<br><b=
r>Okay. I agree with you.<br><br>&gt; &gt; +command_fails_like(<br>&gt; &gt=
; + =C2=A0 =C2=A0 [ &#39;pg_restore&#39;, &#39;-f -&#39;, &#39;-F&#39;, =
9;p&#39; ],<br>&gt; &gt; + =C2=A0 =C2=A0 qr/\Qpg_restore: error: archive fo=
rmat &quot;p&quot; is not supported; please use psql\E/,<br>&gt; &gt; + =C2=
=A0 =C2=A0 &#39;pg_restore: unrecognized archive format p|plain&#39;);<br>&=
gt;<br>&gt; How does this test relate to this change?<br>&gt;<br>&gt; --<br=
>&gt; nathan<br><br>Fixed. I removed this test case from the current patch.=
<br><br></div><div>Here, I am attaching an updated patch.</div><div><br></d=
iv><div>-- <br>Thanks and Regards<br>Mahendra Singh Thalor<br>EnterpriseDB:=
 <a href=3D"http://www.enterprisedb.com">http://www.enterprisedb.com</a></d=
iv></div>

--00000000000092d8ae064d34de3d--

--00000000000092d8b0064d34de3f
Content-Type: text/x-patch; charset="US-ASCII";
	name="v04-pg_restore-format-host-port-options-should-validate-all-values.patch"
Content-Disposition: attachment;
	filename="v04-pg_restore-format-host-port-options-should-validate-all-values.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_mmueiuey0>
X-Attachment-Id: f_mmueiuey0

RnJvbSBlODhiYWFlMWU4ODMxYmVkYTM2Y2IwZTZmNTYxODE2ZTE1NjU4ODYyIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBNYWhlbmRyYSBTaW5naCBUaGFsb3IgPG1haGk2cnVuQGdtYWls
LmNvbT4KRGF0ZTogVHVlLCAxNyBNYXIgMjAyNiAxNDozNToxNSArMDUzMApTdWJqZWN0OiBbUEFU
Q0hdIHBnX3Jlc3RvcmUgLUYvLS1mb3JtYXQsIC1oLy0taG9zdCwgLXAvLS1wb3J0ICBvcHRpb25z
IHNob3VsZCAKIHZhbGlkYXRlIHZhbHVlcyBpbiBhbGwgY2FzZXMKCldpdGggInBnX3Jlc3RvcmUg
LS1mb3JtYXQ9Iiwgd2UgYXJlIG5vdCBnaXZpbmcgYW55IGVycm9yIGJlY2F1c2UgaW4gY29kZSwg
d2UgYXJlIGNoZWNraW5nCmxlbmd0aCBvZiBhcmcgYnV0IHBnX2R1bXAgaXMgdHJvdWdoaW5nIGFu
IGVycm9yIGZvciB0aGUgc2FtZSBvcHRpb24uCgpFeDogYmVmb3JlIHRoaXMgcGF0Y2gsIGJlbG93
IGNvbW1hbmQgd2FzIHBhc3NpbmcuCi9wZ19yZXN0b3JlICB4MSAtZCBwb3N0Z3JlcyAtaiAxMCAt
QyAtLXZlcmJvc2UgLS1mb3JtYXQ9CgphZnRlciBwYXRjaDoKL3BnX3Jlc3RvcmUgIHgxIC1kIHBv
c3RncmVzIC1qIDEwIC1DIC0tdmVyYm9zZSAtLWZvcm1hdD0KcGdfcmVzdG9yZTogZXJyb3I6IHVu
cmVjb2duaXplZCBhcmNoaXZlIGZvcm1hdCAiIjsgcGxlYXNlIHNwZWNpZnkgImMiLCAiZCIsIG9y
ICJ0IgoKTm90ZTogcGdfZHVtcCBhbmQgcGdfZHVtcGFsbCBhcmUgYWxyZWFkeSByZXBvcnRpbmcg
YW4gZXJyb3IgaW4gYWJvdmUgY2FzZS4KCnNhbWUgZml4IGlzIG5lZWRlZCBmb3IgLWgvLS1ob3N0
LCAtcC8tLXBvcnQgb3B0aW9ucyBhbHNvLgotLS0KIHNyYy9iaW4vcGdfZHVtcC9wZ19yZXN0b3Jl
LmMgICB8IDkgKysrLS0tLS0tCiBzcmMvYmluL3BnX2R1bXAvdC8wMDFfYmFzaWMucGwgfCA1ICsr
KysrCiAyIGZpbGVzIGNoYW5nZWQsIDggaW5zZXJ0aW9ucygrKSwgNiBkZWxldGlvbnMoLSkKIG1v
ZGUgY2hhbmdlIDEwMDY0NCA9PiAxMDA3NTUgc3JjL2Jpbi9wZ19kdW1wL3QvMDAxX2Jhc2ljLnBs
CgpkaWZmIC0tZ2l0IGEvc3JjL2Jpbi9wZ19kdW1wL3BnX3Jlc3RvcmUuYyBiL3NyYy9iaW4vcGdf
ZHVtcC9wZ19yZXN0b3JlLmMKaW5kZXggOTFlZmUzMDU2NTAuLmYwMTZiMzM2MzA4IDEwMDY0NAot
LS0gYS9zcmMvYmluL3BnX2R1bXAvcGdfcmVzdG9yZS5jCisrKyBiL3NyYy9iaW4vcGdfZHVtcC9w
Z19yZXN0b3JlLmMKQEAgLTIyNSwxNiArMjI1LDE0IEBAIG1haW4oaW50IGFyZ2MsIGNoYXIgKiph
cmd2KQogCQkJCW9wdHMtPmZpbGVuYW1lID0gcGdfc3RyZHVwKG9wdGFyZyk7CiAJCQkJYnJlYWs7
CiAJCQljYXNlICdGJzoKLQkJCQlpZiAoc3RybGVuKG9wdGFyZykgIT0gMCkKLQkJCQkJb3B0cy0+
Zm9ybWF0TmFtZSA9IHBnX3N0cmR1cChvcHRhcmcpOworCQkJCW9wdHMtPmZvcm1hdE5hbWUgPSBw
Z19zdHJkdXAob3B0YXJnKTsKIAkJCQlicmVhazsKIAkJCWNhc2UgJ2cnOgogCQkJCS8qIHJlc3Rv
cmUgb25seSBnbG9iYWwgc3FsIGNvbW1hbmRzLiAqLwogCQkJCWdsb2JhbHNfb25seSA9IHRydWU7
CiAJCQkJYnJlYWs7CiAJCQljYXNlICdoJzoKLQkJCQlpZiAoc3RybGVuKG9wdGFyZykgIT0gMCkK
LQkJCQkJb3B0cy0+Y3BhcmFtcy5wZ2hvc3QgPSBwZ19zdHJkdXAob3B0YXJnKTsKKwkJCQlvcHRz
LT5jcGFyYW1zLnBnaG9zdCA9IHBnX3N0cmR1cChvcHRhcmcpOwogCQkJCWJyZWFrOwogCQkJY2Fz
ZSAnaic6CQkJLyogbnVtYmVyIG9mIHJlc3RvcmUgam9icyAqLwogCQkJCWlmICghb3B0aW9uX3Bh
cnNlX2ludChvcHRhcmcsICItai8tLWpvYnMiLCAxLApAQCAtMjY0LDggKzI2Miw3IEBAIG1haW4o
aW50IGFyZ2MsIGNoYXIgKiphcmd2KQogCQkJCWJyZWFrOwogCiAJCQljYXNlICdwJzoKLQkJCQlp
ZiAoc3RybGVuKG9wdGFyZykgIT0gMCkKLQkJCQkJb3B0cy0+Y3BhcmFtcy5wZ3BvcnQgPSBwZ19z
dHJkdXAob3B0YXJnKTsKKwkJCQlvcHRzLT5jcGFyYW1zLnBncG9ydCA9IHBnX3N0cmR1cChvcHRh
cmcpOwogCQkJCWJyZWFrOwogCQkJY2FzZSAnUic6CiAJCQkJLyogbm8tb3AsIHN0aWxsIGFjY2Vw
dGVkIGZvciBiYWNrd2FyZHMgY29tcGF0aWJpbGl0eSAqLwpkaWZmIC0tZ2l0IGEvc3JjL2Jpbi9w
Z19kdW1wL3QvMDAxX2Jhc2ljLnBsIGIvc3JjL2Jpbi9wZ19kdW1wL3QvMDAxX2Jhc2ljLnBsCm9s
ZCBtb2RlIDEwMDY0NApuZXcgbW9kZSAxMDA3NTUKaW5kZXggODZlZWMxYjAwNjQuLmZiMWFkZTQ4
ZjFkCi0tLSBhL3NyYy9iaW4vcGdfZHVtcC90LzAwMV9iYXNpYy5wbAorKysgYi9zcmMvYmluL3Bn
X2R1bXAvdC8wMDFfYmFzaWMucGwKQEAgLTIwNiw2ICsyMDYsMTEgQEAgY29tbWFuZF9mYWlsc19s
aWtlKAogCXFyL1xRcGdfcmVzdG9yZTogZXJyb3I6IHVucmVjb2duaXplZCBhcmNoaXZlIGZvcm1h
dCAiZ2FyYmFnZSI7XEUvLAogCSdwZ19kdW1wOiB1bnJlY29nbml6ZWQgYXJjaGl2ZSBmb3JtYXQn
KTsKIAorY29tbWFuZF9mYWlsc19saWtlKAorCVsgJ3BnX3Jlc3RvcmUnLCAnLWYgLScsICctLWZv
cm1hdD0nXSwKKwlxci9cUXBnX3Jlc3RvcmU6IGVycm9yOiB1bnJlY29nbml6ZWQgYXJjaGl2ZSBm
b3JtYXQgIiI7XEUvLAorCSdwZ19yZXN0b3JlOiB1bnJlY29nbml6ZWQgYXJjaGl2ZSBmb3JtYXQg
ZW1wdHkgc3RyaW5nJyk7CisKIGNvbW1hbmRfZmFpbHNfbGlrZSgKIAlbICdwZ19kdW1wJywgJy0t
b24tY29uZmxpY3QtZG8tbm90aGluZycgXSwKIAlxci9wZ19kdW1wOiBlcnJvcjogb3B0aW9uIC0t
b24tY29uZmxpY3QtZG8tbm90aGluZyByZXF1aXJlcyBvcHRpb24gLS1pbnNlcnRzLCAtLXJvd3Mt
cGVyLWluc2VydCwgb3IgLS1jb2x1bW4taW5zZXJ0cy8sCi0tIAoyLjUyLjAKCg==
--00000000000092d8b0064d34de3f--