> On 29 Mar 2022, at 16:38, Peter Eisentraut
> <peter.eisentr...@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 very rarely, 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>
> for a 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/
