Unquoted file name in an error message
Hello.
While translating error messages related to pg_dumpall (1495eff7bdb),
I noticed that one message lacks double quotes around the file name:
could not open map file: %s
Since this placeholder appears standalone and not embedded in a
sentence, I initially thought it might fall outside the usual
convention of quoting file names. However, a similar message added to
pg_restore does quote the file name, as in:
could not open global.dat file: "%s"
So I believe this omission is unintentional.
The first of the attached patches adds double quotes around the file
name for consistency.
In addition, I noticed that pg_dump and pg_restore refer to map.dat
using different wording. The second patch updates the message to
refer to "map.dat file" instead of "map file", for consistency with
how pg_restore refers to both "global.dat file" and "map.dat file"
explicitly.
I noticed that there are other messages in other files that refer to
file names without quotes as well, but I'm not addressing those here.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Quote-file-names-in-error-messages.patchtext/x-patch; charset=us-asciiDownload
From 9752e745322f7c8d2be68aa982546aaac8074f75 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Mon, 7 Apr 2025 14:33:14 +0900
Subject: [PATCH 1/2] Quote file names in error messages
Correct omission of double quotes around file names in newly-added
error messages.
---
src/bin/pg_dump/pg_dumpall.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index cdb8d84f064..c112d5d22b8 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -523,7 +523,7 @@ main(int argc, char *argv[])
OPF = fopen(global_path, PG_BINARY_W);
if (!OPF)
- pg_fatal("could not open global.dat file: %s", strerror(errno));
+ pg_fatal("could not open global.dat file: \"%s\"", strerror(errno));
}
else if (filename)
{
@@ -1663,7 +1663,7 @@ dumpDatabases(PGconn *conn, ArchiveFormat archDumpFormat)
/* Create a map file (to store dboid and dbname) */
map_file = fopen(map_file_path, PG_BINARY_W);
if (!map_file)
- pg_fatal("could not open map file: %s", strerror(errno));
+ pg_fatal("could not open map file: \"%s\"", strerror(errno));
}
for (i = 0; i < PQntuples(res); i++)
--
2.43.5
0002-Use-map.dat-file-instead-of-map-file-in-error-messag.patchtext/x-patch; charset=us-asciiDownload
From 02db0d55997580a9b4fd46a52e7da4802e76ecee Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Mon, 7 Apr 2025 15:03:46 +0900
Subject: [PATCH 2/2] Use "map.dat file" instead of "map file" in error
messages
While pg_restore refers to "map.dat file" and "global.dat file"
explicitly, pg_dump uses the more generic term "map file". Update the
wording to consistently refer to the file as "map.dat file" in error
messages.
---
src/bin/pg_dump/pg_dumpall.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index c112d5d22b8..71978d556aa 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -1660,10 +1660,10 @@ dumpDatabases(PGconn *conn, ArchiveFormat archDumpFormat)
snprintf(map_file_path, MAXPGPATH, "%s/map.dat", filename);
- /* Create a map file (to store dboid and dbname) */
+ /* Create a map.dat file (to store dboid and dbname) */
map_file = fopen(map_file_path, PG_BINARY_W);
if (!map_file)
- pg_fatal("could not open map file: \"%s\"", strerror(errno));
+ pg_fatal("could not open map.dat file: \"%s\"", strerror(errno));
}
for (i = 0; i < PQntuples(res); i++)
@@ -1697,7 +1697,7 @@ dumpDatabases(PGconn *conn, ArchiveFormat archDumpFormat)
else
snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\"", db_subdir, oid);
- /* Put one line entry for dboid and dbname in map file. */
+ /* Put one line entry for dboid and dbname in map.dat file. */
fprintf(map_file, "%s %s\n", oid, dbname);
}
@@ -1748,7 +1748,7 @@ dumpDatabases(PGconn *conn, ArchiveFormat archDumpFormat)
}
}
- /* Close map file */
+ /* Close map.dat file */
if (archDumpFormat != archNull)
fclose(map_file);
--
2.43.5
On 2025-Apr-07, Kyotaro Horiguchi wrote:
Hello.
While translating error messages related to pg_dumpall (1495eff7bdb),
I noticed that one message lacks double quotes around the file name:could not open map file: %s
Since this placeholder appears standalone and not embedded in a
sentence, I initially thought it might fall outside the usual
convention of quoting file names.
Hello, I think the problem here is that the %s is not the file name, but
the string from strerror. So the lack of quotes there would seem to be
correct. The real problem is that the file name isn't mentioned in the
error message. A secondary issue might be that instead of using %s for
strerror(), maybe they should be using %m.
+ pg_fatal("could not open global.dat file: \"%s\"", strerror(errno));
Maybe we should do something like
pg_fatal("could not open "%s" file: %m", map_file_path);
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On 2025-04-07 Mo 6:13 AM, Álvaro Herrera wrote:
On 2025-Apr-07, Kyotaro Horiguchi wrote:
Hello.
While translating error messages related to pg_dumpall (1495eff7bdb),
I noticed that one message lacks double quotes around the file name:could not open map file: %s
Since this placeholder appears standalone and not embedded in a
sentence, I initially thought it might fall outside the usual
convention of quoting file names.Hello, I think the problem here is that the %s is not the file name, but
the string from strerror. So the lack of quotes there would seem to be
correct. The real problem is that the file name isn't mentioned in the
error message. A secondary issue might be that instead of using %s for
strerror(), maybe they should be using %m.+ pg_fatal("could not open global.dat file: \"%s\"", strerror(errno));
Maybe we should do something like
pg_fatal("could not open "%s" file: %m", map_file_path);
Yeah. Here's a more thorough error message cleanup.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachments:
errmsg-cleanup.patchtext/x-patch; charset=UTF-8; name=errmsg-cleanup.patchDownload
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index cdb8d84f064..18b544b0214 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -523,7 +523,7 @@ main(int argc, char *argv[])
OPF = fopen(global_path, PG_BINARY_W);
if (!OPF)
- pg_fatal("could not open global.dat file: %s", strerror(errno));
+ pg_fatal("could not open \"%s\": %m", global_path);
}
else if (filename)
{
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index b3dbde0d044..417317eb193 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -1035,7 +1035,7 @@ get_dbname_oid_list_from_mfile(const char *dumpdirpath, SimpleOidStringList *dbn
*/
if (!file_exists_in_directory(dumpdirpath, "map.dat"))
{
- pg_log_info("databases restoring is skipped as map.dat file is not present in \"%s\"", dumpdirpath);
+ pg_log_info("database restoring is skipped as \"map.dat\" is not present in \"%s\"", dumpdirpath);
return 0;
}
@@ -1045,7 +1045,7 @@ get_dbname_oid_list_from_mfile(const char *dumpdirpath, SimpleOidStringList *dbn
pfile = fopen(map_file_path, PG_BINARY_R);
if (pfile == NULL)
- pg_fatal("could not open map.dat file: \"%s\"", map_file_path);
+ pg_fatal("could not open \"%s\": %m", map_file_path);
/* Append all the dbname/db_oid combinations to the list. */
while ((fgets(line, MAXPGPATH, pfile)) != NULL)
@@ -1064,11 +1064,13 @@ get_dbname_oid_list_from_mfile(const char *dumpdirpath, SimpleOidStringList *dbn
/* Remove \n from dbname. */
dbname[strlen(dbname) - 1] = '\0';
- pg_log_info("found database \"%s\" (OID: %u) in map.dat file while restoring.", dbname, db_oid);
+ pg_log_info("found database \"%s\" (OID: %u) in \"%s\"",
+ dbname, db_oid, map_file_path);
/* Report error and exit if the file has any corrupted data. */
if (!OidIsValid(db_oid) || strlen(dbname) == 0)
- pg_fatal("invalid entry in map.dat file at line : %d", count + 1);
+ pg_fatal("invalid entry in \"%s\" at line : %d", map_file_path,
+ count + 1);
simple_oid_string_list_append(dbname_oid_list, db_oid, dbname);
count++;
@@ -1116,7 +1118,7 @@ restore_all_databases(PGconn *conn, const char *dumpdirpath,
if (dbname_oid_list.head == NULL)
return process_global_sql_commands(conn, dumpdirpath, opts->filename);
- pg_log_info("found total %d database names in map.dat file", num_total_db);
+ pg_log_info("found %d database names in \"map.dat\"", num_total_db);
if (!conn)
{
@@ -1288,7 +1290,7 @@ process_global_sql_commands(PGconn *conn, const char *dumpdirpath, const char *o
pfile = fopen(global_file_path, PG_BINARY_R);
if (pfile == NULL)
- pg_fatal("could not open global.dat file: \"%s\"", global_file_path);
+ pg_fatal("could not open \"%s\": %m", global_file_path);
/*
* If outfile is given, then just copy all global.dat file data into
@@ -1335,7 +1337,7 @@ process_global_sql_commands(PGconn *conn, const char *dumpdirpath, const char *o
/* Print a summary of ignored errors during global.dat. */
if (n_errors)
- pg_log_warning("errors ignored on global.dat file restore: %d", n_errors);
+ pg_log_warning("ignored %d errors in \"%s\"", n_errors, global_file_path);
fclose(pfile);