Re: Frontend error logging style

> On 29 Mar 2022, at 16:38, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> On 29.03.22 12:13, Daniel Gustafsson wrote:
>
> Most of these should probably be addressed separately from Tom's patch.

Yeah, I think so too.  I'll await input from Tom on how he wants to do, but I'm
happy to take these to a new thread.  The main point of the review was to find
logic errors in the logging changes, these came as a bonus from reading them
all in one place.

>>> @@ -508,24 +502,15 @@ writefile(char *path, char **lines)
>>>     if (fclose(out_file))
>>> -    {
>>> -        pg_log_error("could not write file \"%s\": %m", path);
>>> -        exit(1);
>>> -    }
>>> +        pg_fatal("could not write file \"%s\": %m", path);
>>> }
>> Should we update this message to differentiate it from the fwrite error case?
>> Something like "an error occurred during writing.."
>
> Should be "could not close ...", no?

Yes, it should, not sure what I was thinking.

>>> @@ -2057,10 +2004,7 @@ check_locale_name(int category, const char *locale, char **canonname)
>>>
>>>     save = setlocale(category, NULL);
>>>     if (!save)
>>> -    {
>>> -        pg_log_error("setlocale() failed");
>>> -        exit(1);
>>> -    }
>>> +        pg_fatal("setlocale() failed");
>> Should this gain a hint message for those users who have no idea what this
>> really means?
>
> My setlocale() man page says:
>
> ERRORS
>     No errors are defined.
>
> So uh ... ;-)

Even more reason to be confused then =) We have a mixed bag in the tree on how
we handle errors from setlocale, in this case we could reword it to say
something like "failed to retrieve locale for %s, category" which IMO would be
slightly more informative. Might be academic though since I guess it rarely, if
ever, happens.

>>> --- a/src/bin/pg_basebackup/receivelog.c
>>> +++ b/src/bin/pg_basebackup/receivelog.c
>>> @@ -140,7 +140,7 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
>>>             /* fsync file in case of a previous crash */
>>>             if (stream->walmethod->sync(f) != 0)
>>>             {
>>> -                pg_log_fatal("could not fsync existing write-ahead log file \"%s\": %s",
>>> +                pg_log_error("could not fsync existing write-ahead log file \"%s\": %s",
>>>                              fn, stream->walmethod->getlasterror());
>>>                 stream->walmethod->close(f, CLOSE_UNLINK);
>>>                 exit(1);
>> In the case where we already have IO related errors, couldn't the close() call
>> cause an additional error message which potentially could be helpful for
>> debugging?
>
> Yeah, I think in general we have been sloppy with reporting file-closing errors properly.  Those presumably happen
veryrarely, but when they do, things are probably very bad. 

Agreed.  I'm not sure if Tom wants to add net new loggings in this patchset,
else we could do this separately.

>> The ngettext() call seems a bit out of place here since we already know that
>> second form will be taken given the check on PQntuples(res).
>
> See
<https://www.postgresql.org/message-id/flat/CALj2ACUfJKTmK5v%3DvF%2BH2iLkqM9Yvjsp6iXaCqAks6gDpzZh6g%40mail.gmail.com>
fora similar case that explains why this is still correct and necessary. 

Doh, I knew that, sorry.

>>>     }
>>> +            pg_fatal("cannot cluster specific table(s) in all databases");
>> An ngettext() candidate perhaps? There are more like this in main() hunks further down omitted for brevity here.
>
> I would just rephrase this as
>
>    "cannot cluster specific tables in all databases"
>
> This is still correct and sensible if the user specified just one table.

That's one way yes.  Mostly I'm just a bit allergic to the "foo(s)" which my
unscientifically based gut feeling am concerned about not necessarily always
working well for translations.

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