Re: NLS handling fixes.

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));