Обсуждение: Badly designed error reporting code in controldata_utils.c
My attention was drawn to the log_error() stuff in controldata_utils.c by
the fact that buildfarm member pademelon spit up on it. The reason for
that compile failure is that pademelon's dinosaur of a compiler doesn't
support __VA_ARGS__. I do not feel a need to get into a discussion about
whether we should move our portability goalposts for the convenience of
this commit, because there are other reasons why this is a crummy solution
for error reporting:
* It uses elog() not ereport() for what seems a not-particularly-internal
error, which among other things means that an entirely inappropriate
errcode() will be reported.
* It relies on strerror(errno), not %m, which may not work reliably even
in elog() and certainly won't in ereport() (because of order-of-evaluation
uncertainties).
* Translatability of the error message in the frontend context seems
a bit dubious; generally we let translators work with the whole string
to be printed, not just part of it.
* It's randomly unlike every single other place we've addressed the
same problem. Everywhere else in src/common does it like this:
#ifndef FRONTEND ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("outof memory")));
#else fprintf(stderr, _("out of memory\n")); exit(EXIT_FAILURE);
#endif
and I think that's what this needs to do too, especially in view of the
fact that there are only two places that would have to be fixed anyway.
regards, tom lane
On 03/06/2016 01:24 PM, Tom Lane wrote:
> * It's randomly unlike every single other place we've addressed the
> same problem. Everywhere else in src/common does it like this:
>
> #ifndef FRONTEND
> ereport(ERROR,
> (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> errmsg("out of memory")));
> #else
> fprintf(stderr, _("out of memory\n"));
> exit(EXIT_FAILURE);
> #endif
>
> and I think that's what this needs to do too, especially in view of the
> fact that there are only two places that would have to be fixed anyway.
Ok, will fix.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
On 03/06/2016 01:26 PM, Joe Conway wrote: > On 03/06/2016 01:24 PM, Tom Lane wrote: >> * It's randomly unlike every single other place we've addressed the >> same problem. Everywhere else in src/common does it like this: [...snip...] >> and I think that's what this needs to do too, especially in view of the >> fact that there are only two places that would have to be fixed anyway. > > Ok, will fix. Something like the attached what you had in mind? Thanks, Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
On 2016-03-06 17:16:42 -0800, Joe Conway wrote:
> - #ifndef FRONTEND
> - /* NOTE: caller must provide gettext call around the format string */
> - #define log_error(...) \
> - elog(ERROR, __VA_ARGS__)
> - #else
> - #define log_error(...) \
> - do { \
> - char *buf = psprintf(__VA_ARGS__); \
> - fprintf(stderr, "%s: %s\n", progname, buf); \
> - exit(2); \
> - } while (0)
> - #endif
> -
FWIW I'm considering implementing elog/ereport properly for the
frontend. We've grown several hacks around that not being present, and
I think those by now have a higher aggregate complexity than a proper
implementation would have.
Greetings,
Andres Freund
Joe Conway <mail@joeconway.com> writes:
> Something like the attached what you had in mind?
That's much better, but is there a reason you're using exit(2)
and not exit(EXIT_FAILURE) ?
regards, tom lane
On 03/06/2016 05:47 PM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >> Something like the attached what you had in mind? > > That's much better, but is there a reason you're using exit(2) > and not exit(EXIT_FAILURE) ? Only because I was trying to stick with what was originally in src/bin/pg_controldata/pg_controldata.c Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Joe Conway <mail@joeconway.com> writes:
> On 03/06/2016 05:47 PM, Tom Lane wrote:
>> That's much better, but is there a reason you're using exit(2)
>> and not exit(EXIT_FAILURE) ?
> Only because I was trying to stick with what was originally in
> src/bin/pg_controldata/pg_controldata.c
Meh. It looks to me like the standard way to handle this
for code in src/common/ is exit(EXIT_FAILURE).
regards, tom lane
On 03/06/2016 07:34 PM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >> On 03/06/2016 05:47 PM, Tom Lane wrote: >>> That's much better, but is there a reason you're using exit(2) >>> and not exit(EXIT_FAILURE) ? > >> Only because I was trying to stick with what was originally in >> src/bin/pg_controldata/pg_controldata.c > > Meh. It looks to me like the standard way to handle this > for code in src/common/ is exit(EXIT_FAILURE). I have no issue with using EXIT_FAILURE, but note: 1) That will change the exit error from the previous value of 2 to 1 for pg_controldata 2) There are many examples in other parts of the source that do not use EXIT_FAILURE, and even in src/common: 8<------------- grep -rnE "exit\(EXIT_FAILURE\)" src/common/* --include=*.c src/common/fe_memutils.c:36: exit(EXIT_FAILURE); src/common/fe_memutils.c:76: exit(EXIT_FAILURE); src/common/fe_memutils.c:93: exit(EXIT_FAILURE); src/common/fe_memutils.c:99: exit(EXIT_FAILURE); src/common/psprintf.c:135: exit(EXIT_FAILURE); src/common/psprintf.c:182: exit(EXIT_FAILURE); grep -rnE "exit\((1|2)\)" src/common/* --include=*.c src/common/restricted_token.c:187: exit(1); src/common/username.c:86: exit(1); 8<------------- Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On 03/07/2016 08:02 AM, Joe Conway wrote: > On 03/06/2016 07:34 PM, Tom Lane wrote: >> Joe Conway <mail@joeconway.com> writes: >>> On 03/06/2016 05:47 PM, Tom Lane wrote: >>>> That's much better, but is there a reason you're using exit(2) >>>> and not exit(EXIT_FAILURE) ? >> >>> Only because I was trying to stick with what was originally in >>> src/bin/pg_controldata/pg_controldata.c >> >> Meh. It looks to me like the standard way to handle this >> for code in src/common/ is exit(EXIT_FAILURE). > > I have no issue with using EXIT_FAILURE, but note: Committed/pushed with exit(EXIT_FAILURE) Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Joe Conway <mail@joeconway.com> writes:
> Committed/pushed with exit(EXIT_FAILURE)
Thanks! I lit off a new run on gaur/pademelon to confirm. Should
have results in six hours or so...
regards, tom lane
On Mon, Mar 7, 2016 at 10:26 AM, Andres Freund <andres@anarazel.de> wrote: > FWIW I'm considering implementing elog/ereport properly for the > frontend. We've grown several hacks around that not being present, and > I think those by now have a higher aggregate complexity than a proper > implementation would have. That would be handy. And this is are going to need something like callbacks to allow frontend applications how to apply elevel. Take for example pg_rewind, it has an interface with DEBUG and PROGRESS level directly embedded with FATAL controlled by user-defined options. -- Michael
On 2016-03-08 13:45:25 +0900, Michael Paquier wrote: > On Mon, Mar 7, 2016 at 10:26 AM, Andres Freund <andres@anarazel.de> wrote: > > FWIW I'm considering implementing elog/ereport properly for the > > frontend. We've grown several hacks around that not being present, and > > I think those by now have a higher aggregate complexity than a proper > > implementation would have. > > That would be handy. And this is are going to need something like > callbacks to allow frontend applications how to apply elevel. Take for > example pg_rewind, it has an interface with DEBUG and PROGRESS level > directly embedded with FATAL controlled by user-defined options. What does "directly embedded with FATAL" mean? I don't really want to go further than what our system already is capable of; once we have that we can look for the next steps.
On Tue, Mar 8, 2016 at 1:51 PM, Andres Freund <andres@anarazel.de> wrote: > On 2016-03-08 13:45:25 +0900, Michael Paquier wrote: >> On Mon, Mar 7, 2016 at 10:26 AM, Andres Freund <andres@anarazel.de> wrote: >> > FWIW I'm considering implementing elog/ereport properly for the >> > frontend. We've grown several hacks around that not being present, and >> > I think those by now have a higher aggregate complexity than a proper >> > implementation would have. >> >> That would be handy. And this is are going to need something like >> callbacks to allow frontend applications how to apply elevel. Take for >> example pg_rewind, it has an interface with DEBUG and PROGRESS level >> directly embedded with FATAL controlled by user-defined options. > > What does "directly embedded with FATAL" mean? Incorrect words. I just mean that there is a central code path used by DEBUG and FATAL in this case, and DEBUG is controlled by a user-side switch, meaning that if we want to get into something aimed at being used by any in-core or community frontend clients, we are going to need something carefully designed. -- Michael