Re: pg_croak, or something like it? - Mailing list pgsql-hackers

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