Quoting filename in using facing log messages
Looking at ZQFUGOuS5DU4DsE4@paquier.xyz I noticed that we had a a few instances
of filenames in userfacing log messages (ie not elog or DEBUGx etc) not being
quoted, where the vast majority are quoted like \"%s\". Any reason not to
quote them as per the attached to be consistent across all log messages?
--
Daniel Gustafsson
Attachments:
quote_file_errmsg.diffapplication/octet-stream; name=quote_file_errmsg.diff; x-unix-mode=0644Download
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f6f8adc72a..50cd165289 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2248,7 +2248,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
errno = save_errno;
ereport(PANIC,
(errcode_for_file_access(),
- errmsg("could not write to log file %s "
+ errmsg("could not write to log file \"%s\" "
"at offset %u, length %zu: %m",
xlogfname, startoffset, nleft)));
}
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index b9acfed3b7..a3535bdfa9 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -418,11 +418,11 @@ WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
if (errinfo.wre_errno != 0)
{
errno = errinfo.wre_errno;
- pg_fatal("could not read from file %s, offset %d: %m",
+ pg_fatal("could not read from file \"%s\", offset %d: %m",
fname, errinfo.wre_off);
}
else
- pg_fatal("could not read from file %s, offset %d: read %d of %d",
+ pg_fatal("could not read from file \"%s\", offset %d: read %d of %d",
fname, errinfo.wre_off, errinfo.wre_read,
errinfo.wre_req);
}
On 13.09.23 13:48, Daniel Gustafsson wrote:
Looking at ZQFUGOuS5DU4DsE4@paquier.xyz I noticed that we had a a few instances
of filenames in userfacing log messages (ie not elog or DEBUGx etc) not being
quoted, where the vast majority are quoted like \"%s\". Any reason not to
quote them as per the attached to be consistent across all log messages?
Since WAL file names have a predictable format, there is less pressure
to quote them to avoid ambiguities. But in general we should try to be
consistent, so your patch makes sense to me.
On 13 Sep 2023, at 13:55, Peter Eisentraut <peter@eisentraut.org> wrote:
On 13.09.23 13:48, Daniel Gustafsson wrote:
Looking at ZQFUGOuS5DU4DsE4@paquier.xyz I noticed that we had a a few instances
of filenames in userfacing log messages (ie not elog or DEBUGx etc) not being
quoted, where the vast majority are quoted like \"%s\". Any reason not to
quote them as per the attached to be consistent across all log messages?Since WAL file names have a predictable format, there is less pressure to quote them to avoid ambiguities. But in general we should try to be consistent
Correct, this is all for consistency.
so your patch makes sense to me.
Thanks!
It might be worth concatenating the errmsg() while there since we typically
don't linebreak errmsg strings anymore for greppability:
- errmsg("could not write to log file %s "
- "at offset %u, length %zu: %m",
+ errmsg("could not write to log file \"%s\" at offset %u, length %zu: %m",I don't have strong feelings wrt that, just have a vague memory of "concatenate
when touching" as an informal guideline.
--
Daniel Gustafsson
On Wed, Sep 13, 2023 at 02:02:47PM +0200, Daniel Gustafsson wrote:
It might be worth concatenating the errmsg() while there since we typically
don't linebreak errmsg strings anymore for greppability:- errmsg("could not write to log file %s " - "at offset %u, length %zu: %m", + errmsg("could not write to log file \"%s\" at offset %u, length %zu: %m",I don't have strong feelings wrt that, just have a vague memory of "concatenate
when touching" as an informal guideline.
Because these are slightly easier to grep when looking for a given
pattern in the tree.
(I'm OK with your patch as well, FWIW.)
--
Michael