pg_upgrade: prep_status doesn't translate messages
Hello.
In pg_upgrade, prep statuts is shown in English even if LANG is
set to other languages.
$ LANG=ja_JP.UTF8 pg_upgrade ...
<"Performing Consistency Checks on Old Live Server" in Japanese>
--------------------------------------------------
Checking cluster versions ok
Checking database user is the install user ok
Checking database connection settings ok
Checking for prepared transactions ok
...
<"*Clusters are compatible*" in Japanese>
prep_status is marked as GETTEXT_TRIGGERS but actually doesn't
translate. I suppose the reason is we don't have a general and
portable means to align the message strings containing non-ascii
characters.
I'd like to propose to append " ... " instead of aligning messages.
Checking cluster versions ... ok
Checking database user is the install user ... ok
Checking database connection settings ... ok
Checking for prepared transactions ... ok
If we don't do that, translation lines in po files are
useless. prep_stauts must be removed from TETTEXT_TRIGGERS, and a
comment that explains the reason for not translating.
Any opinions?
regardes.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Change-format-of-prep-status-of-pg_upgrade.patchapplication/octet-stream; name=0001-Change-format-of-prep-status-of-pg_upgrade.patchDownload
From 8de770b7bee7fa446a8176416d8887ab54ce096a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Mon, 10 Jun 2019 13:38:48 +0900
Subject: [PATCH] Change format of prep status of pg_upgrade
The current format in prep_status() hinders messages from being
translated since we don't have a general and reliable means to acquire
actual display width of a non-ascii string. To make translation
available, just do append " ... " to it instad of trying to align
strings to fixed position.
---
src/bin/pg_upgrade/util.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/bin/pg_upgrade/util.c b/src/bin/pg_upgrade/util.c
index d867995cc3..2807cacb87 100644
--- a/src/bin/pg_upgrade/util.c
+++ b/src/bin/pg_upgrade/util.c
@@ -73,14 +73,10 @@ prep_status(const char *fmt,...)
char message[MAX_STRING];
va_start(args, fmt);
- vsnprintf(message, sizeof(message), fmt, args);
+ vsnprintf(message, sizeof(message), _(fmt), args);
va_end(args);
- if (strlen(message) > 0 && message[strlen(message) - 1] == '\n')
- pg_log(PG_REPORT, "%s", message);
- else
- /* trim strings that don't end in a newline */
- pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, message);
+ pg_log(PG_REPORT, "%s ... ", message);
}
--
2.16.3
On Mon, Jun 10, 2019 at 01:57:14PM +0900, Kyotaro Horiguchi wrote:
If we don't do that, translation lines in po files are
useless. prep_stauts must be removed from TETTEXT_TRIGGERS, and a
comment that explains the reason for not translating.Any opinions?
I agree with your point that it should be an all-or-nothing, and not
something in the middle. Now, I would fall into the category of
people which would prefer making the full set of contents translated,
and there has been some work in this area recently:
/messages/by-id/20170523002827.lzc2jkzh2gubclqb@alvherre.pgsql
--
Michael
Hello.
At Mon, 10 Jun 2019 16:48:42 +0900, Michael Paquier
<michael@paquier.xyz> wrote in <20190610074842.GH2199@paquier.xyz>
On Mon, Jun 10, 2019 at 01:57:14PM +0900, Kyotaro Horiguchi wrote:
If we don't do that, translation lines in po files are
useless. prep_stauts must be removed from TETTEXT_TRIGGERS, and a
comment that explains the reason for not translating.Any opinions?
I agree with your point that it should be an all-or-nothing, and not
something in the middle. Now, I would fall into the category of
people which would prefer making the full set of contents translated,
I'm on the same side. Whether do you think this as a 12-issue or
for later versions? I think there is no risk in changing it now so
I wish the change is contained in 12-shipping.
and there has been some work in this area recently:
/messages/by-id/20170523002827.lzc2jkzh2gubclqb@alvherre.pgsql
Thanks for the pointer. I'm seeing the result of the discussion
now. Apart from the discussion of translate-or-not decision,
there can be a discussion how we can reduce the burden of
translation work. I was a bit tired to translate something like
the followings:
old and new pg_controldata block sizes are invalid or do not match
old and new pg_controldata maximum relation segment sizes are invalid
or do not match
old and new pg_controldata WAL block sizes are invalid or do not match
...
I'm not sure where is the compromisable point between burden of
translators and programmers, though.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On 2019-Jun-11, Kyotaro Horiguchi wrote:
Thanks for the pointer. I'm seeing the result of the discussion
now. Apart from the discussion of translate-or-not decision,
there can be a discussion how we can reduce the burden of
translation work. I was a bit tired to translate something like
the followings:old and new pg_controldata block sizes are invalid or do not match
old and new pg_controldata maximum relation segment sizes are invalid
or do not match
old and new pg_controldata WAL block sizes are invalid or do not match
...I'm not sure where is the compromisable point between burden of
translators and programmers, though.
I think the problem with those messages is that they are poorly
worded/styled, but I haven't tried to figure out how to make them
better. That may also fix the translation burden, not sure. If you
have proposals for improvement, let's hear them.
Here's a quick idea. We already have this:
msgid "The target cluster lacks some required control information:\n"
msgid " checkpoint next XID\n"
msgid " latest checkpoint next OID\n"
so this gives me the idea that one way to fix the problem you mention is
something like this:
msgid "The following source and target pg_controldata items do not match:"
msgid " block size"
msgid " maximum relation segment size"
etc. (One thing to note is that those strings already exist in the .po
files, so already translated). Obviously needs a bit of code rework
(and the first new one should use the plural stuff, because it's likely
it'll only be one item that does not match). Also will need separate
messages (with plurals) for
msgid "The following source pg_controldata items are invalid:"
msgid "The following target pg_controldata items are invalid:"
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello.
On Tue, Jun 11, 2019 at 11:11 PM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
I think the problem with those messages is that they are poorly
worded/styled, but I haven't tried to figure out how to make them
better. That may also fix the translation burden, not sure. If you
have proposals for improvement, let's hear them.
I didn't think so deeply. What I had in mind at the time was
splitting-out of the variable part from template part, as we have many
existing examples.
Here's a quick idea. We already have this:
msgid "The target cluster lacks some required control information:\n"
msgid " checkpoint next XID\n"
msgid " latest checkpoint next OID\n"
== By the way,
I found a similar but to-exit message:
controldata.c:175
| if (cluster == &old_cluster)
| pg_fatal("The source cluster lacks cluster state information:\n");
The colon should be a period?
== END OF "By the way"
so this gives me the idea that one way to fix the problem you mention is
something like this:msgid "The following source and target pg_controldata items do not match:"
msgid " block size"
msgid " maximum relation segment size"
etc. (One thing to note is that those strings already exist in the .po
files, so already translated). Obviously needs a bit of code rework
Each of the message is pg_fatal'ed. So the following insated will
work:
pg_fatal("The source and target pg_controldata item do not match:%s",
_(" maximum alignment\n"));
That seems closer to the the guideline. (But I don't think
"[SP][SP]maximum[SP]alignment\n" is not proper as a translation unit..)
(and the first new one should use the plural stuff, because it's likely
it'll only be one item that does not match). Also will need separate
messages (with plurals) formsgid "The following source pg_controldata items are invalid:"
msgid "The following target pg_controldata items are invalid:"
Something like the attached works that way.
By the way I'm a bit annoyed also by the (seemingly) random occurrence
of "old/new" and "source/target".
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
PoC_pg_upgrade_controldata_error_refactor.patchapplication/octet-stream; name=PoC_pg_upgrade_controldata_error_refactor.patchDownload
diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
index 6788f882a8..b0678e8e97 100644
--- a/src/bin/pg_upgrade/controldata.c
+++ b/src/bin/pg_upgrade/controldata.c
@@ -609,6 +609,41 @@ get_control_data(ClusterInfo *cluster, bool live_check)
}
}
+/*
+ * subroutine for checking each item in pg_control_data
+ *
+ * This is intended to be a part of CHECK_CTRL_ITEM macro.
+ * item is translated item name to be printed on error
+ * old_invalid, new_invalid, do_not_match is the check results of an item
+ * old/new_invalid are ignored if check_invalid is false.
+ */
+static void
+check_control_data_item(char *item,
+ bool check_invalid, bool old_invalid, bool new_invalid,
+ bool do_not_match)
+{
+ static char *message[] = {
+ "no error", /* should not be used */
+ gettext_noop("souce pg_controldata item is invalid:%s"),
+ gettext_noop("target pg_controldata item is invalid:%s"),
+ gettext_noop("source and target pg_controldata items do not match:%s")
+ };
+ int failure_type = 0;
+
+ if (check_invalid)
+ {
+ if (old_invalid)
+ failure_type = 1;
+ else if (new_invalid)
+ failure_type = 2;
+ }
+
+ if (failure_type == 0 && do_not_match)
+ failure_type = 3;
+
+ if (failure_type > 0)
+ pg_fatal(message[failure_type], item);
+}
/*
* check_control_data()
@@ -619,38 +654,24 @@ void
check_control_data(ControlData *oldctrl,
ControlData *newctrl)
{
- if (oldctrl->align == 0 || oldctrl->align != newctrl->align)
- pg_fatal("old and new pg_controldata alignments are invalid or do not match\n"
- "Likely one cluster is a 32-bit install, the other 64-bit\n");
+#define CHECK_CTRL_ITEM(member, item, check_invalid) \
+ check_control_data_item(_(item), check_invalid, \
+ oldctrl->member == 0, newctrl->member == 0, \
+ oldctrl->member != newctrl->member)
- if (oldctrl->blocksz == 0 || oldctrl->blocksz != newctrl->blocksz)
- pg_fatal("old and new pg_controldata block sizes are invalid or do not match\n");
-
- if (oldctrl->largesz == 0 || oldctrl->largesz != newctrl->largesz)
- pg_fatal("old and new pg_controldata maximum relation segment sizes are invalid or do not match\n");
-
- if (oldctrl->walsz == 0 || oldctrl->walsz != newctrl->walsz)
- pg_fatal("old and new pg_controldata WAL block sizes are invalid or do not match\n");
-
- if (oldctrl->walseg == 0 || oldctrl->walseg != newctrl->walseg)
- pg_fatal("old and new pg_controldata WAL segment sizes are invalid or do not match\n");
-
- if (oldctrl->ident == 0 || oldctrl->ident != newctrl->ident)
- pg_fatal("old and new pg_controldata maximum identifier lengths are invalid or do not match\n");
-
- if (oldctrl->index == 0 || oldctrl->index != newctrl->index)
- pg_fatal("old and new pg_controldata maximum indexed columns are invalid or do not match\n");
-
- if (oldctrl->toast == 0 || oldctrl->toast != newctrl->toast)
- pg_fatal("old and new pg_controldata maximum TOAST chunk sizes are invalid or do not match\n");
+ CHECK_CTRL_ITEM(align, " maximum alignment\n", true);
+ CHECK_CTRL_ITEM(blocksz, " block size\n", true);
+ CHECK_CTRL_ITEM(largesz, " large relation segment size\n", true);
+ CHECK_CTRL_ITEM(walsz, " WAL block size\n", true);
+ CHECK_CTRL_ITEM(walseg, " WAL segment size\n", true);
+ CHECK_CTRL_ITEM(ident, " maximum identifier length\n", true);
+ CHECK_CTRL_ITEM(index, " maximum number of indexed columns\n", true);
+ CHECK_CTRL_ITEM(toast, " maximum TOAST chunk size\n", true);
/* large_object added in 9.5, so it might not exist in the old cluster */
- if (oldctrl->large_object != 0 &&
- oldctrl->large_object != newctrl->large_object)
- pg_fatal("old and new pg_controldata large-object chunk sizes are invalid or do not match\n");
+ CHECK_CTRL_ITEM(large_object, " large-object chunk size\n", true);
- if (oldctrl->date_is_int != newctrl->date_is_int)
- pg_fatal("old and new pg_controldata date/time storage types do not match\n");
+ CHECK_CTRL_ITEM(date_is_int, " dates/times are integers\n", false);
/*
* float8_pass_by_value does not need to match, but is used in