Thread: Error logging messages

When reviewing the patch in "Frontend error logging style" [0] I noticed that
some messages could do with a little bit of touching up.  The original review
was posted and responded to in that thread, but to keep goalposts in place it
was put off until that patch had landed.  To avoid this getting buried in that
thread I decided to start a new one with the findings from there.  To make
reviewing easier I split the patch around the sorts of changes proposed.

0001: Makes sure that database and file names are printed quoted.  This patch
has hunks in contrib and backend as well.

0002: Capitalizes pg_log_error_detail and conversely starts pg_log_error with a
lowercase letter without punctuation.

0003: Extend a few messages with more information to be more consistent with
other messages (and more helpful).

0004: Add pg_log_error() calls on all calls to close in pg_basebackup.  Nearly
all had already, and while errors here are likely to be rare, when they do
happen something is really wrong and every bit of information can help
debugging.

0005: Put keywords as parameters in a few pg_dump error messages, to make their
translations reuseable.

--
Daniel Gustafsson        https://vmware.com/

[0] https://postgr.es/m/1363732.1636496441@sss.pgh.pa.us

On Wed, Apr 13, 2022 at 01:51:16PM +0200, Daniel Gustafsson wrote:
> 0001: Makes sure that database and file names are printed quoted.  This patch
> has hunks in contrib and backend as well.
>
> 0002: Capitalizes pg_log_error_detail and conversely starts pg_log_error with a
> lowercase letter without punctuation.
>
> 0003: Extend a few messages with more information to be more consistent with
> other messages (and more helpful).
>
> 0005: Put keywords as parameters in a few pg_dump error messages, to make their
> translations reuseable.

These look fine.

> 0004: Add pg_log_error() calls on all calls to close in pg_basebackup.  Nearly
> all had already, and while errors here are likely to be rare, when they do
> happen something is really wrong and every bit of information can help
> debugging.

+               if (stream->walmethod->close(f, CLOSE_UNLINK) != 0)
+                    pg_log_error("could not delete write-ahead log file \"%s\": %s",
+                    fn, stream->walmethod->getlasterror());
With only the file names provided, it is possible to know that this is
a WAL file.  Could we switch to a simpler "could not delete file
\"%s\"" instead?  Same comment for the history file and the fsync
failure a couple of lines above.
--
Michael

> On 14 Apr 2022, at 09:10, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Apr 13, 2022 at 01:51:16PM +0200, Daniel Gustafsson wrote:
>> 0001: Makes sure that database and file names are printed quoted.  This patch
>> has hunks in contrib and backend as well.
>>
>> 0002: Capitalizes pg_log_error_detail and conversely starts pg_log_error with a
>> lowercase letter without punctuation.
>>
>> 0003: Extend a few messages with more information to be more consistent with
>> other messages (and more helpful).
>>
>> 0005: Put keywords as parameters in a few pg_dump error messages, to make their
>> translations reuseable.
>
> These look fine.

Thanks

>> 0004: Add pg_log_error() calls on all calls to close in pg_basebackup.  Nearly
>> all had already, and while errors here are likely to be rare, when they do
>> happen something is really wrong and every bit of information can help
>> debugging.
>
> +               if (stream->walmethod->close(f, CLOSE_UNLINK) != 0)
> +                    pg_log_error("could not delete write-ahead log file \"%s\": %s",
> +                    fn, stream->walmethod->getlasterror());
> With only the file names provided, it is possible to know that this is
> a WAL file.  Could we switch to a simpler "could not delete file
> \"%s\"" instead?  Same comment for the history file and the fsync
> failure a couple of lines above.

I don't have strong opinions, simplifying makes it easier on translators (due
to reuse) and keeping the verbose message may make it easier for users
experiencing problems.

--
Daniel Gustafsson        https://vmware.com/

On 13.04.22 13:51, Daniel Gustafsson wrote:
> 0002: Capitalizes pg_log_error_detail and conversely starts pg_log_error with a
> lowercase letter without punctuation.

I'm having some doubts about some of these changes, especially for 
interactive features in psql, where the messages are often use a bit of 
a different style.  I don't think this kind of thing is an improvement, 
for example:

-       pg_log_error("You are currently not connected to a database.");
+       pg_log_error("you are currently not connected to a database");

If we want to be strict about it, we could change the message to

"not currently connected to a database"

But I do not think just chopping of the punctuation and lower-casing the 
first letter of what is after all still a sentence would be an improvement.

> On 14 Apr 2022, at 16:32, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> On 13.04.22 13:51, Daniel Gustafsson wrote:
>> 0002: Capitalizes pg_log_error_detail and conversely starts pg_log_error with a
>> lowercase letter without punctuation.
>
> I'm having some doubts about some of these changes, especially for interactive features in psql, where the messages
areoften use a bit of a different style.  I don't think this kind of thing is an improvement, for example: 
>
> -       pg_log_error("You are currently not connected to a database.");
> +       pg_log_error("you are currently not connected to a database");

That may also be a bit of Stockholm Syndrome since I prefer the latter error
message, but I see your point.

--
Daniel Gustafsson        https://vmware.com/