Feedback-ID: i131946ab:Fastmail
Message-ID: <8e795834-7c09-e204-97cd-c3b194ea64fb@enterprisedb.com>
Date: Sun, 13 Nov 2022 12:13:15 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.4.1
Subject: Re: libpq error message refactoring
Content-Language: en-US
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <20221109122931.zmjx264q52jjhk7y@alvherre.pgsql>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <20221109122931.zmjx264q52jjhk7y@alvherre.pgsql>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/8e795834-7c09-e204-97cd-c3b194ea64fb%40enterprisedb.com>
Precedence: bulk

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