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