Обсуждение: 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] 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

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 05:10:47PM +0200, Julien Rouhaud wrote:
> I did some searching, and oid2name.c is also missing this.

And pgbench, no?
--
Michael

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.

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.