Re: NLS handling fixes.
Michael Paquier <michael@paquier.xyz>
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>, pgsql-hackers@lists.postgresql.org
Date: 2018-08-20T04:23:38Z
Lists: pgsql-hackers
Attachments
- nls-fixes-v2.patch (text/x-diff) patch v2
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
Commits
-
Fix set of NLS translation issues
- f64e65bd76ef 9.3.25 landed
- 788ae09f4a95 9.4.20 landed
- 6120f2234d76 9.5.15 landed
- 90b0f30aea0e 9.6.11 landed
- ecf56dc5e5b6 10.6 landed
- 3ef153c7157f 11.0 landed
- 72be8c29a188 12.0 landed