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