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.