Thread: NLS handling fixes.
Hello. I found that backend's .po file has lines for GUC
descriptions but we won't see them anywhere.
The cause is GetConfigOptionByNum is fogetting to retrieve
translations for texts that have been marked with gettext_noop.
Regarding GUCs, group names, short desc and long desc have
translations so the attached first patch (fix_GUC_nls.patch) let
the translations appear.
Besides GUCs, I found another misuse of gettext_noop in
pg_GSS_recvauth. (2nd fix_GSSerr_nls.patch)
view_query_is_auto_updatable() and most of its caller are making
the same mistake in a similar way. All caller sites require only
translated message but bringing translated message around doesn't
seem good so the attached third patch adds _() to all required
places. (3rd fix_view_updt_nls.patch, 5th fix_vacuumdb_nls.patch)
psql is making a bit different mistake. \gdesc seems intending
the output columns names in NLS string but they actually
aren't. DescribeQuery is using PrintQueryResults but it is
intended to be used only for SendQuery. Replacing it with
printQuery let \gdesc print NLS string but I'm not sure it is the
right thing to fix this. (4th, fix psql_nls.patch)
plperl/plpgsql/tcl have another kind of problem in NLS. It
installs some GUC parameters and their descriptions actually have
a translation but in *its own* po file. So GetConfigOptionByNum
cannot get them. I don't have an idea to fix this for now.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index b05fb209bb..7b023c0064 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -8419,13 +8419,13 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow)
values[2] = NULL;
/* group */
- values[3] = config_group_names[conf->group];
+ values[3] = gettext(config_group_names[conf->group]);
/* short_desc */
- values[4] = conf->short_desc;
+ values[4] = gettext(conf->short_desc);
/* extra_desc */
- values[5] = conf->long_desc;
+ values[5] = gettext(conf->long_desc);
/* context */
values[6] = GucContext_Names[conf->context];
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index cecd104b4a..43a93e493a 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1227,7 +1227,7 @@ pg_GSS_recvauth(Port *port)
{
gss_delete_sec_context(&lmin_s, &port->gss->ctx, GSS_C_NO_BUFFER);
pg_GSS_error(ERROR,
- gettext_noop("accepting GSS security context failed"),
+ gettext("accepting GSS security context failed"),
maj_stat, min_stat);
}
@@ -1253,7 +1253,7 @@ pg_GSS_recvauth(Port *port)
maj_stat = gss_display_name(&min_stat, port->gss->name, &gbuf, NULL);
if (maj_stat != GSS_S_COMPLETE)
pg_GSS_error(ERROR,
- gettext_noop("retrieving GSS user name failed"),
+ gettext("retrieving GSS user name failed"),
maj_stat, min_stat);
/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index eb2d33dd86..2ac7eae84b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10807,7 +10807,7 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("WITH CHECK OPTION is supported only on automatically updatable views"),
- errhint("%s", view_updatable_error)));
+ errhint("%s", _(view_updatable_error))));
}
}
diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
index 7d4511c585..ffb71c0ea7 100644
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -502,7 +502,7 @@ DefineView(ViewStmt *stmt, const char *queryString,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("WITH CHECK OPTION is supported only on automatically updatable views"),
- errhint("%s", view_updatable_error)));
+ errhint("%s", _(view_updatable_error))));
}
/*
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index b56995925b..1894e812e6 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1640,7 +1640,14 @@ DescribeQuery(const char *query, double *elapsed_msec)
}
if (OK && results)
- OK = PrintQueryResults(results);
+ {
+ printQueryOpt myopt = pset.popt;
+
+ myopt.nullPrint = NULL;
+ myopt.title = NULL;
+ myopt.translate_header = true;
+ printQuery(results, &myopt, pset.queryFout, false, pset.logfile);
+ }
termPQExpBuffer(&buf);
}
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 60f8b1c394..9408b8f869 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -371,7 +371,7 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
{
if (stage != ANALYZE_NO_STAGE)
printf(_("%s: processing database \"%s\": %s\n"),
- progname, PQdb(conn), stage_messages[stage]);
+ progname, PQdb(conn), _(stage_messages[stage]));
else
printf(_("%s: vacuuming database \"%s\"\n"),
progname, PQdb(conn));
On Fri, Aug 10, 2018 at 03:21:31PM +0900, Kyotaro HORIGUCHI wrote: > The cause is GetConfigOptionByNum is forgetting to retrieve > translations for texts that have been marked with gettext_noop. > > Regarding GUCs, group names, short desc and long desc have > translations so the attached first patch (fix_GUC_nls.patch) let > the translations appear. > > Besides GUCs, I found another misuse of gettext_noop in > pg_GSS_recvauth. (2nd fix_GSSerr_nls.patch) > > view_query_is_auto_updatable() and most of its caller are making > the same mistake in a similar way. All caller sites require only > translated message but bringing translated message around doesn't > seem good so the attached third patch adds _() to all required > places. (3rd fix_view_updt_nls.patch, 5th fix_vacuumdb_nls.patch) I have been looking at all the things you are proposing here, and it seems to me that you are right for these. I lack a bit of knowledge about the translation of items, but can such things be back-patched? > psql is making a bit different mistake. \gdesc seems intending > the output columns names in NLS string but they actually > aren't. DescribeQuery is using PrintQueryResults but it is > intended to be used only for SendQuery. Replacing it with > printQuery let \gdesc print NLS string but I'm not sure it is the > right thing to fix this. (4th, fix psql_nls.patch) This one I am not sure though... -- Michael
Attachment
On 2018-Aug-10, Michael Paquier wrote: > On Fri, Aug 10, 2018 at 03:21:31PM +0900, Kyotaro HORIGUCHI wrote: > > The cause is GetConfigOptionByNum is forgetting to retrieve > > translations for texts that have been marked with gettext_noop. > > > > Regarding GUCs, group names, short desc and long desc have > > translations so the attached first patch (fix_GUC_nls.patch) let > > the translations appear. > > > > Besides GUCs, I found another misuse of gettext_noop in > > pg_GSS_recvauth. (2nd fix_GSSerr_nls.patch) > > > > view_query_is_auto_updatable() and most of its caller are making > > the same mistake in a similar way. All caller sites require only > > translated message but bringing translated message around doesn't > > seem good so the attached third patch adds _() to all required > > places. (3rd fix_view_updt_nls.patch, 5th fix_vacuumdb_nls.patch) > > I have been looking at all the things you are proposing here, and it > seems to me that you are right for these. I lack a bit of knowledge > about the translation of items, but can such things be back-patched? Well, if I understood correctly, the translations would have the messages already translated -- the problem is that they are not used at runtime. So, yes, this should be backpatched. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Michael Paquier <michael@paquier.xyz> writes:
> I have been looking at all the things you are proposing here, and it
> seems to me that you are right for these. I lack a bit of knowledge
> about the translation of items, but can such things be back-patched?
I would certainly *not* back-patch the GetConfigOptionByNum change,
as that will be a user-visible behavioral change that people will
not be expecting. We might get away with back-patching the other changes,
but SHOW ALL output seems like something that people might be expecting
not to change drastically in a minor release.
Some general review notes:
* I believe our usual convention is to write _() not gettext().
This patch set is pretty schizophrenic about that.
* In the patch fixing view_query_is_auto_updatable misuse, nothing's
been done to remove the underlying cause of the errors, which IMO
is that the header comment for view_query_is_auto_updatable fails to
specify the return convention. I'd consider adding a comment along
the lines of
* view_query_is_auto_updatable - test whether the specified view definition
* represents an auto-updatable view. Returns NULL (if the view can be updated)
* or a message string giving the reason that it cannot be.
+*
+* The returned string has not been translated; if it is shown as an error
+* message, the caller should apply _() to translate it.
*
* Perhaps pg_GSS_error likewise could use a comment saying the error
string must be translated already. Or we could leave its callers alone
and put the _() into it, but either way the API contract ought to be
written down.
* The proposed patch for psql/common.c seems completely wrong, or at
least it has a lot of side-effects other than enabling header translation,
and I doubt they are desirable.
regards, tom lane
On Fri, Aug 10, 2018 at 04:50:55PM -0400, Tom Lane wrote: > I would certainly *not* back-patch the GetConfigOptionByNum change, > as that will be a user-visible behavioral change that people will > not be expecting. We might get away with back-patching the other changes, > but SHOW ALL output seems like something that people might be expecting > not to change drastically in a minor release. Agreed, some folks may rely on that. > * In the patch fixing view_query_is_auto_updatable misuse, nothing's > been done to remove the underlying cause of the errors, which IMO > is that the header comment for view_query_is_auto_updatable fails to > specify the return convention. I'd consider adding a comment along > the lines of > > * view_query_is_auto_updatable - test whether the specified view definition > * represents an auto-updatable view. Returns NULL (if the view can be updated) > * or a message string giving the reason that it cannot be. > +* > +* The returned string has not been translated; if it is shown as an error > +* message, the caller should apply _() to translate it. That sounds right. A similar comment should be added for view_cols_are_auto_updatable and view_col_is_auto_updatable. > * Perhaps pg_GSS_error likewise could use a comment saying the error > string must be translated already. Or we could leave its callers alone > and put the _() into it, but either way the API contract ought to be > written down. Both things are indeed possible. As pg_SSPI_error applies translation beforehand, I have taken the approach to let the caller just apply _(). For two messages that does not matter much one way or another. > * The proposed patch for psql/common.c seems completely wrong, or at > least it has a lot of side-effects other than enabling header translation, > and I doubt they are desirable. I doubted about it, and at closer look I think that you are right, so +1 for discarding this one. Attached is a patch which should address all the issues reported and all the remarks done. What do you think? -- Michael
Attachment
At Mon, 20 Aug 2018 13:23:38 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180820042338.GH7403@paquier.xyz>
> On Fri, Aug 10, 2018 at 04:50:55PM -0400, Tom Lane wrote:
> > I would certainly *not* back-patch the GetConfigOptionByNum change,
> > as that will be a user-visible behavioral change that people will
> > not be expecting. We might get away with back-patching the other changes,
> > but SHOW ALL output seems like something that people might be expecting
> > not to change drastically in a minor release.
>
> Agreed, some folks may rely on that.
>
> > * In the patch fixing view_query_is_auto_updatable misuse, nothing's
> > been done to remove the underlying cause of the errors, which IMO
> > is that the header comment for view_query_is_auto_updatable fails to
> > specify the return convention. I'd consider adding a comment along
> > the lines of
> >
> > * view_query_is_auto_updatable - test whether the specified view definition
> > * represents an auto-updatable view. Returns NULL (if the view can be updated)
> > * or a message string giving the reason that it cannot be.
> > +*
> > +* The returned string has not been translated; if it is shown as an error
> > +* message, the caller should apply _() to translate it.
>
> That sounds right. A similar comment should be added for
> view_cols_are_auto_updatable and view_col_is_auto_updatable.
>
> > * Perhaps pg_GSS_error likewise could use a comment saying the error
> > string must be translated already. Or we could leave its callers alone
> > and put the _() into it, but either way the API contract ought to be
> > written down.
>
> Both things are indeed possible. As pg_SSPI_error applies translation
> beforehand, I have taken the approach to let the caller just apply _().
> For two messages that does not matter much one way or another.
>
> > * The proposed patch for psql/common.c seems completely wrong, or at
> > least it has a lot of side-effects other than enabling header translation,
> > and I doubt they are desirable.
>
> I doubted about it, and at closer look I think that you are right, so +1
> for discarding this one.
>
> Attached is a patch which should address all the issues reported and all
> the remarks done. What do you think?
Agreed on all of the above, but if we don't need translation in
the header line of \gdesc, gettext_noop for the strings is
useless.
A period is missing in the comment for
view_col_is_auto_updatable.
The attached is the fixed version.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7cedc28c6b..2a12d64aeb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10826,7 +10826,7 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("WITH CHECK OPTION is supported only on automatically updatable views"),
- errhint("%s", view_updatable_error)));
+ errhint("%s", _(view_updatable_error))));
}
}
diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
index 7d4511c585..ffb71c0ea7 100644
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -502,7 +502,7 @@ DefineView(ViewStmt *stmt, const char *queryString,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("WITH CHECK OPTION is supported only on automatically updatable views"),
- errhint("%s", view_updatable_error)));
+ errhint("%s", _(view_updatable_error))));
}
/*
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index cecd104b4a..18c4921d61 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1037,6 +1037,10 @@ static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = &GSS_C_NT_USER_NAME_desc;
#endif
+/*
+ * Generate an error for GSSAPI authentication. The caller needs to apply
+ * _() to errmsg to make it translatable.
+ */
static void
pg_GSS_error(int severity, const char *errmsg, OM_uint32 maj_stat, OM_uint32 min_stat)
{
@@ -1227,7 +1231,7 @@ pg_GSS_recvauth(Port *port)
{
gss_delete_sec_context(&lmin_s, &port->gss->ctx, GSS_C_NO_BUFFER);
pg_GSS_error(ERROR,
- gettext_noop("accepting GSS security context failed"),
+ _("accepting GSS security context failed"),
maj_stat, min_stat);
}
@@ -1253,7 +1257,7 @@ pg_GSS_recvauth(Port *port)
maj_stat = gss_display_name(&min_stat, port->gss->name, &gbuf, NULL);
if (maj_stat != GSS_S_COMPLETE)
pg_GSS_error(ERROR,
- gettext_noop("retrieving GSS user name failed"),
+ _("retrieving GSS user name failed"),
maj_stat, min_stat);
/*
@@ -1317,6 +1321,11 @@ pg_GSS_recvauth(Port *port)
*----------------------------------------------------------------
*/
#ifdef ENABLE_SSPI
+
+/*
+ * Generate an error for SSPI authentication. The caller needs to apply
+ * _() to errmsg to make it translatable.
+ */
static void
pg_SSPI_error(int severity, const char *errmsg, SECURITY_STATUS r)
{
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 3123ee274d..d830569641 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -2217,6 +2217,9 @@ view_has_instead_trigger(Relation view, CmdType event)
* is auto-updatable. Returns NULL (if the column can be updated) or a message
* string giving the reason that it cannot be.
*
+ * The returned string has not been translated; if it is shown as an error
+ * message, the caller should apply _() to translate it.
+ *
* Note that the checks performed here are local to this view. We do not check
* whether the referenced column of the underlying base relation is updatable.
*/
@@ -2256,6 +2259,9 @@ view_col_is_auto_updatable(RangeTblRef *rtr, TargetEntry *tle)
* view_query_is_auto_updatable - test whether the specified view definition
* represents an auto-updatable view. Returns NULL (if the view can be updated)
* or a message string giving the reason that it cannot be.
+
+ * The returned string has not been translated; if it is shown as an error
+ * message, the caller should apply _() to translate it.
*
* If check_cols is true, the view is required to have at least one updatable
* column (necessary for INSERT/UPDATE). Otherwise the view's columns are not
@@ -2396,6 +2402,9 @@ view_query_is_auto_updatable(Query *viewquery, bool check_cols)
* required columns can be updated) or a message string giving the reason that
* they cannot be.
*
+ * The returned string has not been translated; if it is shown as an error
+ * message, the caller should apply _() to translate it.
+ *
* This should be used for INSERT/UPDATE to ensure that we don't attempt to
* assign to any non-updatable columns.
*
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 9989d3a351..9b9400c30a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -8454,13 +8454,13 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow)
values[2] = NULL;
/* group */
- values[3] = config_group_names[conf->group];
+ values[3] = _(config_group_names[conf->group]);
/* short_desc */
- values[4] = conf->short_desc;
+ values[4] = _(conf->short_desc);
/* extra_desc */
- values[5] = conf->long_desc;
+ values[5] = _(conf->long_desc);
/* context */
values[6] = GucContext_Names[conf->context];
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index b56995925b..81178c05bc 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1594,10 +1594,8 @@ DescribeQuery(const char *query, double *elapsed_msec)
initPQExpBuffer(&buf);
printfPQExpBuffer(&buf,
- "SELECT name AS \"%s\", pg_catalog.format_type(tp, tpm) AS \"%s\"\n"
- "FROM (VALUES ",
- gettext_noop("Column"),
- gettext_noop("Type"));
+ "SELECT name AS \"Column\", pg_catalog.format_type(tp, tpm) AS \"Type\"\n"
+ "FROM (VALUES ");
for (i = 0; i < PQnfields(results); i++)
{
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 6ab77b6206..bcea9e556d 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -371,7 +371,7 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
{
if (stage != ANALYZE_NO_STAGE)
printf(_("%s: processing database \"%s\": %s\n"),
- progname, PQdb(conn), stage_messages[stage]);
+ progname, PQdb(conn), _(stage_messages[stage]));
else
printf(_("%s: vacuuming database \"%s\"\n"),
progname, PQdb(conn));
On Tue, Aug 21, 2018 at 11:49:05AM +0900, Kyotaro HORIGUCHI wrote: > Agreed on all of the above, but if we don't need translation in > the header line of \gdesc, gettext_noop for the strings is > useless. I have let that one alone, as all columns showing up in psql have the same, consistent way of handling things. > A period is missing in the comment for view_col_is_auto_updatable. Fixed this one, and pushed. All things are back-patched where they apply, except for the part of GetConfigOptionByNum getting only on HEAD as it could be surprising after a minor upgrade as Tom has suggested. -- Michael