From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: libpq error message refactoring
Date: Sun, 13 Nov 2022 12:13:15 +0100
Message-ID: <8e795834-7c09-e204-97cd-c3b194ea64fb@enterprisedb.com> (raw)
In-Reply-To: <20221109122931.zmjx264q52jjhk7y@alvherre.pgsql>
References: <20221109122931.zmjx264q52jjhk7y@alvherre.pgsql>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@enterprisedb.com, alvherre@alvh.no-ip.org
Subject: Re: libpq error message refactoring
In-Reply-To: <8e795834-7c09-e204-97cd-c3b194ea64fb@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox