public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Eisentraut <[email protected]>
To: Daniel Gustafsson <[email protected]>
To: Tom Lane <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Frontend error logging style
Date: Tue, 29 Mar 2022 16:38:35 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <YZMfMI3kTDw/[email protected]>
<CA+TgmoZy84orCfUNipEp+QzWwRuR2yrjsdcJN9ROHzPkXDRbxQ@mail.gmail.com>
<[email protected]>
<CA+TgmoYaA1wtU91yNg1_C6oS8azAfp5dQ1YrfuNAm2=Yefrn1A@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
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%4...;
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.
view thread (41+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Frontend error logging style
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox