Re: Some code cleanup for pgbench and pg_verifybackup

Bonjour Michaël,

My 0.02€:

> - pgbench has its own parsing routines for int64 and double, with
> an option to skip errors. That's not surprising in itself, but, for
> strtodouble(), errorOK is always true, meaning that the error strings
> are dead.

Indeed. However, there are "atof" calls for option parsing which should
rather use strtodouble, and that may or may not call it with errorOk as
true or false, it may depend.

> For strtoint64(), errorOK is false only when parsing a Variable, where a
> second error string is generated.

ISTM that it just returns false, there is no message about the parsing
error, hence the message is generated in the function.

> I don't really think that we need to be that pedantic about the type of
> errors generated in those code paths when failing to parse a variable,
> so I'd like to propose a simplification of the code where we reuse the
> same error message as for double, cutting a bit the number of
> translatable strings.

ISTM that point is that errors from the parser are handled differently (by
calling some "yyerror" function which do different things), so they need a
special call for that.

For other cases we would not to have to replicate generating an error
messages for each caller, so it is best done directly in the function. Ok,
currently there is only one call, but there can be more, eg I have a
not-yet submitted patch to add a new option which will need to parse an
int64.

> - pgbench and pg_verifybackup make use of pg_log_fatal() to report
> some failures in code paths dedicated to command-line options, which
> is inconsistent with all the other tools that use pg_log_error().

The semantics for fatal vs error is that an error is somehow handled while
a fatal is not. If the log message is followed by an cold exit, ISTM that
fatal is the right call, and I cannot help if other commands do not do
that. ISTM more logical to align other commands to fatal when appropriate.

> Thoughts?

I'd be in favor of letting it as it is.

--
Fabien.