From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Some code cleanup for pgbench and pg_verifybackup
Date: Mon, 26 Jul 2021 10:13:27 +0200 (CEST)
Message-ID: <alpine.DEB.2.22.394.2107260946550.297424@pseudo> (raw)
In-Reply-To: <YP5dtCVuVP6boi69@paquier.xyz>
References: <YP5dtCVuVP6boi69@paquier.xyz>
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.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: coelho@cri.ensmp.fr, michael@paquier.xyz, pgsql-hackers@lists.postgresql.org
Subject: Re: Some code cleanup for pgbench and pg_verifybackup
In-Reply-To: <alpine.DEB.2.22.394.2107260946550.297424@pseudo>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox