Re: pg_upgrade: Error out on too many command-line arguments

[Fabien COELHO]

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.

Re: pg_upgrade: Error out on too many command-line arguments

[Tom Lane]

Fabien COELHO  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

Re: pg_upgrade: Error out on too many command-line arguments

[Fabien COELHO]

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.

Re: pg_upgrade: Error out on too many command-line arguments

[Tom Lane]

Fabien COELHO  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

Re: pg_upgrade: Error out on too many command-line arguments

[Fabien COELHO]

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.

Re: pg_upgrade: Error out on too many command-line arguments

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_upgrade: Error out on too many command-line arguments

[Fabien COELHO]

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.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
 	[

Re: pg_upgrade: Error out on too many command-line arguments

[Peter Eisentraut]

On 2019-08-26 17:45, Ibrar Ahmed wrote:
> On Mon, Aug 26, 2019 at 9:46 AM Michael Paquier  > 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

Re: pg_upgrade: Error out on too many command-line arguments

[Ibrar Ahmed]

On Mon, Aug 26, 2019 at 9:46 AM Michael Paquier  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


0001-pgbench-Error-out-on-too-many-command-line-argume.patch
Description: Binary data

Re: pg_upgrade: Error out on too many command-line arguments

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_upgrade: Error out on too many command-line arguments

[Ibrar Ahmed]

On Sun, Aug 25, 2019 at 8:39 PM Julien Rouhaud  wrote:

> On Sun, Aug 25, 2019 at 4:30 PM Tom Lane  wrote:
> >
> > Peter Eisentraut  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


0001-oid2name-Error-out-on-too-many-command-line-argume.patch
Description: Binary data

Re: pg_upgrade: Error out on too many command-line arguments

[Julien Rouhaud]

On Sun, Aug 25, 2019 at 4:30 PM Tom Lane  wrote:
>
> Peter Eisentraut  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.

Re: pg_upgrade: Error out on too many command-line arguments

[Tom Lane]

Peter Eisentraut  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

Re: pg_upgrade: Error out on too many command-line arguments

[Julien Rouhaud]

On Sun, Aug 25, 2019 at 10:52 AM Peter Eisentraut
 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

pg_upgrade: Error out on too many command-line arguments

[Peter Eisentraut]

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:

[0]:
https://www.postgresql.org/message-id/871sdbzizp.fsf%40jsievers.enova.com

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 64bbb562b90bd6272988c608524da2339323f114 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
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