Re: Remove trailing newlines from pg_upgrade's messages
Kyotaro Horiguchi writes:
> FWIW, the following change makes sense to me according to the spec of
> validate_exec()...
> diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c
> index fadeea12ca..3cff186213 100644
> --- a/src/bin/pg_upgrade/exec.c
> +++ b/src/bin/pg_upgrade/exec.c
> @@ -430,10 +430,10 @@ check_exec(const char *dir, const char *program, bool
> check_version)
> ret = validate_exec(path);
> if (ret == -1)
> - pg_fatal("check for \"%s\" failed: not a regular file\n",
> + pg_fatal("check for \"%s\" failed: does not exist or
> inexecutable\n",
>path);
> else if (ret == -2)
> - pg_fatal("check for \"%s\" failed: cannot execute (permission
> denied)\n",
> + pg_fatal("check for \"%s\" failed: cannot read (permission
> denied)\n",
>path);
> snprintf(cmd, sizeof(cmd), "\"%s\" -V", path);
I initially did this, but then I wondered why validate_exec() was
making it so hard: why can't we just report the failure with %m?
It turns out to take only a couple extra lines of code to ensure
that something more-or-less appropriate is returned, so we don't
need to guess about it here. Pushed that way.
regards, tom lane
Re: Remove trailing newlines from pg_upgrade's messages
Peter Eisentraut writes: > On 14.06.22 20:57, Tom Lane wrote: >> Hence, the patch below removes trailing newlines from all of >> pg_upgrade's message strings, and teaches its logging infrastructure >> to print them where appropriate. As in logging.c, there's now an >> Assert that no format string passed to pg_log() et al ends with >> a newline. > This patch looks okay to me. I compared the output before and after in > a few scenarios and didn't see any problematic differences. Thanks, pushed after rebasing and adjusting some recently-added messages. > In this particular patch, the few empty lines that disappeared don't > bother me. In general, however, I think we can just fprintf(stderr, > "\n") directly as necessary. Hmm, if anyone wants to do that I think it'd be advisable to invent "pg_log_blank_line()" or something like that, so as to preserve the logging abstraction layer. But it's moot unless anyone's interested enough to send a patch for that. I'm not. (I think it *would* be a good idea to try to get rid of the leading newlines that appear in some of the messages, as discussed upthread. But I'm not going to trouble over that right now either.) regards, tom lane
Re: Remove trailing newlines from pg_upgrade's messages
On 14.06.22 20:57, Tom Lane wrote: Hence, the patch below removes trailing newlines from all of pg_upgrade's message strings, and teaches its logging infrastructure to print them where appropriate. As in logging.c, there's now an Assert that no format string passed to pg_log() et al ends with a newline. This patch looks okay to me. I compared the output before and after in a few scenarios and didn't see any problematic differences. This doesn't quite exactly match the code's prior behavior. Aside from the buggy-looking newlines mentioned above, there are a few messages that formerly ended with a double newline, thus intentionally producing a blank line, and now they don't. I could have removed just one of their newlines, but I'd have had to give up the Assert about it, and I did not think that the extra blank lines were important enough to justify that. In this particular patch, the few empty lines that disappeared don't bother me. In general, however, I think we can just fprintf(stderr, "\n") directly as necessary.
Re: Remove trailing newlines from pg_upgrade's messages
Greg Stark writes: > On Wed, 15 Jun 2022 at 11:54, Tom Lane wrote: >> Yeah, that is sort of the inverse problem. I think those are there >> to ensure that the text appears on a fresh line even if the current >> line has transient status on it. We could get rid of those perhaps >> if we teach pg_log_v to remember whether it ended the last output >> with a newline or not, and then put out a leading newline only if >> necessary, rather than hard-wiring one into the message texts. > Is the problem that pg_upgrade doesn't know what the utilities it's > calling are outputting to the same terminal? Hmmm ... that's a point I'd not considered, but I think it's not an issue here. The subprograms generally have their output redirected to their own log files. regards, tom lane
Re: Remove trailing newlines from pg_upgrade's messages
On Wed, 15 Jun 2022 at 11:54, Tom Lane wrote: > > Yeah, that is sort of the inverse problem. I think those are there > to ensure that the text appears on a fresh line even if the current > line has transient status on it. We could get rid of those perhaps > if we teach pg_log_v to remember whether it ended the last output > with a newline or not, and then put out a leading newline only if > necessary, rather than hard-wiring one into the message texts. Is the problem that pg_upgrade doesn't know what the utilities it's calling are outputting to the same terminal? Another thing I wonder is if during development and testing there might have been more output from utilities or even the backend going on that are not happening now. -- greg
Re: Remove trailing newlines from pg_upgrade's messages
Kyotaro Horiguchi writes: > Also leading newlines and just "\n" bug me when I edit message > catalogues with poedit. I might want a line-spacing function like > pg_log_newline(PG_REPORT) if we need line-breaks in the ends of a > message. Yeah, that is sort of the inverse problem. I think those are there to ensure that the text appears on a fresh line even if the current line has transient status on it. We could get rid of those perhaps if we teach pg_log_v to remember whether it ended the last output with a newline or not, and then put out a leading newline only if necessary, rather than hard-wiring one into the message texts. This might take a little bit of fiddling to make it work, because we'd not want the extra newline when completing an incomplete line by adding status. That would mean that report_status would have to do something special, plus we'd have to be sure that all such cases do go through report_status rather than calling pg_log directly. (I'm fairly sure that the code is sloppy about that today :-(.) It seems probably do-able, though. regards, tom lane
Re: Remove trailing newlines from pg_upgrade's messages
On 14.06.22 20:57, Tom Lane wrote: I'll stick this in the CF queue, but I wonder if there is any case for squeezing it into v15 instead of waiting for v16. Let's stick this into 16 and use it as a starting point of tidying all this up in pg_upgrade.
Re: Remove trailing newlines from pg_upgrade's messages
At Wed, 15 Jun 2022 13:05:52 +0900 (JST), Kyotaro Horiguchi
wrote in
> By the way, I noticed that pg_upgrade complains wrong way when the
> specified binary path doesn't contain "postgres" file.
>
> $ pg_upgrade -b /tmp -B /tmp -d /tmp -D /tmp
>
> check for "/tmp/postgres" failed: not a regular file
> Failure, exiting
>
> I think it should be a quite common mistake to specify the parent
> directory of the binary directory..
FWIW, the following change makes sense to me according to the spec of
validate_exec()...
diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c
index fadeea12ca..3cff186213 100644
--- a/src/bin/pg_upgrade/exec.c
+++ b/src/bin/pg_upgrade/exec.c
@@ -430,10 +430,10 @@ check_exec(const char *dir, const char *program, bool
check_version)
ret = validate_exec(path);
if (ret == -1)
- pg_fatal("check for \"%s\" failed: not a regular file\n",
+ pg_fatal("check for \"%s\" failed: does not exist or
inexecutable\n",
path);
else if (ret == -2)
- pg_fatal("check for \"%s\" failed: cannot execute (permission
denied)\n",
+ pg_fatal("check for \"%s\" failed: cannot read (permission
denied)\n",
path);
snprintf(cmd, sizeof(cmd), "\"%s\" -V", path);
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Remove trailing newlines from pg_upgrade's messages
By the way, I noticed that pg_upgrade complains wrong way when the specified binary path doesn't contain "postgres" file. $ pg_upgrade -b /tmp -B /tmp -d /tmp -D /tmp check for "/tmp/postgres" failed: not a regular file Failure, exiting I think it should be a quite common mistake to specify the parent directory of the binary directory.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Remove trailing newlines from pg_upgrade's messages
At Tue, 14 Jun 2022 14:57:40 -0400, Tom Lane wrote in > Per the discussion at [1], pg_upgrade currently doesn't use > common/logging.c's functions. Making it do so looks like a > bigger lift than is justified, but there is one particular > inconsistency that I think we ought to remove: pg_upgrade > expects (most) message strings to end in newlines, while logging.c > expects them not to. This is bad for a couple of reasons: > > * Translatable strings that otherwise could be shared with other > code are different. Yes. Also it is annoying that we need to care about ending new lines.. > * Developers might mistakenly add or leave off a newline because of > familiarity with how it's done elsewhere. This is especially bad for > pg_fatal() which is otherwise caller-compatible with the version > provided by logging.c. We fixed a couple of bugs of exactly that > description recently, and I found a few more as I went through > pg_upgrade for the attached patch. It doesn't help any that as it > stands, pg_upgrade requires some messages to end in newline and > others not: there are some places that are adding an extra newline, > apparently because whoever coded them was confused about which > convention applied. > > Hence, the patch below removes trailing newlines from all of > pg_upgrade's message strings, and teaches its logging infrastructure > to print them where appropriate. As in logging.c, there's now an > Assert that no format string passed to pg_log() et al ends with > a newline. I think it's the least-bad way to control ending newline. - PG_STATUS, + PG_STATUS, /* these messages do not get a newline added */ Really? + PG_REPORT_NONL, /* these too */ PG_REPORT, > This doesn't quite exactly match the code's prior behavior. Aside > from the buggy-looking newlines mentioned above, there are a few > messages that formerly ended with a double newline, thus intentionally > producing a blank line, and now they don't. I could have removed just > one of their newlines, but I'd have had to give up the Assert about > it, and I did not think that the extra blank lines were important > enough to justify that. I don't think traling double-newlines for pg_fatal is useful so I agree to you in this point. Also leading newlines and just "\n" bug me when I edit message catalogues with poedit. I might want a line-spacing function like pg_log_newline(PG_REPORT) if we need line-breaks in the ends of a message. > BTW, as I went through the code I realized just how badly pg_upgrade > needs a visit from the message style police. Its messages are not > even consistent with each other, let alone with our message style > guidelines. I have refrained (mostly) from doing any re-wording > here, but it could stand to be done. A bit apart from this, I experince a bit hard time to find an appropriate translation for "Your installation", which I finally translate them into (a literal translation of ) "This cluster" or such.. > I'll stick this in the CF queue, but I wonder if there is any case > for squeezing it into v15 instead of waiting for v16. I think we can as it doen't seem to make functional change. But I haven't checked if the patch doesn't break anything.. > regards, tom lane > > [1] https://www.postgresql.org/message-id/4036037.1655174501%40sss.pgh.pa.us > regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Remove trailing newlines from pg_upgrade's messages
Per the discussion at [1], pg_upgrade currently doesn't use
common/logging.c's functions. Making it do so looks like a
bigger lift than is justified, but there is one particular
inconsistency that I think we ought to remove: pg_upgrade
expects (most) message strings to end in newlines, while logging.c
expects them not to. This is bad for a couple of reasons:
* Translatable strings that otherwise could be shared with other
code are different.
* Developers might mistakenly add or leave off a newline because of
familiarity with how it's done elsewhere. This is especially bad for
pg_fatal() which is otherwise caller-compatible with the version
provided by logging.c. We fixed a couple of bugs of exactly that
description recently, and I found a few more as I went through
pg_upgrade for the attached patch. It doesn't help any that as it
stands, pg_upgrade requires some messages to end in newline and
others not: there are some places that are adding an extra newline,
apparently because whoever coded them was confused about which
convention applied.
Hence, the patch below removes trailing newlines from all of
pg_upgrade's message strings, and teaches its logging infrastructure
to print them where appropriate. As in logging.c, there's now an
Assert that no format string passed to pg_log() et al ends with
a newline.
This doesn't quite exactly match the code's prior behavior. Aside
from the buggy-looking newlines mentioned above, there are a few
messages that formerly ended with a double newline, thus intentionally
producing a blank line, and now they don't. I could have removed just
one of their newlines, but I'd have had to give up the Assert about
it, and I did not think that the extra blank lines were important
enough to justify that.
BTW, as I went through the code I realized just how badly pg_upgrade
needs a visit from the message style police. Its messages are not
even consistent with each other, let alone with our message style
guidelines. I have refrained (mostly) from doing any re-wording
here, but it could stand to be done.
I'll stick this in the CF queue, but I wonder if there is any case
for squeezing it into v15 instead of waiting for v16.
regards, tom lane
[1] https://www.postgresql.org/message-id/4036037.1655174501%40sss.pgh.pa.us
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ace7387eda..89a2135c7b 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -69,13 +69,13 @@ output_check_banner(bool live_check)
{
pg_log(PG_REPORT,
"Performing Consistency Checks on Old Live Server\n"
- "\n");
+ "");
}
else
{
pg_log(PG_REPORT,
"Performing Consistency Checks\n"
- "-\n");
+ "-");
}
}
@@ -207,7 +207,7 @@ report_clusters_compatible(void)
{
if (user_opts.check)
{
- pg_log(PG_REPORT, "\n*Clusters are compatible*\n");
+ pg_log(PG_REPORT, "\n*Clusters are compatible*");
/* stops new cluster */
stop_postmaster(false);
@@ -217,7 +217,7 @@ report_clusters_compatible(void)
pg_log(PG_REPORT, "\n"
"If pg_upgrade fails after this point, you must re-initdb the\n"
- "new cluster before continuing.\n");
+ "new cluster before continuing.");
}
@@ -258,19 +258,19 @@ output_completion_banner(char *deletion_script_file_name)
pg_log(PG_REPORT,
"Optimizer statistics are not transferred by pg_upgrade.\n"
"Once you start the new server, consider running:\n"
- "%s/vacuumdb %s--all --analyze-in-stages\n\n", new_cluster.bindir, user_specification.data);
+ "%s/vacuumdb %s--all --analyze-in-stages", new_cluster.bindir, user_specification.data);
if (deletion_script_file_name)
pg_log(PG_REPORT,
"Running this script will delete the old cluster's data files:\n"
- "%s\n",
+ "%s",
deletion_script_file_name);
else
pg_log(PG_REPORT,
"Could not create a script to delete the old cluster's data files\n"
"because user-defined tablespaces or the new cluster's data directory\n"
"exist in the old cluster directory. The old cluster's contents must\n"
- "be deleted manually.\n");
+ "be deleted manually.");
termPQExpBuffer(_specification);
}
@@ -291,12 +291,12 @@ check_cluster_versions(void)
*/
if (GET_MAJOR_VERSION(old_cluster.major_version) < 902)
- pg_fatal("This utility can only upgrade from PostgreSQL version %s and later.\n",
+ pg_fatal("This utility can only upgrade from PostgreSQL version %s and later.",
"9.2");
/* Only current PG version is supported as a target */
if (GET_MAJOR_VERSION(new_cluster.major_version) != GET_MAJOR_VERSION(PG_VERSION_NUM))
- pg_fatal("This utility can only upgrade to PostgreSQL version %s.\n",
+ pg_fatal("This utility can only upgrade to PostgreSQL version
