pg_upgrade: Error out on too many command-line arguments
I propose the attached patch to make pg_upgrade error out on too many
command-line arguments. This makes it match the behavior of other
PostgreSQL programs.
See [0]/messages/by-id/871sdbzizp.fsf@jsievers.enova.com for an issue related to the lack of this check:
[0]: /messages/by-id/871sdbzizp.fsf@jsievers.enova.com
/messages/by-id/871sdbzizp.fsf@jsievers.enova.com
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-pg_upgrade-Error-out-on-too-many-command-line-argume.patchtext/plain; charset=UTF-8; name=0001-pg_upgrade-Error-out-on-too-many-command-line-argume.patch; x-mac-creator=0; x-mac-type=0Download
From 64bbb562b90bd6272988c608524da2339323f114 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sun, 25 Aug 2019 10:46:07 +0200
Subject: [PATCH] pg_upgrade: Error out on too many command-line arguments
This makes it match the behavior of other PostgreSQL programs.
pg_upgrade doesn't take any non-option arguments, so these are always
a mistake.
---
src/bin/pg_upgrade/option.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index e4093ed5af..6afdbeabc8 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -218,6 +218,9 @@ parseCommandLine(int argc, char *argv[])
}
}
+ if (optind < argc)
+ pg_fatal("too many command-line arguments (first is \"%s\")\n", argv[optind]);
+
if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL)
pg_fatal("could not open log file \"%s\": %m\n", INTERNAL_LOG_FILE);
--
2.22.0
On Sun, Aug 25, 2019 at 10:52 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
I propose the attached patch to make pg_upgrade error out on too many
command-line arguments. This makes it match the behavior of other
PostgreSQL programs.See [0] for an issue related to the lack of this check:
+1
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
I propose the attached patch to make pg_upgrade error out on too many
command-line arguments. This makes it match the behavior of other
PostgreSQL programs.
+1 ... are we missing this anywhere else?
regards, tom lane
On Sun, Aug 25, 2019 at 4:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
I propose the attached patch to make pg_upgrade error out on too many
command-line arguments. This makes it match the behavior of other
PostgreSQL programs.+1 ... are we missing this anywhere else?
I did some searching, and oid2name.c is also missing this.
On Sun, Aug 25, 2019 at 8:39 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
On Sun, Aug 25, 2019 at 4:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
I propose the attached patch to make pg_upgrade error out on too many
command-line arguments. This makes it match the behavior of other
PostgreSQL programs.+1 ... are we missing this anywhere else?
I did some searching, and oid2name.c is also missing this.
Yes, "oid2name" missing that check too.
--
Ibrar Ahmed
Attachments:
0001-oid2name-Error-out-on-too-many-command-line-argume.patchapplication/octet-stream; name=0001-oid2name-Error-out-on-too-many-command-line-argume.patchDownload
diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index d3a4a50005..3682a46602 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -185,6 +185,12 @@ get_opts(int argc, char **argv, struct options *my_opts)
exit(1);
}
}
+ if (optind < argc)
+ {
+ fprintf(stderr, _("too many command-line arguments (first is \"%s\")\n"), argv[optind]);
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
}
static void
On Sun, Aug 25, 2019 at 05:10:47PM +0200, Julien Rouhaud wrote:
I did some searching, and oid2name.c is also missing this.
And pgbench, no?
--
Michael
On Mon, Aug 26, 2019 at 9:46 AM Michael Paquier <michael@paquier.xyz> wrote:
On Sun, Aug 25, 2019 at 05:10:47PM +0200, Julien Rouhaud wrote:
I did some searching, and oid2name.c is also missing this.
And pgbench, no?
Yes, the patch is slightly different.
--
Michael
--
Ibrar Ahmed
Attachments:
0001-pgbench-Error-out-on-too-many-command-line-argume.patchapplication/octet-stream; name=0001-pgbench-Error-out-on-too-many-command-line-argume.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 570cf3306a..f87c06a2dd 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -5493,6 +5493,13 @@ main(int argc, char **argv)
}
}
+ if (optind < (argc-1))
+ {
+ fprintf(stderr, _("too many command-line arguments (first is \"%s\")\n"), argv[optind]);
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+
/* set default script if none */
if (num_scripts == 0 && !is_init_mode)
{
On 2019-08-26 17:45, Ibrar Ahmed wrote:
On Mon, Aug 26, 2019 at 9:46 AM Michael Paquier <michael@paquier.xyz
<mailto:michael@paquier.xyz>> wrote:On Sun, Aug 25, 2019 at 05:10:47PM +0200, Julien Rouhaud wrote:
I did some searching, and oid2name.c is also missing this.
And pgbench, no?
Yes, the patch is slightly different.
Thanks, pushed all that together.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello Peter,
On Sun, Aug 25, 2019 at 05:10:47PM +0200, Julien Rouhaud wrote:
I did some searching, and oid2name.c is also missing this.
And pgbench, no?
Yes, the patch is slightly different.Thanks, pushed all that together.
Great!
Could we maintain coverage by adding a TAP test? See 1 liner attached.
--
Fabien.
Attachments:
pgbench-too-many-1.patchtext/x-diff; name=pgbench-too-many-1.patchDownload
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index f7fa18418b..912312b7ca 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -157,6 +157,7 @@ my @options = (
qr{error while setting random seed from --random-seed option}
]
],
+ [ 'too many args', 'dbname foo', [ qr{too many .*first is "foo"} ] ],
# logging sub-options
[
On Fri, Aug 30, 2019 at 08:44:28AM +0200, Fabien COELHO wrote:
Could we maintain coverage by adding a TAP test? See 1 liner attached.
I don't see why not. Perhaps this could be done for pgbench and
oid2name as well?
--
Michael
Bonjour Michaël,
I don't see why not. Perhaps this could be done for pgbench and
oid2name as well?
This is for pgbench.
I did not found a TAP test in pg_upgrade, I do not think that it is worth
creating one for that purpose. The "test.sh" script does not seem
appropriate for this kind of coverage error test.
The TAP test for oid2name only includes basic checks, options and
arguments are not even tested, and there is no infra for actual testing,
eg starting a server… Improving that would be a much more significant
effort that the one line I added to pgbench existing TAP test.
--
Fabien.
Fabien COELHO <coelho@cri.ensmp.fr> writes:
Could we maintain coverage by adding a TAP test? See 1 liner attached.
Is this issue *really* worth expending test cycles on forevermore?
Test cycles are not free, and I see zero reason to think that a
check of this sort would ever catch any bugs. Now, if you had a
way to detect that somebody had forgotten the case in some new
program, that would be interesting.
regards, tom lane
Hello Tom,
Could we maintain coverage by adding a TAP test? See 1 liner attached.
Is this issue *really* worth expending test cycles on forevermore?
With this argument consistently applied, postgres code coverage is
consistently weak, with 25% of the code never executed, and 15% of
functions never called. "psql" is abysmal, "libpq" is really weak.
Test cycles are not free, and I see zero reason to think that a
check of this sort would ever catch any bugs. Now, if you had a
way to detect that somebody had forgotten the case in some new
program, that would be interesting.
It could get broken somehow, and the test would catch it?
That would be the only command which tests this feature?
This is a TAP test, not a test run on basic "make check". The cost is not
measurable: pgbench 533 TAP tests run in 5 wallclock seconds, and this
added test does not change that much.
Now, if you say you are against it, then it is rejected…
--
Fabien.
Fabien COELHO <coelho@cri.ensmp.fr> writes:
Is this issue *really* worth expending test cycles on forevermore?
With this argument consistently applied, postgres code coverage is
consistently weak, with 25% of the code never executed, and 15% of
functions never called. "psql" is abysmal, "libpq" is really weak.
It's all a question of balance. If you go too far in the other
direction, you end up with test suites that take an hour and a half
to run so nobody ever runs them (case in point, mysql). I'm all for
improving coverage in meaningful ways --- but cases like this seem
unlikely to be worth ongoing expenditures of testing effort.
regards, tom lane
Is this issue *really* worth expending test cycles on forevermore?
With this argument consistently applied, postgres code coverage is
consistently weak, with 25% of the code never executed, and 15% of
functions never called. "psql" is abysmal, "libpq" is really weak.It's all a question of balance. If you go too far in the other
direction, you end up with test suites that take an hour and a half
to run so nobody ever runs them (case in point, mysql). I'm all for
improving coverage in meaningful ways --- but cases like this seem
unlikely to be worth ongoing expenditures of testing effort.
Sure.
I think there is room for several classes of tests, important ones always
run and others run say by the farm, and I thought that is what TAP tests
were for, given they are quite expensive anyway (eg most TAP test create
their own postgres instance).
So for me the suggestion was appropriate for a pgbench-specific TAP test.
--
Fabien.