Increase footprint of %m and reduce strerror()
Hi all,
Since commit d6c55de1, we support %m in the in-core port for printf
and such. And it seems to me that we could do better for the frontend
code by reducing the dependency to strerror().
One advantage of doing a switch, or at least reduce the use of
strerror(), would be to ease the work of translators with more error
messages unified between the frontend and the backend. A possible
drawback is that this could be a cause of minor conflicts when
back-patching. Always easy enough to fix, still that can be
annoying.
Thoughts?
--
Michael
At Fri, 29 Nov 2019 15:51:15 +0900, Michael Paquier <michael@paquier.xyz> wrote in
Hi all,
Since commit d6c55de1, we support %m in the in-core port for printf
and such. And it seems to me that we could do better for the frontend
code by reducing the dependency to strerror().One advantage of doing a switch, or at least reduce the use of
strerror(), would be to ease the work of translators with more error
messages unified between the frontend and the backend. A possible
drawback is that this could be a cause of minor conflicts when
back-patching. Always easy enough to fix, still that can be
annoying.Thoughts?
It sounds good to me. Message unification (including printf) needs
somehow treating trailing new lines, though. About translation
burden, I'm not sure how the message unification eases translators'
work. Identical messages of different commands appear having different
neighbours in different po files.
By the way aren't we going to have ereport on frontend?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Wed, Dec 04, 2019 at 03:32:11PM +0900, Kyotaro Horiguchi wrote:
It sounds good to me. Message unification (including printf) needs
somehow treating trailing new lines, though. About translation
burden, I'm not sure how the message unification eases translators'
work. Identical messages of different commands appear having different
neighbours in different po files.
Newlines are a problem. Still there are cases where we don't use
them. See for example pg_waldump.c. It seems like it would be first
interesting to fix the code paths where we know we can reduce the
duplicates.
By the way aren't we going to have ereport on frontend?
Not sure that this will happen, there are quite a few things to
consider related to what error hints and such should be for frontends.
That's quite a different discussion..
--
Michael
At Thu, 5 Dec 2019 11:36:48 +0900, Michael Paquier <michael@paquier.xyz> wrote in
On Wed, Dec 04, 2019 at 03:32:11PM +0900, Kyotaro Horiguchi wrote:
It sounds good to me. Message unification (including printf) needs
somehow treating trailing new lines, though. About translation
burden, I'm not sure how the message unification eases translators'
work. Identical messages of different commands appear having different
neighbours in different po files.Newlines are a problem. Still there are cases where we don't use
them. See for example pg_waldump.c. It seems like it would be first
interesting to fix the code paths where we know we can reduce the
duplicates.
So, (IIUC) do we replace fprintf()s for error reporting together (but
maybe in a separate patch)?
By the way aren't we going to have ereport on frontend?
Not sure that this will happen, there are quite a few things to
consider related to what error hints and such should be for frontends.
That's quite a different discussion..
Agreed.
+1 for going that way after having above considerations.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
(Just to clarifying the last mail..)
At Thu, 05 Dec 2019 12:06:54 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
At Thu, 5 Dec 2019 11:36:48 +0900, Michael Paquier <michael@paquier.xyz> wrote in
On Wed, Dec 04, 2019 at 03:32:11PM +0900, Kyotaro Horiguchi wrote:
It sounds good to me. Message unification (including printf) needs
somehow treating trailing new lines, though. About translation
burden, I'm not sure how the message unification eases translators'
work. Identical messages of different commands appear having different
neighbours in different po files.Newlines are a problem. Still there are cases where we don't use
them. See for example pg_waldump.c. It seems like it would be first
interesting to fix the code paths where we know we can reduce the
duplicates.So, (IIUC) do we replace fprintf()s for error reporting together (but
maybe in a separate patch)?By the way aren't we going to have ereport on frontend?
Not sure that this will happen, there are quite a few things to
consider related to what error hints and such should be for frontends.
That's quite a different discussion..Agreed.
+1 for going that way after having above considerations.
(This might be took wrongly. The following would be clearer.)
Since I see the above considertaions, I put +1 for this.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Thu, Dec 05, 2019 at 12:29:29PM +0900, Kyotaro Horiguchi wrote:
At Thu, 05 Dec 2019 12:06:54 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
So, (IIUC) do we replace fprintf()s for error reporting together (but
maybe in a separate patch)?
I guess that we should do that at the end of the day. A lookup at the
in-core tools I see three areas which stand out compared to the rest:
- pg_waldump, and attached is a patch for it.
- pgbench. However for this one we also have some status messages
showing up in stderr output, and the TAP tests have dependencies with
the output generated. This part is not plugged into the generic
logging facility yet, and we have 162 places where fprintf/stderr is
used, so that's kind of messy.
- pg_standby. For this one, we may actually be closer to just remove
it from the tree :)
--
Michael
Attachments:
pgwaldump-strerror.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index a05fbe6938..be4bf49499 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -142,8 +142,7 @@ open_file_in_directory(const char *directory, const char *fname)
fd = open(fpath, O_RDONLY | PG_BINARY, 0);
if (fd < 0 && errno != ENOENT)
- fatal_error("could not open file \"%s\": %s",
- fname, strerror(errno));
+ fatal_error("could not open file \"%s\": %m", fname);
return fd;
}
@@ -207,8 +206,8 @@ search_directory(const char *directory, const char *fname)
else
{
if (errno != 0)
- fatal_error("could not read file \"%s\": %s",
- fname, strerror(errno));
+ fatal_error("could not read file \"%s\": %m",
+ fname);
else
fatal_error("could not read file \"%s\": read %d of %zu",
fname, r, (Size) XLOG_BLCKSZ);
@@ -316,7 +315,7 @@ WALDumpOpenSegment(XLogSegNo nextSegNo, WALSegmentContext *segcxt,
break;
}
- fatal_error("could not find file \"%s\": %s", fname, strerror(errno));
+ fatal_error("could not find file \"%s\": %m", fname);
return -1; /* keep compiler quiet */
}
@@ -925,8 +924,7 @@ main(int argc, char **argv)
/* validate path points to directory */
if (!verify_directory(waldir))
{
- pg_log_error("path \"%s\" could not be opened: %s",
- waldir, strerror(errno));
+ pg_log_error("could not open directory \"%s\": %m", waldir);
goto bad_argument;
}
}
@@ -946,8 +944,7 @@ main(int argc, char **argv)
waldir = directory;
if (!verify_directory(waldir))
- fatal_error("could not open directory \"%s\": %s",
- waldir, strerror(errno));
+ fatal_error("could not open directory \"%s\": %m", waldir);
}
waldir = identify_target_directory(waldir, fname);
On Fri, Dec 06, 2019 at 02:09:05PM +0900, Michael Paquier wrote:
I guess that we should do that at the end of the day. A lookup at the
in-core tools I see three areas which stand out compared to the rest:
- pg_waldump, and attached is a patch for it.
Okay, I have committed this one.
--
Michael