Re: pgbench: improve --help and --version parsing - Mailing list pgsql-hackers

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