Re: Frontend error logging style

On 29.03.22 12:13, Daniel Gustafsson wrote:

Most of these should probably be addressed separately from Tom's patch.

>> @@ -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?

>> @@ -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 ... ;-)

>> @@ -3089,18 +2979,14 @@ main(int argc, char *argv[])
>> else if (strcmp(optarg, "libc") == 0)
>> locale_provider = COLLPROVIDER_LIBC;
>> else
>> - {
>> - pg_log_error("unrecognized locale provider: %s", optarg);
>> - exit(1);
>> - }
>> + pg_fatal("unrecognized locale provider: %s", optarg);
>
> Should this %s be within quotes to match how we usually emit user-input?

Usually not done after colon, but could be.

>> @@ -1123,9 +1097,9 @@ verify_btree_slot_handler(PGresult *res, PGconn *conn, void *context)
>> pg_log_warning("btree index \"%s.%s.%s\": btree checking function returned unexpected number of rows: %d",
>> rel->datinfo->datname, rel->nspname, rel->relname, ntups);
>> if (opts.verbose)
>> - pg_log_info("query was: %s", rel->sql);
>> - pg_log_warning("Are %s's and amcheck's versions compatible?",
>> - progname);
>> + pg_log_warning_detail("Query was: %s", rel->sql);
>> + pg_log_warning_hint("Are %s's and amcheck's versions compatible?",
>> + progname);
>
> Should "amcheck's" be a %s parameter to make translation reusable (which it
> miht never be) and possibly avoid translation mistake?

We don't have translations set up for contrib modules. Otherwise, this
kind of thing would probably be something to look into.

>> --- 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.

>> @@ -597,31 +570,19 @@ main(int argc, char *argv[])
>
>> if (ControlFile->data_checksum_version == 0 &&
>> mode == PG_MODE_CHECK)
>> - {
>> - pg_log_error("data checksums are not enabled in cluster");
>> - exit(1);
>> - }
>> + pg_fatal("data checksums are not enabled in cluster");

> Fatal seems sort of out place here, it's not really a case of "something
> terrible happened" but rather "the preconditions weren't met". Couldn't these
> be a single pg_log_error erroring out with the reason in a pg_log_detail?

"fatal" means error plus exit, so this seems ok. There is no separate
log level for really-bad-error.

>> @@ -721,7 +721,7 @@ setFilePath(ArchiveHandle *AH, char *buf, const char *relativeFilename)
>> dname = ctx->directory;
>>
>> if (strlen(dname) + 1 + strlen(relativeFilename) + 1 > MAXPGPATH)
>
> Unrelated, but shouldn't that be >= MAXPGPATH?

Seems correct to me as is.

>> @@ -14951,18 +14942,18 @@ createViewAsClause(Archive *fout, const TableInfo *tbinfo)
>
>> - fatal("definition of view \"%s\" appears to be empty (length zero)",
>> - tbinfo->dobj.name);
>> + pg_fatal("definition of view \"%s\" appears to be empty (length zero)",
>> + tbinfo->dobj.name);
>
> I'm not sure we need to provide a definition of empty here, most readers will
> probably understand that already =)

It could mean, contains no columns, or something similar. If I had to
change it, I would remove the "empty" and keep the "length zero".

>> @@ -16602,13 +16593,10 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo)
>> res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
>>
>> if (PQntuples(res) != 1)
>> - {
>> - pg_log_error(ngettext("query to get data of sequence \"%s\" returned %d row (expected 1)",
>> - "query to get data of sequence \"%s\" returned %d rows (expected 1)",
>> - PQntuples(res)),
>> - tbinfo->dobj.name, PQntuples(res));
>> - exit_nicely(1);
>> - }
>> + pg_fatal(ngettext("query to get data of sequence \"%s\" returned %d row (expected 1)",
>> + "query to get data of sequence \"%s\" returned %d rows (expected 1)",
>> + PQntuples(res)),
>> + tbinfo->dobj.name, PQntuples(res));
>
> 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.

>> @@ -144,16 +145,10 @@ main(int argc, char *argv[])
>> if (alldb)
>> {
>> if (dbname)
>> - {
>> - pg_log_error("cannot cluster all databases and a specific one at the same time");
>> - exit(1);
>> - }
>> + pg_fatal("cannot cluster all databases and a specific one at the same time");
>>
>> if (tables.head != NULL)
>> - {
>> - pg_log_error("cannot cluster specific table(s) in all databases");
>> - exit(1);
>> - }
>> + 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.