Thread: libpq error message refactoring
libpq now contains a mix of error message strings that end with newlines and don't end with newlines, due to some newer code paths with new ways of passing errors around. This has now gotten me confused a few too many times both during development and translation. So I looked into whether we can unify this, similar to how we have done elsewhere (e.g., pg_upgrade). I came up with the attached patch. It's not complete, but it shows the idea and it looks like a nice simplification to me. Thoughts on this approach?
Attachment
Hi, On 2022-08-25 16:34:26 +0200, Peter Eisentraut wrote: > libpq now contains a mix of error message strings that end with newlines and > don't end with newlines, due to some newer code paths with new ways of > passing errors around. This has now gotten me confused a few too many times > both during development and translation. So I looked into whether we can > unify this, similar to how we have done elsewhere (e.g., pg_upgrade). I > came up with the attached patch. It's not complete, but it shows the idea > and it looks like a nice simplification to me. Thoughts on this approach? This patch has been failing for a while: https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3854 Interestingly, previously the error only happened when targetting windows, but meson also shows it on freebsd. It's not the cause of this failure, I think, but doesn't appendPQExpBufferVA need to be added to exports.txt? Greetings, Andres Freund
On 22.09.22 17:42, Andres Freund wrote: > This patch has been failing for a while: > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3854 > > Interestingly, previously the error only happened when targetting windows, but > meson also shows it on freebsd. > > It's not the cause of this failure, I think, but doesn't appendPQExpBufferVA > need to be added to exports.txt? I don't want to make that function available to users of libpq, just use it inside libpq across .c files. Is there no visibility level for that? Is that also the problem in the freebsd build?
HHi, On 2022-09-22 22:00:00 -0400, Peter Eisentraut wrote: > On 22.09.22 17:42, Andres Freund wrote: > > This patch has been failing for a while: > > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3854 > > > > Interestingly, previously the error only happened when targetting windows, but > > meson also shows it on freebsd. > > > > It's not the cause of this failure, I think, but doesn't appendPQExpBufferVA > > need to be added to exports.txt? > > I don't want to make that function available to users of libpq, just use it > inside libpq across .c files. Is there no visibility level for that? Is > that also the problem in the freebsd build? I suspect the appendPQExpBufferVA is orthogonal - most (all?) of the other functions in pqexpbuffer.h are visible, so it feels weird/confusing to not make appendPQExpBufferVA() available. I just noticed it when trying to understand the linker failure - which I still don't... Greetings, Andres Freund
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 22.09.22 17:42, Andres Freund wrote:
>> It's not the cause of this failure, I think, but doesn't appendPQExpBufferVA
>> need to be added to exports.txt?
> I don't want to make that function available to users of libpq, just use
> it inside libpq across .c files. Is there no visibility level for that?
Should "just work", I should think. I agree with Andres that that's
not the cause of the build failure. I wonder if somehow the failing
links are picking up the wrong libpq.a.
Separately from that: is it really okay to delegate use of a va_list
argument like that? The other call paths of
appendPQExpBufferVA[_internal] write va_start/va_end directly around it,
not somewhere else in the call chain. I'm too tired to language-lawyer
out what happens when you do it like this, but I'm suspecting that it's
not well-defined portable behavior.
I think what you probably need to do is export appendPQExpBufferVA
as-is and require libpq_append_error to provide the error loop.
regards, tom lane
Andres Freund <andres@anarazel.de> writes:
> I suspect the appendPQExpBufferVA is orthogonal - most (all?) of the other
> functions in pqexpbuffer.h are visible, so it feels weird/confusing to not
> make appendPQExpBufferVA() available.
I thought the same to start with, but if I'm right in my nearby reply
that we'll have to make callers loop around appendPQExpBufferVA,
then it seems like a good idea to keep it closely held.
More than zero commentary about that would be a good thing, too.
regards, tom lane
Hi, On 2022-09-22 19:27:27 -0700, Andres Freund wrote: > I just noticed it when trying to understand the linker failure - which I > still don't... Heh, figured it out. It's inside #ifdef ENABLE_NLS. So it fails on all platforms without NLS enabled. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> Heh, figured it out. It's inside #ifdef ENABLE_NLS. So it fails on all
> platforms without NLS enabled.
Argh, how simple!
The question about va_start/va_end placement still stands, though.
regards, tom lane
On 23.09.22 04:45, Andres Freund wrote: > On 2022-09-22 19:27:27 -0700, Andres Freund wrote: >> I just noticed it when trying to understand the linker failure - which I >> still don't... > > Heh, figured it out. It's inside #ifdef ENABLE_NLS. So it fails on all > platforms without NLS enabled. Hah! Here is an updated patch to get the CI clean. I'll look into the other discussed issues later.
Attachment
On 25.08.22 16:34, Peter Eisentraut wrote: > libpq now contains a mix of error message strings that end with newlines > and don't end with newlines, due to some newer code paths with new ways > of passing errors around. This has now gotten me confused a few too > many times both during development and translation. So I looked into > whether we can unify this, similar to how we have done elsewhere (e.g., > pg_upgrade). I came up with the attached patch. It's not complete, but > it shows the idea and it looks like a nice simplification to me. I have completed this patch, taking into account the fixes discussed in this thread. I have split the patch in two, for review: The first is just the new APIs, the second are the changes that apply the API everywhere.
Attachment
On 23.09.22 04:37, Tom Lane wrote: > Separately from that: is it really okay to delegate use of a va_list > argument like that? The other call paths of > appendPQExpBufferVA[_internal] write va_start/va_end directly around it, > not somewhere else in the call chain. I'm too tired to language-lawyer > out what happens when you do it like this, but I'm suspecting that it's > not well-defined portable behavior. > > I think what you probably need to do is export appendPQExpBufferVA > as-is and require libpq_append_error to provide the error loop. There was actually a live problem here, maybe not the exact one you had in mind: When you actually need the "need more space" loop, you must do va_end() and va_start() before calling down again. Otherwise, the next va_arg() gets garbage. It so happens that the error message "private key file \"%s\" has group or world access; file must have permissions u=rw (0600) or less if owned by the current user, or permissions u=rw,g=r (0640) or less if owned by root" together with an in-tree test location for the file in question just barely exceeds INITIAL_EXPBUFFER_SIZE (256), and so my previous patch would fail the "ssl" test suite. Good test coverage. :) Anyway, I have updated my patch with your suggestion, which should fix these kinds of issues.
Hello
I gave this series a quick look. Overall it seems a good idea, since
the issue of newlines-or-not is quite bothersome for the libpq
translations.
> +/*
> + * Append a formatted string to the given buffer, after translation. A
> + * newline is automatically appended; the format should not end with a
> + * newline.
> + */
I find the "after translation" bit unclear -- does it mean that the
caller should have already translated, or is it the other way around? I
would say "Append a formatted string to the given buffer, after
translating it", which (to me) conveys more clearly that translation
occurs here.
> + /* Loop in case we have to retry after enlarging the buffer. */
> + do
> + {
> + errno = save_errno;
> + va_start(args, fmt);
> + done = appendPQExpBufferVA(errorMessage, libpq_gettext(fmt), args);
I wonder if it makes sense to do libpq_gettext() just once, instead of
redoing it on each iteration.
> +void
> +libpq_append_conn_error(PGconn *conn, const char *fmt, ...)
These two routines are essentially identical. While we could argue
about sharing an underlying implementation, I think it's okay the way
you have it, because the overheard of sharing it would make that
pointless, given how short they are.
> +extern void libpq_append_error(PQExpBuffer errorMessage, const char *fmt, ...) pg_attribute_printf(2, 3);
> +extern void libpq_append_conn_error(PGconn *conn, const char *fmt, ...) pg_attribute_printf(2, 3);
pg_attribute_printf marker present -- check.
> -GETTEXT_TRIGGERS = libpq_gettext pqInternalNotice:2
> -GETTEXT_FLAGS = libpq_gettext:1:pass-c-format pqInternalNotice:2:c-format
> +GETTEXT_TRIGGERS = libpq_append_conn_error:2 \
> + libpq_append_error:2 \
> + libpq_gettext pqInternalNotice:2
> +GETTEXT_FLAGS = libpq_append_conn_error:2:c-format \
> + libpq_append_error:2:c-format \
> + libpq_gettext:1:pass-c-format pqInternalNotice:2:c-format
Looks good.
> --- a/src/interfaces/libpq/pqexpbuffer.h
> +++ b/src/interfaces/libpq/pqexpbuffer.h
> +/*------------------------
> + * appendPQExpBufferVA
> + * Shared guts of printfPQExpBuffer/appendPQExpBuffer.
> + * Attempt to format data and append it to str. Returns true if done
> + * (either successful or hard failure), false if need to retry.
> + *
> + * Caution: callers must be sure to preserve their entry-time errno
> + * when looping, in case the fmt contains "%m".
> + */
> +extern bool appendPQExpBufferVA(PQExpBuffer str, const char *fmt, va_list args) pg_attribute_printf(2, 0);
As an API user, I don't care that this is shared guts for something
else, I just care about what it does. I think deleting that line is a
sufficient fix.
> - appendPQExpBufferStr(&conn->errorMessage,
> - libpq_gettext("malformed SCRAM message (empty message)\n"));
> + libpq_append_conn_error(conn, "malformed SCRAM message (empty message)");
Overall, this type of change looks positive. I didn't review all these
changes too closely other than the first couple of dozens, as there are
way too many; I suppose you did these with some Emacs macros or something?
> @@ -420,7 +418,8 @@ pqsecure_raw_write(PGconn *conn, const void *ptr, size_t len)
> snprintf(msgbuf, sizeof(msgbuf),
> libpq_gettext("server closed the connection unexpectedly\n"
> "\tThis probably means the server terminated abnormally\n"
> - "\tbefore or while processing the request.\n"));
> + "\tbefore or while processing the request."));
> + strlcat(msgbuf, "\n", sizeof(msgbuf));
> conn->write_err_msg = strdup(msgbuf);
> /* Now claim the write succeeded */
> n = len;
In these two places, we're writing the error message manually to a
separate variable, so the extra \n is necessary. It looks a bit odd to
do it with strlcat() after the fact, but AFAICT it's necessary as it
keeps the \n out of the translation catalog, which is good. This is
nonobvious, so perhaps add a comment about it.
Thanks
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Before you were born your parents weren't as boring as they are now. They
got that way paying your bills, cleaning up your room and listening to you
tell them how idealistic you are." -- Charles J. Sykes' advice to teenagers
On 09.11.22 13:29, Alvaro Herrera wrote:
>> +/*
>> + * Append a formatted string to the given buffer, after translation. A
>> + * newline is automatically appended; the format should not end with a
>> + * newline.
>> + */
>
> I find the "after translation" bit unclear -- does it mean that the
> caller should have already translated, or is it the other way around? I
> would say "Append a formatted string to the given buffer, after
> translating it", which (to me) conveys more clearly that translation
> occurs here.
ok
>> + /* Loop in case we have to retry after enlarging the buffer. */
>> + do
>> + {
>> + errno = save_errno;
>> + va_start(args, fmt);
>> + done = appendPQExpBufferVA(errorMessage, libpq_gettext(fmt), args);
>
> I wonder if it makes sense to do libpq_gettext() just once, instead of
> redoing it on each iteration.
I wonder whether that would expose us to potential compiler warnings
about the format string not being constant. As long as the compiler can
trace that the string comes from gettext, it knows what's going on.
Also, most error strings in practice don't need the loop, so maybe it's
not a big issue.
>> +/*------------------------
>> + * appendPQExpBufferVA
>> + * Shared guts of printfPQExpBuffer/appendPQExpBuffer.
>> + * Attempt to format data and append it to str. Returns true if done
>> + * (either successful or hard failure), false if need to retry.
>> + *
>> + * Caution: callers must be sure to preserve their entry-time errno
>> + * when looping, in case the fmt contains "%m".
>> + */
>> +extern bool appendPQExpBufferVA(PQExpBuffer str, const char *fmt, va_list args) pg_attribute_printf(2, 0);
>
> As an API user, I don't care that this is shared guts for something
> else, I just care about what it does. I think deleting that line is a
> sufficient fix.
ok
>> @@ -420,7 +418,8 @@ pqsecure_raw_write(PGconn *conn, const void *ptr, size_t len)
>> snprintf(msgbuf, sizeof(msgbuf),
>> libpq_gettext("server closed the connection unexpectedly\n"
>> "\tThis probably means the server terminated abnormally\n"
>> - "\tbefore or while processing the request.\n"));
>> + "\tbefore or while processing the request."));
>> + strlcat(msgbuf, "\n", sizeof(msgbuf));
>> conn->write_err_msg = strdup(msgbuf);
>> /* Now claim the write succeeded */
>> n = len;
>
> In these two places, we're writing the error message manually to a
> separate variable, so the extra \n is necessary. It looks a bit odd to
> do it with strlcat() after the fact, but AFAICT it's necessary as it
> keeps the \n out of the translation catalog, which is good. This is
> nonobvious, so perhaps add a comment about it.
ok
On 2022-Nov-13, Peter Eisentraut wrote:
> On 09.11.22 13:29, Alvaro Herrera wrote:
> > > + /* Loop in case we have to retry after enlarging the buffer. */
> > > + do
> > > + {
> > > + errno = save_errno;
> > > + va_start(args, fmt);
> > > + done = appendPQExpBufferVA(errorMessage, libpq_gettext(fmt), args);
> >
> > I wonder if it makes sense to do libpq_gettext() just once, instead of
> > redoing it on each iteration.
>
> I wonder whether that would expose us to potential compiler warnings about
> the format string not being constant. As long as the compiler can trace
> that the string comes from gettext, it knows what's going on.
>
> Also, most error strings in practice don't need the loop, so maybe it's not
> a big issue.
True.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/