Thread: pg_croak, or something like it?
Hi,
Recent developments on the "backup manifest" thread and others have
caused me to take an interest in making more code that has
historically been backend-only accessible in frontend environments
also. I'm pretty sure I'm not alone in having often wished for more
backend-only facilities to be available in front-end code, because
many of them are very convenient, but up until now I haven't been
motivated enough to do much about it.
Probably the thorniest problem is the backend's widespread dependence
on ereport() and elog(). Now, it would not be enormously difficult to
code up something that will sigsetjmp() and siglongjmp() in front-end
code just as we do in backend code, but I think it would be largely
missing the point. Just jumping out of a function some place and back
to the top level is not very safe, because you will tend to leak
resources and leave data structures in broken states. In the backend,
we've made this safe via transaction cleanup, which arranges to
release resources, including memory, at the proper times. Getting this
to work everywhere and for all the kinds of resources that matter has
been a pretty significant undertaking; while frontend environments
tend to be simpler, I think it's likely to take a good deal of work
and thought to figure it all out. There's another problem, too: when
ereport() is used in the backend, it reports not only an error message
but a bunch of other things like an error code, an optional error
detail, and an error context. It might be good to introduce some of
these concepts on the frontend side, but it will be confusing if
frontend errors start getting reported just like backend errors, so
here again I feel we need to think carefully.
That being said, not all uses of ereport() and elog() are created
equal. Sometimes, they are just used to report warnings, which
typically don't contain much more than a primary message, and
sometimes they are used to report can't-happen conditions, which ERROR
in backend code and could probably just print a message and exit() in
frontend code. Providing a general way to do this kind of thing seems
a lot easier than solving the whole problem, and it would allow us to
avoid continuing to copy stuff like this:
#ifndef FRONTEND
#define pg_log_warning(...) elog(WARNING, __VA_ARGS__)
#else
#include "common/logging.h"
#endif
We have two copies of that already, and I don't think we should
continue to add more. So, what I'd like to propose is a pair of
macros, one of which arranges for a warning of an appropriate sort for
either a backend or frontend environment, and the other of which is
used for a can't happen condition that should either ERROR or just
exit. Taking a page from my Perl programming background, I propose to
call these pg_carp() and pg_croak(), although I'm not in love with
those names so let the bikeshedding commence. Something like:
#ifdef FRONTEND
#define pg_croak(...) do { pg_log_fatal(__VA_ARGS__); exit(1) } while (0)
#define pg_carp(...) pg_log_warning(__VA_ARGS__);
#else
#define pg_croak(...) elog(ERROR, __VA_ARGS__)
#define pg_carp(...) elog(WARNING, __VA_ARGS__)
#endif
It is of course somewhat questionable to consider a "croak" as
effectively *fatal* on the frontend side but only ERROR rather than
FATAL on the backend side, but for the kinds of things I'm looking at
it seems like the most useful definition. If the JSON parser goes
horribly wrong to due to some logic bug, it is neither useful nor
friendly to terminate the session, but a frontend tool is probably
fine for it to just exit. The user perception will be, in each case,
that the last command they attempted (either from the command-line or
from psql, as the case may be) failed but that they are free to enter
more commands without needing to start a new session (either of psql
or bash or whatever).
Thoughts?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> Probably the thorniest problem is the backend's widespread dependence
> on ereport() and elog(). Now, it would not be enormously difficult to
> code up something that will sigsetjmp() and siglongjmp() in front-end
> code just as we do in backend code, but I think it would be largely
> missing the point.
Agreed that we don't want to introduce anything like transaction
abort recovery in our frontend tools.
> That being said, not all uses of ereport() and elog() are created
> equal. Sometimes, they are just used to report warnings, which
> typically don't contain much more than a primary message, and
> sometimes they are used to report can't-happen conditions, which ERROR
> in backend code and could probably just print a message and exit() in
> frontend code. Providing a general way to do this kind of thing seems
> a lot easier than solving the whole problem,
I think the $64 question is whether handling those two cases is sufficient
for *all* elog/ereport usages in the code we'd like to move to src/common.
If it is, great; but if it isn't, we still have a problem to solve, and
it's not clear that solving that problem won't yield a better answer for
these cases too.
> Taking a page from my Perl programming background, I propose to
> call these pg_carp() and pg_croak(), although I'm not in love with
> those names so let the bikeshedding commence.
Yeah, I'm not in love with those names either, partly because I think
they carry some baggage about when to use them and what the reports
are likely to look like. But sans caffeine, I don't have a better idea
either.
> Something like:
> #ifdef FRONTEND
> #define pg_croak(...) do { pg_log_fatal(__VA_ARGS__); exit(1) } while (0)
> #define pg_carp(...) pg_log_warning(__VA_ARGS__);
> #else
> #define pg_croak(...) elog(ERROR, __VA_ARGS__)
> #define pg_carp(...) elog(WARNING, __VA_ARGS__)
> #endif
Hm, the thing that jumps out at me about those is the lack of attention
to translatability. Sure, for really "can't happen" cases we probably
just want to use bare elog with a non-translated message. But warnings
are user-facing, and there will be an enormous temptation to use the
croak mechanism for user-facing errors too, and those should be
translated. There's also a problem that there's noplace to add an
ERRCODE; that's flat out not acceptable for backend code that's
reporting anything but absolutely-cannot-happen cases.
> It is of course somewhat questionable to consider a "croak" as
> effectively *fatal* on the frontend side but only ERROR rather than
> FATAL on the backend side, but for the kinds of things I'm looking at
> it seems like the most useful definition.
Agreed there.
What I keep thinking is that we should stick with ereport() as the
reporting notation, and invent a frontend-side implementation of it
that covers the cases you mention (ie WARNING and ERROR ... and maybe
DEBUG?), ignoring any components of the ereport that aren't helpful for
the purpose. That'd eliminate the temptation to shave the quality of
the backend-side error reports, and we still end up with about the same
basic functionality on frontend side.
regards, tom lane
On Mon, Jan 27, 2020 at 10:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Something like:
> > #ifdef FRONTEND
> > #define pg_croak(...) do { pg_log_fatal(__VA_ARGS__); exit(1) } while (0)
> > #define pg_carp(...) pg_log_warning(__VA_ARGS__);
> > #else
> > #define pg_croak(...) elog(ERROR, __VA_ARGS__)
> > #define pg_carp(...) elog(WARNING, __VA_ARGS__)
> > #endif
>
> Hm, the thing that jumps out at me about those is the lack of attention
> to translatability. Sure, for really "can't happen" cases we probably
> just want to use bare elog with a non-translated message. But warnings
> are user-facing, and there will be an enormous temptation to use the
> croak mechanism for user-facing errors too, and those should be
> translated. There's also a problem that there's noplace to add an
> ERRCODE; that's flat out not acceptable for backend code that's
> reporting anything but absolutely-cannot-happen cases.
I sorta meant to mention the translatability issues, but it went out
of my head; not enough caffeine here either, I guess.
pg_log_generic_v() does fmt = _(fmt) and
FRONTEND_COMMON_GETTEXT_TRIGGERS includes pg_log_error and friends, so
I guess the idea is that everything that goes through these routines
gets transalated. But then how does one justify this code that I
quoted before?
#ifndef FRONTEND
#define pg_log_warning(...) elog(WARNING, __VA_ARGS__)
#else
#include "common/logging.h"
#endif
These messages are, I guess, user-facing on the frontend, but can't
happen on the backend? Uggh.
> What I keep thinking is that we should stick with ereport() as the
> reporting notation, and invent a frontend-side implementation of it
> that covers the cases you mention (ie WARNING and ERROR ... and maybe
> DEBUG?), ignoring any components of the ereport that aren't helpful for
> the purpose. That'd eliminate the temptation to shave the quality of
> the backend-side error reports, and we still end up with about the same
> basic functionality on frontend side.
Well, the cases that I'm concerned about use elog(), not ereport(), so
I don't think this would help me very much. They actually are
can't-happen messages. If I were to do what you propose here, I'd then
have to run around and convert a bunch of elog() calls to ereport() to
make it work. That seems like it's going in the direction of
increasing complexity rather than reducing it, and I don't much care
for the idea that we should run around changing things like:
elog(ERROR, "unexpected json parse state: %d", ctx);
to say:
ereport(ERROR, (errmsg_internal("unexpected json parse state: %d", ctx)));
That's not a step forward in my book, especially because it'll be
*necessary* for code in src/common and optional in other places. Also,
using ereport() -- or elog() -- in frontend code seems like a large
step down the slippery slope of leading people to believe that error
recovery in the frontend can, does, or should work like error recovery
in the backend, and I think if we do even the slightest thing to feed
that impression we will regret it bitterly.
That being said, I do agree that there's a danger of people thinking
that they can use my proposed pg_croak() for user-facing messages.
Now, comments would help. (I note in passing that the comments in
common/logging.h make no mention of translatability, which IMHO they
probably should.) But we could also try to make it clear via the names
themselves, like call the macros cant_happen_error() and
cant_happen_warning(). I actually thought about that option at one
point while I was fiddling around with this, but I am psychologically
incapable of coping with spelling "can't" without an apostrophe.
However, maybe some other spelling would dodge that problem.
pg_cannot_happen_error() and pg_cannot_happen_warning()? I don't know.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jan 27, 2020 at 10:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What I keep thinking is that we should stick with ereport() as the
>> reporting notation, and invent a frontend-side implementation of it
>> that covers the cases you mention (ie WARNING and ERROR ... and maybe
>> DEBUG?), ignoring any components of the ereport that aren't helpful for
>> the purpose. That'd eliminate the temptation to shave the quality of
>> the backend-side error reports, and we still end up with about the same
>> basic functionality on frontend side.
> Well, the cases that I'm concerned about use elog(), not ereport(), so
> I don't think this would help me very much.
So? elog() is just a specific degenerate case of ereport(). If we have
a way to implement ereport() on frontend side then we can surely do
elog() too.
What it sounds to me like you want to do is implement (some equivalent of)
elog() but not ereport() for this environment. I'm going to resist that
pretty strongly, because I think it will lead directly to abuse of elog()
for user-facing errors, with a consequent degradation of the user
experience when that code executes on backend side. I do not believe
that there are no user-facing error cases in the JSON parser, for
example; much less that we'll never introduce any in future.
> That's not a step forward in my book, especially because it'll be
> *necessary* for code in src/common and optional in other places. Also,
> using ereport() -- or elog() -- in frontend code seems like a large
> step down the slippery slope of leading people to believe that error
> recovery in the frontend can, does, or should work like error recovery
> in the backend, and I think if we do even the slightest thing to feed
> that impression we will regret it bitterly.
I'm not exactly buying that. For subsystems like the JSON parser,
at least, either elog or ereport with ERROR just means "I'm throwing
up my hands, what happens next does not concern me". I don't see
any problem with interpreting that as leading to exit(1) on frontend
side. I'm also not seeing why using some other, randomly different
notation for what are fundamentally going to be the same semantics
would really improve the situation.
regards, tom lane
On Mon, Jan 27, 2020 at 10:50 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > So? elog() is just a specific degenerate case of ereport(). If we have > a way to implement ereport() on frontend side then we can surely do > elog() too. I suppose that's true. > What it sounds to me like you want to do is implement (some equivalent of) > elog() but not ereport() for this environment. I'm going to resist that > pretty strongly, because I think it will lead directly to abuse of elog() > for user-facing errors, with a consequent degradation of the user > experience when that code executes on backend side. I do not believe > that there are no user-facing error cases in the JSON parser, for > example; much less that we'll never introduce any in future. You clearly haven't read the thread on this topic, or at least not very carefully. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jan 27, 2020 at 10:50 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What it sounds to me like you want to do is implement (some equivalent of)
>> elog() but not ereport() for this environment. I'm going to resist that
>> pretty strongly, because I think it will lead directly to abuse of elog()
>> for user-facing errors, with a consequent degradation of the user
>> experience when that code executes on backend side. I do not believe
>> that there are no user-facing error cases in the JSON parser, for
>> example; much less that we'll never introduce any in future.
> You clearly haven't read the thread on this topic, or at least not
> very carefully.
I have not, but I'm still going to stand by that point. It is not
credible that the code we will want to share between frontend and
backend will never contain any user-facing error reports. Designing
a reporting mechanism that assumes that is just going to lead to
degraded reporting of things that are indeed user-facing.
regards, tom lane
On Mon, Jan 27, 2020 at 11:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I have not, but I'm still going to stand by that point. It is not > credible that the code we will want to share between frontend and > backend will never contain any user-facing error reports. It's hard to refute a statement this general; it can only devolve into an argument about what we will want to do in the future. And, if I may be permitted to appeal to a highier authority on that topic, I think this quote is highly relevant: "Difficult to see. Always in motion is the future." What I was hoping to do in this thread was to focus on the problem of which I have several instances immediately at hand, which is handling can't-happen conditions, rather than blowing open the issue in its full generality. I think there is a meaningful amount of code in the backend where that is, or can be made to be, the only issue that needs to be solved, and I therefore think that it is a reasonable special case to tackle first. Of course, I can't force you to have that conversation, but I don't think refusing to have it is going to make the problem go away. Realistically, people aren't going to stop moving code to src/common; what they're going to do is put special-case hacks in each file, like we already have in a couple of places, instead of using some more general solution upon which we might try to agree. In other words, somebody who comes across a chunk of code that uses ereport() extensively might conceivably give up on moving it to src/common, but somebody who finds one elog(ERROR, "oops this is broken") is not going to for that reason give up. They are going to do something to get around it. Better for them all to do the same thing, and something that's had some general thought given to it, than for each person who runs into such a problem to hand-roll their own way of handling it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company