Re: libpq error message refactoring - Mailing list pgsql-hackers

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