Re: Remove trailing newlines from pg_upgrade's messages

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