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

At Tue, 14 Jun 2022 14:57:40 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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

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

At Wed, 15 Jun 2022 13:05:52 +0900 (JST), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> 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

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.

Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> 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

On Wed, 15 Jun 2022 at 11:54, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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

Greg Stark <stark(at)mit(dot)edu> writes:
> On Wed, 15 Jun 2022 at 11:54, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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

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.

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> 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

Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> 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