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.
