Frontend error logging style

At Tue, 09 Nov 2021 17:20:41 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in

ISTM that the recently-introduced new frontend logging support
(common/logging.h et al) could use a second pass now that we have
some experience with it. There are two points that are bugging me:

1. The distinction between "error" and "fatal" levels seems squishy
to the point of uselessness. I think we should either get rid of it
entirely, or make an effort to use "fatal" exactly for the cases that
are going to give up and exit right away. Of the approximately 830
pg_log_error calls in HEAD, I count at least 450 that are immediately
followed by exit(1), and so should be pg_log_fatal if this distinction
means anything at all. OTOH, if we decide it doesn't mean anything,
there are only about 90 pg_log_fatal calls to convert. I lean
slightly to the "get rid of the distinction" option, not only because
it'd be a much smaller patch but also because I don't care to expend
brain cells on the which-to-use question while reviewing future
patches.

Agreed. On backends, FATAL is bound to a behavior defferent from
ERROR. I doubt of the necessity of the difference between the two on
a console application (or frontend in PG-term). I would prefer to get
rid of the FATAL level.

I once thought ERROR should cause outoright end of an application, but
things don't seem so simple..

2. What is the preferred style for adding extra lines to log messages?
I see a lot of direct prints to stderr:

pg_log_error("missing required argument: database name");
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);

but a number of places have chosen to do this:

pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_error("query was: %s", todo);

and some places got creative and did this:

pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_info("query was: %s", sql.data);

In the first place the pg_log_info doesn't work at all since pgbench
doesn't make the log-level go below PG_LOG_INFO. (--quiet works the
different way..) In that case the pg_log_info can even be said to be
used just to avoid the message from being prepended by a severity tag.
I think we should fix that.

I think this ought to be cleaned up so we have a more-or-less uniform
approach. Aside from the randomly different source code, each of
these choices has different implications for whether the extra line
gets printed or not depending on verbosity level, and that seems bad.

One plausible choice is to drop the first style (which surely has
little to recommend it) and use either the second or third style
depending on whether you think the addendum ought to appear at the
same or higher verbosity level as the main message. But we'd
still be at hazard of people making randomly different choices
about that in identical cases, as indeed the second and third
examples show.

Another idea is to reduce such cases to one call:

pg_log_error("query failed: %s\nquery was: %s",
PQerrorMessage(conn), sql.data);

but I don't much like that: it knows more than it should about
the presentation format, and it can't support hiding the detail
portion at lower verbosity levels.

So I'm not totally satisfied with these ideas, but I don't
immediately have a better one.

Thoughts?

Honestly I don't like that:p

The cause of the confusion is we are using verbosity to hide *a part
of* a grouped messages. We make distinction beween the two concepts
in the backend code but not in frontend. At least in pgbench, it
seems to me as if the severity tag prepended by the pg_log_hoge
functions is mere a label just to be shown on console.

That being said it is a kind of impssible to expect users to specify
the two independent levels on command line.

Couldn't we have a function, say, named as "pg_log_error_detail",
which prints the message when __pg_log_level is *above* ERROR and
prepended by "detail:" tag? (or not showing the tag, keeping the
current behavior on surface.) We would have the same for
pg_log_warning. (pg_log_error_detail behaves a bit differetnly from
pg_log_info in this definition.)

pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_error_detail("query was: %s", sql.data);

So pg_log_generic(_v) have an additional parameter, say, detailed bool.

pg_log_generic(enum pg_log_level level, bool detailed, const char *pg_restrict fmt,...)

Considering that the level is used only to identify severity tag
string, it is somewhat strange that the detailed flag conflicts with
it. But I'm not sure it is sane to add instead an extra-and-hidden log
level to pg_log_level..

Or as a simpler way, we could just have aliases for pg_log_info().

#define pg_log_error_detail(...) pg_log_info(__VA_ARGS__)
#define pg_log_warning_detail(...) pg_log_info(__VA_ARGS__)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Nov 9, 2021 at 5:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

1. The distinction between "error" and "fatal" levels seems squishy
to the point of uselessness.

2. What is the preferred style for adding extra lines to log messages?

I agree with this list of problems. I think that the end game here is
getting to be able to use ereport() and friends in the frontend, which
would require confronting both of these issues at a deep level. We
don't necessarily have to do that now, though, but I think it's an
argument against just nuking "fatal" from orbit. What I think we ought
to be driving towards is having pg_log_fatal() forcibly exit, and
pg_log_error() do the same unless the error is somehow caught. That
might require more changes than you or whoever wants to do right now,
so perhaps what we ought to do is just enforce the policy you
suggested before: if we're going to exit immediately afterward, it's
fatal; if not, it's an error.

I have been wondering for some time about trying to make the frontend
and backend facilities symmetric and using case to distinguish. That
is, if you see:

ERROR: this stinks
DETAIL: It smells very bad.
CONTEXT: garbage dump

...well then that's a backend message. And if you see:

error: this stinks
detail: It smells very bad.
context: garbage dump

...well then that's a frontend message. I don't completely love that
way of making a distinction, but I think we need something, and that's
pretty nearly the present practice at least for the primary message.
We don't really have a solid convention for detail/context/hint on the
FE side; this is one idea.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

I agree with this list of problems. I think that the end game here is
getting to be able to use ereport() and friends in the frontend, which
would require confronting both of these issues at a deep level. We
don't necessarily have to do that now, though, but I think it's an
argument against just nuking "fatal" from orbit. What I think we ought
to be driving towards is having pg_log_fatal() forcibly exit, and
pg_log_error() do the same unless the error is somehow caught.

Perhaps. The usage that I'm concerned about is exemplified by this
common pattern:

pg_log_error("%s", PQerrorMessage(conn));
PQfinish(conn);
exit(1);

ie there's some cleanup you want to do between emitting the error and
actually exiting. (I grant that maybe someday the PQfinish could be
done in an atexit handler or the like, but that's way more redesign
than I want to do right now.) In the case where you *don't* have
any cleanup to do, though, it'd be nice to just write one line not
two.

That leads me to think that we should redefine pg_log_fatal as

#define pg_log_fatal(...) (pg_log_error(__VA_ARGS__), exit(1))

We still want to get rid of the distinct PG_LOG_FATAL log level,
because whether you are using this form of pg_log_fatal or writing
exit() separately is a coding detail that should not manifest
as visibly distinct user output.

This proposal doesn't move us very far towards your endgame,
but I think it's a reasonable incremental step, and it makes
the difference between pg_log_error() and pg_log_fatal()
clear and useful.

I have been wondering for some time about trying to make the frontend
and backend facilities symmetric and using case to distinguish. That
is, if you see:

ERROR: this stinks
DETAIL: It smells very bad.
CONTEXT: garbage dump

...well then that's a backend message. And if you see:

error: this stinks
detail: It smells very bad.
context: garbage dump

...well then that's a frontend message. I don't completely love that
way of making a distinction, but I think we need something, and that's
pretty nearly the present practice at least for the primary message.
We don't really have a solid convention for detail/context/hint on the
FE side; this is one idea.

Hmm, interesting. Taking up my point #2, I'd been thinking about
proposing that we convert

pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_error("query was: %s", todo);

to

pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_error_detail("Query was: %s", todo);

and similarly add a pg_log_info_detail() companion for pg_log_info(), etc.
With your point above, the prefix these'd print would be "detail:" not
"DETAIL:", but otherwise it seems to match up pretty well.

Also, if we are modeling this on backend conventions, we should say that
style rules for pg_log_error_detail match those for errdetail(), with
complete sentences and so forth. I wonder whether "detail:" followed
by a capitalized sentence would look weird ... but you did it above,
so maybe that's what people would expect.

Again, there's more that could be done later, but this seems to be
enough to address the cases that people have been inventing ad-hoc
solutions for.

regards, tom lane

I wrote:

Hmm, interesting. Taking up my point #2, I'd been thinking about
proposing that we convert
pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_error("query was: %s", todo);
to
pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_error_detail("Query was: %s", todo);

After looking around a bit, I see that a lot of these add-on messages
are more nearly hints than details, so we'd probably better support
both those cases right off the bat.

To move things along a bit, here's a draft patch to logging.h/.c only
to implement what I'm envisioning. I don't think there's much point
in doing the per-call-site gruntwork until we have agreement on what
the API is, so this seems like enough for discussion.

(As a fervent hater of colorization, I don't have an opinion about
whether or how to colorize the "detail:" and "hint:" fragments.
But I'll happily take somebody else's adjustment to add that.)

regards, tom lane

At Wed, 10 Nov 2021 14:25:08 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in

I wrote:

Hmm, interesting. Taking up my point #2, I'd been thinking about
proposing that we convert
pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_error("query was: %s", todo);
to
pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_error_detail("Query was: %s", todo);

After looking around a bit, I see that a lot of these add-on messages
are more nearly hints than details, so we'd probably better support
both those cases right off the bat.

Sounds reasonable.

To move things along a bit, here's a draft patch to logging.h/.c only
to implement what I'm envisioning. I don't think there's much point
in doing the per-call-site gruntwork until we have agreement on what
the API is, so this seems like enough for discussion.

(As a fervent hater of colorization, I don't have an opinion about
whether or how to colorize the "detail:" and "hint:" fragments.
But I'll happily take somebody else's adjustment to add that.)

(:) I don't hate colorization so much, but I'm frequently annoyed by
needing to turn off colorization and I disgust when there's no easy
way to disable that in a simple steps..)

Aren't DETAIL and HINT expected to be hidden at the targetted cutoff
level? In other words, I suspect that people want to hide non-primary
messages for a lower verbosity level. On the other hand I'm not sure
it is a proper behavior that log_level = WARNING causes ERROR messages
are accompanied by DETAIL/HINT submessages...

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:

Aren't DETAIL and HINT expected to be hidden at the targetted cutoff
level? In other words, I suspect that people want to hide non-primary
messages for a lower verbosity level. On the other hand I'm not sure
it is a proper behavior that log_level = WARNING causes ERROR messages
are accompanied by DETAIL/HINT submessages...

I abandoned that idea in the draft patch. We could maybe do something
about it further down the line, but I'm not sure there's really any
demand.

As the patch is set up, you could theoretically do something like

pg_log_error("blah blah");
pg_log_info_detail("Very boring detail goes here.");

(note the intentionally different log priorities). But that feels wrong
to me --- it doesn't seem like individual call sites should be setting
such policy. If we do arrange for a way to hide the optional message
parts, I'd rather that the control were centralized in logging.c.

It certainly wouldn't be hard for logging.c to make different decisions
about what to print; the thing that's not clear to me is what the
user-level knob for it should look like. We already used up the
option of more or fewer -v switches.

regards, tom lane

On 09.11.21 23:20, Tom Lane wrote:

1. The distinction between "error" and "fatal" levels seems squishy
to the point of uselessness. I think we should either get rid of it
entirely, or make an effort to use "fatal" exactly for the cases that
are going to give up and exit right away. Of the approximately 830
pg_log_error calls in HEAD, I count at least 450 that are immediately
followed by exit(1), and so should be pg_log_fatal if this distinction
means anything at all. OTOH, if we decide it doesn't mean anything,
there are only about 90 pg_log_fatal calls to convert. I lean
slightly to the "get rid of the distinction" option, not only because
it'd be a much smaller patch but also because I don't care to expend
brain cells on the which-to-use question while reviewing future
patches.

This logging system has been designed more generally, drawing some
inspiration from Python and Java libraries, for example. It's up to the
program using this to make sensible use of it. I think there are
programs such as pg_receivewal where there is a meaningful distinction
between errors flowing by and a fatal exit. But with something like
pg_basebackup, there is no real difference, so that code sort of uses
pg_log_error by default, since any error is implicitly fatal. I see the
apparent inconsistency, but I don't think it's a real problem. Each
program by itself has arguably sensible behavior.

On 09.11.21 23:20, Tom Lane wrote:

2. What is the preferred style for adding extra lines to log messages?
I see a lot of direct prints to stderr:

pg_log_error("missing required argument: database name");
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);

This is mainly used for those few messages that are sort of a "global
standard".

but a number of places have chosen to do this:

pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_error("query was: %s", todo);

and some places got creative and did this:

pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_info("query was: %s", sql.data);

I can't decide between those two. It depends on how each program
handles the log level internally.

Some kind of "detail" system could be useful. But it would need to be
tied into the log level, layout, etc. I have not seen much inspiration
for this kind of thing in other logging libraries, so I didn't do
anything about it yet.

On 10.11.21 16:28, Robert Haas wrote:

What I think we ought
to be driving towards is having pg_log_fatal() forcibly exit, and
pg_log_error() do the same unless the error is somehow caught.

This is specifically designed not to do any flow control. In the
backend, we have many instances, where log messages are issued with the
wrong log level because the stronger log level would have flow control
impact that is not appropriate at the call site. I don't think we want
more of that, especially since the flow control requirements in the
varied suite of frontend programs is quite diverse. Moreover, we also
require control over the exit codes in some cases, which this kind of
API wouldn't allow.

Several programs wrap, say, pg_log_fatal() into a pg_fatal(), that does
logging, cleanup, and exit, as the case may be. I think that's a good
solution. If someone wanted to write a more widely reusable pg_fatal(),
why not, but in my previous attempts, this was quite complicated and
didn't turn out to be useful.

On Mon, Nov 15, 2021 at 2:15 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

This is specifically designed not to do any flow control. In the
backend, we have many instances, where log messages are issued with the
wrong log level because the stronger log level would have flow control
impact that is not appropriate at the call site. I don't think we want
more of that, especially since the flow control requirements in the
varied suite of frontend programs is quite diverse. Moreover, we also
require control over the exit codes in some cases, which this kind of
API wouldn't allow.

I agree that the system we use in the backend isn't problem free, but
making the frontend do something randomly different isn't an
improvement. I think that most people who are writing PostgreSQL
frontend code have also written a lot of backend code, and they are
used to the way things work in the backend. More importantly, there's
an increasing amount of code that wants to run in either environment.
And there have been suggestions that we want to make more things, like
memory contexts, work that way. The design decision that you've made
here makes that harder, and results in stuff like this:

[rhaas pgsql]$ git grep pg_log_fatal.*VA_ARGS
src/bin/pg_rewind/pg_rewind.h:#define pg_fatal(...) do {
pg_log_fatal(__VA_ARGS__); exit(1); } while(0)
src/bin/pg_waldump/pg_waldump.c:#define fatal_error(...) do {
pg_log_fatal(__VA_ARGS__); exit(EXIT_FAILURE); } while(0)
src/include/lib/simplehash.h: do { pg_log_fatal(__VA_ARGS__);
exit(1); } while(0)

Having different frontend utilities each invent their own
slightly-different way of doing this makes it hard to reuse code, and
hard to understand code. We need to find ways to make it more uniform,
not just observe that it isn't uniform today and give up.

IOW, I think the fact that it's not designed to do any flow control is
a bad thing.

--
Robert Haas
EDB: http://www.enterprisedb.com

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

Several programs wrap, say, pg_log_fatal() into a pg_fatal(), that does
logging, cleanup, and exit, as the case may be. I think that's a good
solution.

I agree, and my draft patch formalized that by turning pg_log_fatal into
exactly that.

The question that I think is relevant here is what is the point of
labeling errors as "error:" or "fatal:" if we're not going to make any
serious attempt to make that distinction meaningful. I'm not really
buying your argument that it's fine as-is. Anybody who thinks that
there's a difference is going to be very confused by the behavior they
observe. But, if we all know there's no difference, why have the
difference?

regards, tom lane

On Mon, Nov 15, 2021 at 02:40:10PM -0500, Robert Haas wrote:

Having different frontend utilities each invent their own
slightly-different way of doing this makes it hard to reuse code, and
hard to understand code. We need to find ways to make it more uniform,
not just observe that it isn't uniform today and give up.

I agree with this sentiment, but this is a bit more complex than just
calling exit() with pg_log_fatal(), no? pg_dump likes playing a lot
with its exit_nicely(), meaning that we may want to allow frontends to
plug in callbacks.
--
Michael

On Mon, Nov 15, 2021 at 10:02 PM Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Nov 15, 2021 at 02:40:10PM -0500, Robert Haas wrote:

Having different frontend utilities each invent their own
slightly-different way of doing this makes it hard to reuse code, and
hard to understand code. We need to find ways to make it more uniform,
not just observe that it isn't uniform today and give up.

I agree with this sentiment, but this is a bit more complex than just
calling exit() with pg_log_fatal(), no? pg_dump likes playing a lot
with its exit_nicely(), meaning that we may want to allow frontends to
plug in callbacks.

Yep.

I think we need frontend facilities that look like the backend
facilities, so try/catch blocks, on-exit callbacks, and whatever else
there is. Otherwise code reuse is going to continue to be annoying.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 16.11.21 16:18, Robert Haas wrote:

I think we need frontend facilities that look like the backend
facilities, so try/catch blocks, on-exit callbacks, and whatever else
there is. Otherwise code reuse is going to continue to be annoying.

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.

On Fri, Nov 19, 2021 at 5:17 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.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.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Nov 19, 2021 at 5:17 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.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

Hi,

On 2022-02-22 18:23:19 -0500, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Nov 19, 2021 at 5:17 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.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.

That'd maybe be a convincing argument in a project that didn't have
elog(FATAL).

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.

What about adding a pg_fatal() that's pg_log_fatal() + exit()? That keeps
pg_log_* stuff "log only", but adds something adjacent enough to hopefully
reduce future misunderstandings?

To further decrease the chance of such mistakes, we could rename
pg_log_fatal() to something else. Or add a a pg_log_fatal() function that's
marked pg_nodiscard(), so that each callsite explicitly has to be (void)
pg_log_fatal().

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.

Agreed.

I don't care that much about the naming here.

For a moment I was wondering whether there'd be a benefit for having the "pure
logging" stuff be in a different static library than the erroring variant. So
that e.g. libpq's check for exit() doesn't run into trouble. But then I
remembered that libpq doesn't link against common...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

What about adding a pg_fatal() that's pg_log_fatal() + exit()? That keeps
pg_log_* stuff "log only", but adds something adjacent enough to hopefully
reduce future misunderstandings?

I'd be okay with that, except that pg_upgrade already has a pg_fatal
(because it has its *own* logging system, just in case you thought
this wasn't enough of a mess yet). I'm in favor of aligning
pg_upgrade's logging with the rest, but I'd hoped to leave that for
later. Making the names collide would be bad even as a short-term
thing, I fear.

Looks like libpq_pipeline.c has its own pg_fatal, too.

I'm not against choosing some name other than pg_log_fatal, but that
particular suggestion has got conflicts. Got any other ideas?

regards, tom lane

On 2022-02-22 22:44:25 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

What about adding a pg_fatal() that's pg_log_fatal() + exit()? That keeps
pg_log_* stuff "log only", but adds something adjacent enough to hopefully
reduce future misunderstandings?

I'd be okay with that, except that pg_upgrade already has a pg_fatal
(because it has its *own* logging system, just in case you thought
this wasn't enough of a mess yet). I'm in favor of aligning
pg_upgrade's logging with the rest, but I'd hoped to leave that for
later. Making the names collide would be bad even as a short-term
thing, I fear.

I guess we could name pg_upgrade's out of the way...

I'm not against choosing some name other than pg_log_fatal, but that
particular suggestion has got conflicts. Got any other ideas?

Maybe pg_fatal_exit(), pg_exit_fatal() or pg_fatal_error()?