Re: NLS handling fixes.

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: NLS handling fixes.
Date: 2018-08-20 04:23:38
Message-ID: 20180820042338.GH7403@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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 Content-Type Size
nls-fixes-v2.patch text/x-diff 5.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2018-08-20 04:38:20 simplehash.h comment
Previous Message Tom Lane 2018-08-20 03:30:41 Re: [FEATURE PATCH] pg_stat_statements with plans (v02)