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