pg_log_fatal vs pg_log_error

On Mon, Jun 17, 2019 at 02:19:30PM +0200, Antonin Houska wrote:

I'd expect that the pg_log_fatal() should be called when the error is serious
enough to cause premature exit, but I can see cases where even pg_log_error()
is followed by exit(1). pg_waldump makes me feel that pg_log_error() is used
to handle incorrect user input (before the actual execution started) while
pg_log_fatal() handles error conditions that user does not fully control
(things that happen during the actual execution). But this is rather a guess.

I agree with what you say when pg_log_fatal should be used for an
error bad enough that the binary should exit immediately. In the case
of pg_waldump, not using pg_log_fatal() makes the code more readable
because there is no need to repeat the "Try --help for more
information on a bad argument". Have you spotted other areas of the
code where it makes sense to change a pg_log_error() + exit to a
single pg_log_fatal()?
--
Michael

Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Jun 17, 2019 at 02:19:30PM +0200, Antonin Houska wrote:

I'd expect that the pg_log_fatal() should be called when the error is serious
enough to cause premature exit, but I can see cases where even pg_log_error()
is followed by exit(1). pg_waldump makes me feel that pg_log_error() is used
to handle incorrect user input (before the actual execution started) while
pg_log_fatal() handles error conditions that user does not fully control
(things that happen during the actual execution). But this is rather a guess.

I agree with what you say when pg_log_fatal should be used for an
error bad enough that the binary should exit immediately. In the case
of pg_waldump, not using pg_log_fatal() makes the code more readable
because there is no need to repeat the "Try --help for more
information on a bad argument".

I'd understand this if pg_log_fatal() called exit() itself, but it does not
(unless I miss something).

Have you spotted other areas of the code where it makes sense to change a
pg_log_error() + exit to a single pg_log_fatal()?

I haven't done an exhaustive search so far, but as I mentioned above,
pg_log_fatal() does not seem to be "pg_log_error() + exit()".

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

On Mon, Jun 17, 2019 at 03:39:49PM +0200, Antonin Houska wrote:

I'd understand this if pg_log_fatal() called exit() itself, but it does not
(unless I miss something).

Oops. My apologies. I have my own wrapper of pg_log_fatal() for an
internal tool which does an exit on top of the logging in this case.
You are right the PG code does not exit() in this case.
--
Michael

On 2019-06-17 14:19, Antonin Houska wrote:

Can anyone please give me a hint (and possibly add some comments to the code)
when pg_log_fatal() should be used in frontend code and when it's appropriate
to call pg_log_error()? The current use does not seem very consistent.

For a program that runs in a loop, like for example psql or
pg_receivewal, use error if the program keeps running and fatal if not.
For one-shot programs like for example createdb, there is no difference,
so we have used error in those cases.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:

On 2019-06-17 14:19, Antonin Houska wrote:

Can anyone please give me a hint (and possibly add some comments to the code)
when pg_log_fatal() should be used in frontend code and when it's appropriate
to call pg_log_error()? The current use does not seem very consistent.

For a program that runs in a loop, like for example psql or
pg_receivewal, use error if the program keeps running and fatal if not.
For one-shot programs like for example createdb, there is no difference,
so we have used error in those cases.

That makes sense, but shouldn't then pg_log_fatal() perform exit(EXIT_FAILURE)
internally? Just like elog(FATAL) does on backend side.

Actually there are indications that someone would appreciate such behaviour
even in frontends.

In pg_rewind.h I see:

/* logging support */
#define pg_fatal(...) do { pg_log_fatal(__VA_ARGS__); exit(1); } while(0)

or this in pg_upgrade/util.c:

void
pg_fatal(const char *fmt,...)
{
va_list args;

va_start(args, fmt);
pg_log_v(PG_FATAL, fmt, args);
va_end(args);
printf(_("Failure, exiting\n"));
exit(1);
}

--
Antonin Houska
Web: https://www.cybertec-postgresql.com