Обсуждение: pgbench: improve --help and --version parsing

Hi,

There is a small catch in the parsing of --help and --version args by pgbench:
they are processed correctly only as the first argument. If it's not the case,
strange error message occurs:

$ pgbench -q --help
pgbench: unrecognized option '--help'
Try "pgbench --help" for more information.

The reason for this behavior is how these two arguments are handled in
src/bin/pgbench/pgbench.c:

if (argc > 1)
{
    if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
    {
        usage();
        exit(0);
    }
    if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
    {
        puts("pgbench (PostgreSQL) " PG_VERSION);
        exit(0);
    }
}

All other arguments are processed with getopt_long. The proposed patch replaces
the existing way of parsing the --help and --version arguments with getopt_long,
expanding the existing switch statement.

--
Andrei Korigodski

Andrei Korigodski <akorigod@gmail.com> writes:
> There is a small catch in the parsing of --help and --version args by pgbench:
> they are processed correctly only as the first argument.

This is, in fact, how it's done in all PG apps.

            regards, tom lane

On July 21, 2018 11:15:51 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Andrei Korigodski <akorigod@gmail.com> writes:
>> There is a small catch in the parsing of --help and --version args by
>pgbench:
>> they are processed correctly only as the first argument.
>
>This is, in fact, how it's done in all PG apps.

Think there's a fair argument that we should improve that at some point...

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Andres Freund <andres@anarazel.de> writes:
> On July 21, 2018 11:15:51 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This is, in fact, how it's done in all PG apps.

> Think there's a fair argument that we should improve that at some point...

Perhaps.  Peter E. might remember why it's like that.  But I'm dubious
about changing it in only one app.

            regards, tom lane

On July 21, 2018 11:52:05 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>But I'm dubious
>about changing it in only one app.

Agreed.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Andres Freund <andres@anarazel.de> writes:
> > On July 21, 2018 11:15:51 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > This is, in fact, how it's done in all PG apps.
> > Think there's a fair argument that we should improve that at some point...
> Perhaps.  Peter E. might remember why it's like that.

It was done this way because then there was HAVE_GETOPT_LONG define for systems
that doesn't support getopt_long, see commit 41fde5460387 ("Polish help output.
Allow --help to work with BSD getopts.", Peter Eisentraut, 2001-01-06). Now this
define is not used by any app in src/bin, so I believe there is no need for this
workaround anymore.

By the way, this approach is already not used in pg_waldump and psql handles the
arguments more complex way to avoid the problem under discussion.

> But I'm dubious about changing it in only one app.

Agreed. I have changed handling of the --help and --version options in all apps
where it exhibits the problem described, with the exception for pg_archivecleanup
where getopt is used instead of getopt_long. The separate patch will be proposed
to address it.

The patch is against current master. All tests pass.

> Agreed. I have changed handling of the --help and --version options in all apps
> where it exhibits the problem described, with the exception for pg_archivecleanup
> where getopt is used instead of getopt_long. The separate patch will be proposed
> to address it.
>
> The patch is against current master. All tests pass.

I doubt that -V & -? are heavily tested:-) Patch works for me, though.

There seems to be other instances as well:

  ./src/interfaces/ecpg/preproc/ecpg.c:   while ((c = getopt_long(argc, argv, "vcio:I:tD:dC:r:h", ecpg_options, NULL))
!=-1)
 
  ./src/bin/scripts/clusterdb.c:  while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:at:v", long_options, &optindex)) !=
-1)
  ./src/bin/scripts/createdb.c:   while ((c = getopt_long(argc, argv, "h:p:U:wWeO:D:T:E:l:", long_options, &optindex))
!=-1)
 
  ./src/bin/scripts/dropuser.c:   while ((c = getopt_long(argc, argv, "h:p:U:wWei", long_options, &optindex)) != -1)
  ./src/bin/scripts/pg_isready.c: while ((c = getopt_long(argc, argv, "d:h:p:qt:U:", long_options, NULL)) != -1)
  ./src/bin/scripts/dropdb.c:     while ((c = getopt_long(argc, argv, "h:p:U:wWei", long_options, &optindex)) != -1)
  ./src/bin/scripts/vacuumdb.c:   while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:zZFat:fvj:", long_options,
&optindex))!= -1)
 
  ./src/bin/scripts/createuser.c: while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSaArRiIlLc:PE",
  ./src/bin/scripts/reindexdb.c:  while ((c = getopt_long(argc, argv, "h:p:U:wWeqS:d:ast:i:v", long_options,
&optindex))!= -1)
 

  ./src/interfaces/ecpg/preproc/ecpg.c:           if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
  ./src/timezone/zic.c:           else if (strcmp(argv[k], "--help") == 0)
  ./src/backend/main/main.c:              if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
  ./src/bin/pg_archivecleanup/pg_archivecleanup.c:                if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1],
"-?")== 0)
 
  ./src/bin/pg_upgrade/option.c:          if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
  ./src/bin/pg_ctl/pg_ctl.c:              if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
  ./src/bin/psql/startup.c:               if ((strcmp(argv[1], "-?") == 0) || (argc == 2 && (strcmp(argv[1], "--help")
==0)))
 
  ./src/bin/scripts/common.c:             if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
     => implementation shared by many C "scripts".
  ./src/bin/pg_config/pg_config.c:                if (strcmp(argv[i], "--help") == 0 || strcmp(argv[i], "-?") == 0)
  ./contrib/oid2name/oid2name.c:          if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
  ./contrib/pg_standby/pg_standby.c:              if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
  ./contrib/vacuumlo/vacuumlo.c:          if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:
> There seems to be other instances as well

Thanks! I made the same modification of src/interfaces/ecpg/preproc/ecpg.c, but
in other cases it's either not a problem (as with src/bin/psql/startup.c or
src/timezone/zic.c) or the solution is too complex to be added to the current
patch: e.g. these tools in src/bin/scripts use handle_help_version_opts function
from src/bin/scripts/common.c which cannot be refactored so straightforward.
It's possible to remove it and modify each tool to parse the --help and
--version args for themselves but I would not include it in the same patch as
it's already not too short for a "fix" patch and these changes are better to be
discussed separately IMHO. Do you think the handle_help_version_opts function
should be removed and these args should be parsed in each src/bin/scripts app?

There are several cases where separate comparisons of argv[1] are made to detect
"--help" or "--version" before non-root user is enforced (to make it possible to
the root user to check the version) e.g. src/bin/pg_upgrade/option.c — in this
case I left this comparisons untouched while expanding the switch statement of
getopt_long, so non-root user sees the correct behavior and root still sees
"cannot be run as root" error when trying # pg_upgrade -v --help. The
alternative is to wrap these argv[...] comparisons in a for statement to
iterate through all the arguments. Another option is to enforce non-root after
getopt_long parsing but it's harder to be sure that the patch does not alter
the apps behavior unexpected way.

There are also the few apps when getopt is used instead of getopt_long, so I
decided not to fix these in the current patch because it's not so obvious. It's
possible either to replace getopt with getopt_long (and add long options, which
may be nice) or wrap --help/--version parsing in a for loop.

--
Andrei

Hello

My 0.02€:

> Thanks! I made the same modification of src/interfaces/ecpg/preproc/ecpg.c, but
> in other cases it's either not a problem (as with src/bin/psql/startup.c or
> src/timezone/zic.c) or the solution is too complex to be added to the current
> patch: e.g. these tools in src/bin/scripts use handle_help_version_opts function
> from src/bin/scripts/common.c which cannot be refactored so straightforward.

I think this function could be simply removed: it is just calling its 
third argument on help, all printing the version with the command name on 
version.

> It's possible to remove it and modify each tool to parse the --help and
> --version args for themselves but I would not include it in the same patch as
> it's already not too short for a "fix" patch and these changes are better to be
> discussed separately IMHO. Do you think the handle_help_version_opts function
> should be removed and these args should be parsed in each src/bin/scripts app?

Given the contents of "handle_help_version_opts", I see no issue with 
managing an update in the same patch as other commands, if this is to be 
done.

> There are several cases where separate comparisons of argv[1] are made to detect
> "--help" or "--version" before non-root user is enforced (to make it possible to
> the root user to check the version) e.g. src/bin/pg_upgrade/option.c
> — in this case I left this comparisons untouched while expanding the 
> switch statement of getopt_long, so non-root user sees the correct 
> behavior and root still sees "cannot be run as root" error when trying # 
> pg_upgrade -v --help.

This seems reasonable.

> The alternative is to wrap these argv[...] comparisons in a for 
> statement to iterate through all the arguments. Another option is to 
> enforce non-root after getopt_long parsing but it's harder to be sure 
> that the patch does not alter the apps behavior unexpected way.

Personnaly I would not bother that a must-not-be-root command does not 
process its options when called as root, but people may object on this 
one.

> There are also the few apps when getopt is used instead of getopt_long, so I
> decided not to fix these in the current patch because it's not so obvious. It's
> possible either to replace getopt with getopt_long (and add long options, which
> may be nice) or wrap --help/--version parsing in a for loop.

I'd go for homogeneity.

About the patch.

Some commands take care to manage their option struct and/or switch cases 
more or less in alphabetical order (with errors, eg dbname/d in pg_dumpall 
is misplaced), and that you strive to be consistent with that. You missed
on some cases though: pg_test_fsync, pg_test_timing, ecpg.

For some commands (initdb, pg_basebackup, pg_receivewal...), I see that 
?/V are already in the option struct but the option management is missing 
from the switch, so this is also fixing minor bugs.

You have changed the behavior in some commands: eg -? in pg_rewind used to 
point to --help, now it directly provide said help. I'm fine with that.

For commands which do not use getopt_long, probably the change belongs to 
another patch.

-- 
Fabien.

On Sun, Jul 22, 2018 at 10:19:50AM -0400, Fabien COELHO wrote:
>> Agreed. I have changed handling of the --help and --version options in all apps
>> where it exhibits the problem described, with the exception for pg_archivecleanup
>> where getopt is used instead of getopt_long. The separate patch will be proposed
>> to address it.
>>
>> The patch is against current master. All tests pass.
>
> I doubt that -V & -? are heavily tested:-) Patch works for me, though.

They are not, and the patch misses this area.

I don't think that it is a bad idea to improve things the way you are
doing, however you should extend program_version_ok() and
program_help_ok() in src/test/perl/TestLib.pm so as short options are
tested for two reasons:
1) We can make sure that everything is consistent and works properly
easily without testing manually your patch, which is a pain for anybody
looking at the patch.
2) Any new standalone binary added in the core tree would be able to
check natively if it handles common options correctly with TAP tests
added.

This will need tweaks for the number of tests in a couple of TAP files,
but that's worth the shot in the long term.
--
Michael

Hello Michaël,

>> I doubt that -V & -? are heavily tested:-) Patch works for me, though.
>
> They are not, and the patch misses this area.

Indeed.

> I don't think that it is a bad idea to improve things the way you are

For the record, this is not my patch, I'm merely reviewing it.

> doing, however you should extend program_version_ok() and 
> program_help_ok() in src/test/perl/TestLib.pm so as short options are 
> tested for two reasons:

Interesting, I did not notice these functions before. I fully agree that 
manual testing is a pain for such a simple change.

Do you mean something like the attached?

-- 
Fabien.

On Mon, Jul 23, 2018 at 07:47:44AM -0400, Fabien COELHO wrote:
>> I don't think that it is a bad idea to improve things the way you are
>
> For the record, this is not my patch, I'm merely reviewing it.

Of course, any input is welcome.  It is nice to see that you took some
time to look at the patch.

>> doing, however you should extend program_version_ok() and
>> program_help_ok() in src/test/perl/TestLib.pm so as short options are
>> tested for two reasons:
>
> Interesting, I did not notice these functions before. I fully agree that
> manual testing is a pain for such a simple change.
>
> Do you mean something like the attached?

You basically have the idea, except that the number of tests in any TAP
files calling program_help_ok and program_version_ok needs to be
updated, and that the test is too verbose :)

>  # Version
> -pgbench('-V', 0, [qr{^pgbench .PostgreSQL. }], [qr{^$}], 'pgbench version');
> +pgbench('-V', 0, [qr{^pgbench \(PostgreSQL\) \d+}], [qr{^$}], 'pgbench version');

This could go away.

> +    is($stdout, $stdout2, "$cmd --help and -? have same output");
> +    like($stdout, qr{Usage:}, "$cmd --help is about usage");
> +    like($stdout, qr{\b$cmd\b}, "$cmd --help is about $cmd");
> +    like($stdout, qr{--help}, "$cmd --help is about option --help");
> +    like($stdout, qr{-\?}, "$cmd --help is about options -?");
> +    like($stdout, qr{--version}, "$cmd --help is about options --version");
> +    like($stdout, qr{-V}, "$cmd --help is about options -V");

I would keep things a bit more simple by not parsing the output as you
do, and it would be possible to reduce that to one single test with a
text block as well, say using qq().

Andrei, could you update your patch in this area please?  That will ease
reviews and checks.  Double-checking that the documentation gets the
correct call would not hurt as well.
--
Michael

Hello Michaël,

>> Do you mean something like the attached?
>
> You basically have the idea, except that the number of tests in any TAP
> files calling program_help_ok and program_version_ok

Indeed. I wanted to outline the perl module part, really.

> needs to be updated, and that the test is too verbose :)
>
>>  # Version
>> -pgbench('-V', 0, [qr{^pgbench .PostgreSQL. }], [qr{^$}], 'pgbench version');
>> +pgbench('-V', 0, [qr{^pgbench \(PostgreSQL\) \d+}], [qr{^$}], 'pgbench version');
>
> This could go away.

Indeed.

>> +    is($stdout, $stdout2, "$cmd --help and -? have same output");
>> +    like($stdout, qr{Usage:}, "$cmd --help is about usage");
>> +    like($stdout, qr{\b$cmd\b}, "$cmd --help is about $cmd");
>> +    like($stdout, qr{--help}, "$cmd --help is about option --help");
>> +    like($stdout, qr{-\?}, "$cmd --help is about options -?");
>> +    like($stdout, qr{--version}, "$cmd --help is about options --version");
>> +    like($stdout, qr{-V}, "$cmd --help is about options -V");
>
> I would keep things a bit more simple by not parsing the output as you
> do, and it would be possible to reduce that to one single test with a
> text block as well, say using qq().

I do not understand what you mean by "reduce that to one single test". I 
cannot see how to test stdouts equalities and contents' sanity in one 
single test.

-- 
Fabien.

On Thu, Jul 26, 2018 at 06:22:16PM -0400, Fabien COELHO wrote:
> I do not understand what you mean by "reduce that to one single test". I
> cannot see how to test stdouts equalities and contents' sanity in one single
> test.

I was wondering about reducing the number of tests to a strict minimum.
At the end I think that I would just remove most of the tests you are
adding here, they seem rather noisy..
--
Michael

>> I do not understand what you mean by "reduce that to one single test". I
>> cannot see how to test stdouts equalities and contents' sanity in one single
>> test.
>
> I was wondering about reducing the number of tests to a strict minimum.
> At the end I think that I would just remove most of the tests you are
> adding here, they seem rather noisy..

Ok, if you remove tests, it can be done in less tests, obviously.

ISTM that some minimal sanity on the output would be useful. If some 
minimal common format is assumed, one re could check that the name of the 
program appears, and a few options are indeed describe. That could make 
one "like" test.

-- 
Fabien.

On Thu, Jul 26, 2018 at 06:42:02PM -0400, Fabien COELHO wrote:
> Ok, if you remove tests, it can be done in less tests, obviously.

Well, if we can find a way to reduce the number of tests but not their
coverage, that's always welcome.

> ISTM that some minimal sanity on the output would be useful. If some minimal
> common format is assumed, one re could check that the name of the program
> appears, and a few options are indeed describe. That could make one "like"
> test.

Let's see the patch author's take on the matter then.
--
Michael

Hello.

Sorry for not getting into this but I'm looking forward to this.
(I'm tired to insert '--help' to the top of argument list..)

This patch still applies to the current master but ecpg.c.

At Sun, 22 Jul 2018 16:13:20 -0400 (EDT), Fabien COELHO <coelho@cri.ensmp.fr> wrote in
<alpine.DEB.2.21.1807221540080.13768@lancre>
> 
> Hello
> 
> My 0.02€:
> 
> > Thanks! I made the same modification of
> > src/interfaces/ecpg/preproc/ecpg.c, but
> > in other cases it's either not a problem (as with
> > src/bin/psql/startup.c or
> > src/timezone/zic.c) or the solution is too complex to be added to the
> > current
> > patch: e.g. these tools in src/bin/scripts use
> > handle_help_version_opts function
> > from src/bin/scripts/common.c which cannot be refactored so
> > straightforward.
> 
> I think this function could be simply removed: it is just calling its
> third argument on help, all printing the version with the command name
> on version.

Still harmonized version string is useful. We can remove the
function simply and add print_version_string(const char
*fixed_progname) instead.

> > It's possible to remove it and modify each tool to parse the --help
> > and
> > --version args for themselves but I would not include it in the same
> > --patch as
> > it's already not too short for a "fix" patch and these changes are
> > better to be
> > discussed separately IMHO. Do you think the handle_help_version_opts
> > function
> > should be removed and these args should be parsed in each
> > src/bin/scripts app?
> 
> Given the contents of "handle_help_version_opts", I see no issue with
> managing an update in the same patch as other commands, if this is to
> be done.

+1.

> > There are several cases where separate comparisons of argv[1] are made
> > to detect
> > "--help" or "--version" before non-root user is enforced (to make it
> > possible to
> > the root user to check the version) e.g. src/bin/pg_upgrade/option.c
> > — in this case I left this comparisons untouched while expanding the
> > switch statement of getopt_long, so non-root user sees the correct
> > behavior and root still sees "cannot be run as root" error when trying
> > # pg_upgrade -v --help.
> 
> This seems reasonable.
> 
> > The alternative is to wrap these argv[...] comparisons in a for
> > statement to iterate through all the arguments. Another option is to
> > enforce non-root after getopt_long parsing but it's harder to be sure
> > that the patch does not alter the apps behavior unexpected way.
> 
> Personnaly I would not bother that a must-not-be-root command does not
> process its options when called as root, but people may object on this
> one.

I forgot the detailed discussion for the code but looking with a
fresh eyes, the only thing we should avoid there is creating a
log file with superuser privilege. All other stuff in the getopt
loop is harmless. So just inhibiting root to make the internal
log file before the loop, then exiting if root after it would
work. Some error messages can be emitted to stderr but it doesn't
matter.

> > There are also the few apps when getopt is used instead of
> > getopt_long, so I
> > decided not to fix these in the current patch because it's not so
> > obvious. It's
> > possible either to replace getopt with getopt_long (and add long
> > options, which
> > may be nice) or wrap --help/--version parsing in a for loop.
> 
> I'd go for homogeneity.

+1.

> About the patch.
> 
> Some commands take care to manage their option struct and/or switch
> cases more or less in alphabetical order (with errors, eg dbname/d in
> pg_dumpall is misplaced), and that you strive to be consistent with
> that. You missed
> on some cases though: pg_test_fsync, pg_test_timing, ecpg.
> 
> For some commands (initdb, pg_basebackup, pg_receivewal...), I see
> that ?/V are already in the option struct but the option management is
> missing from the switch, so this is also fixing minor bugs.
> 
> You have changed the behavior in some commands: eg -? in pg_rewind
> used to point to --help, now it directly provide said help. I'm fine
> with that.

Mmm. This is related to getopt's specification. getopt_long
returns '?' for ambiguous match or extraneous parameter. So any
wrong parameter or even --help let the program to show only "try
--help" and we no longer have a means to see the true help
message without the change. In other words pg_rewind's (or all
our programs using -? as the synonum of --help) original behavior
can be incompatible with getopt's behavior. We should redefine
the behavior for -?, --help and unknown options respectively
basing getopts's behavior. We can identify --help using
option_index returnd from getopt_long if it were not at the
beginning of long_options:p

> For commands which do not use getopt_long, probably the change belongs
> to another patch.

Agreed.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center