Re: Frontend error logging style

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Frontend error logging style
Date: 2022-02-22 23:23:19
Message-ID: 913687.1645572199@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Nov 19, 2021 at 5:17 AM Peter Eisentraut
> <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>> If people want to do that kind of thing (I'm undecided whether the
>> complexity is worth it), then make it a different API. The pg_log_*
>> calls are for writing formatted output. They normalized existing
>> hand-coded patterns at the time. We can wrap another API on top of them
>> that does flow control and output. The pg_log_* stuff is more on the
>> level of syslog(), which also just outputs stuff. Nobody is suggesting
>> that syslog(LOG_EMERG) should exit the program automatically. But you
>> can wrap higher-level APIs such as ereport() on top of that that might
>> do that.

> Yeah, that might be a way forward.

This conversation seems to have tailed off without full resolution,
but I observe that pretty much everyone except Peter is on board
with defining pg_log_fatal as pg_log_error + exit(1). So I think
we should just do that, unless Peter wants to produce a finished
alternative proposal.

I've now gone through and fleshed out the patch I sketched upthread.
This exercise convinced me that we absolutely should do something
very like this, because

(1) As it stands, this patch removes a net of just about 1000 lines
of code. So we clearly need *something*.

(2) The savings would be even more, except that people have invented
macros for pg_log_error + exit(1) in at least four places already.
(None of them quite the same of course.) Here I've just fixed those
macro definitions to use pg_log_fatal. In the name of consistency, we
should probably get rid of those macros in favor of using pg_log_fatal
directly; but I've not done so here, since this patch is eyewateringly
long and boring already.

(3) The amount of inconsistency in how we add on details/hints right
now is even worse than I thought. For example, we've got places
burying the lede like this:

- pg_log_error("server version: %s; %s version: %s",
- remoteversion_str, progname, PG_VERSION);
- fatal("aborting because of server version mismatch");
+ pg_log_error("aborting because of server version mismatch");
+ pg_log_error_detail("server version: %s; %s version: %s",
+ remoteversion_str, progname, PG_VERSION);
+ exit(1);

or misidentifying the primary message altogether, like this:

pg_log_error("query failed: %s",
PQerrorMessage(AH->connection));
- fatal("query was: %s", query);
+ pg_log_error_detail("Query was: %s", query);
+ exit(1);

Thoughts?

regards, tom lane

Attachment Content-Type Size
frontend-logging-API-revision-1.patch text/x-diff 198.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2022-02-22 23:24:30 Re: logical decoding and replication of sequences
Previous Message Justin Pryzby 2022-02-22 23:19:48 wal_compression=zstd