Thanks Nathan for the review and feedback. On Tue, 17 Mar 2026 at 01:04, Nathan Bossart <[email protected]> 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
From e88baae1e8831beda36cb0e6f561816e15658862 Mon Sep 17 00:00:00 2001 From: Mahendra Singh Thalor <[email protected]> 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 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 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 | 5 +++++ 2 files changed, 8 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 91efe305650..f016b336308 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 86eec1b0064..fb1ade48f1d --- a/src/bin/pg_dump/t/001_basic.pl +++ b/src/bin/pg_dump/t/001_basic.pl @@ -206,6 +206,11 @@ 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_restore: unrecognized archive format empty string'); + 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
