remove "msg" parameter from convert_tuples_by_name
Hello, here's a pretty trivial cleanup.
Currently, you have to pass the errmsg text to convert_tuples_by_name
and convert_tuples_by_position that's going to be raised if the tuple
descriptors don't match. In the latter's case that makes sense, as each
case is pretty specific and tailored messages can be offered, so this is
useful.
However, in the case of convert_tuples_by_name, it seems we don't have
enough control over what is being called, so there's no way to
produce tailored messages -- all the callers are using the same generic
wording: "could not convert row type".
This code was introduced by dcb2bda9b704; I think back then we were
thinking that it would be possible to give different error messages for
different cases (as convert_tuples_by_position was already doing then),
however it seems clear now that that'll never happen.
I propose we get rid of it by having convert_tuples_by_name supply the
error message by itself, as in the attached patch.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Remove-msg-param-from-convert_tuples_by_name-and-fri.patchtext/x-diff; charset=us-asciiDownload+33-64
On Wed, Aug 7, 2019 at 7:47 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Hello, here's a pretty trivial cleanup.
Currently, you have to pass the errmsg text to convert_tuples_by_name
and convert_tuples_by_position that's going to be raised if the tuple
descriptors don't match. In the latter's case that makes sense, as each
case is pretty specific and tailored messages can be offered, so this is
useful.However, in the case of convert_tuples_by_name, it seems we don't have
enough control over what is being called, so there's no way to
produce tailored messages -- all the callers are using the same generic
wording: "could not convert row type".This code was introduced by dcb2bda9b704; I think back then we were
thinking that it would be possible to give different error messages for
different cases (as convert_tuples_by_position was already doing then),
however it seems clear now that that'll never happen.I propose we get rid of it by having convert_tuples_by_name supply the
error message by itself, as in the attached patch.
+1. I always wondered when writing partitioning patches why I have to
pass the same string.
If we're reducing the message string to occur only once in the source
code, can we maybe write it to be more informative? I wonder if users
aren't normally supposed to see this message?
Thanks,
Amit
On 2019-Aug-07, Amit Langote wrote:
If we're reducing the message string to occur only once in the source
code, can we maybe write it to be more informative? I wonder if users
aren't normally supposed to see this message?
Grepping for the messages given to convert_tuples_by_position yields
quite a few matches in regression test output, but none for the one in
convert_tuples_by_name. This makes me think that it isn't user-visible,
unless things go very wrong.
Pushed the patch, thanks.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Sep 4, 2019 at 3:52 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-Aug-07, Amit Langote wrote:
If we're reducing the message string to occur only once in the source
code, can we maybe write it to be more informative? I wonder if users
aren't normally supposed to see this message?Grepping for the messages given to convert_tuples_by_position yields
quite a few matches in regression test output, but none for the one in
convert_tuples_by_name. This makes me think that it isn't user-visible,
unless things go very wrong.Pushed the patch, thanks.
Thanks. I thought you'd change the ereport to elog while at it.
Thanks,
Amit
On 2019-Sep-04, Amit Langote wrote:
On Wed, Sep 4, 2019 at 3:52 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-Aug-07, Amit Langote wrote:
If we're reducing the message string to occur only once in the source
code, can we maybe write it to be more informative? I wonder if users
aren't normally supposed to see this message?Grepping for the messages given to convert_tuples_by_position yields
quite a few matches in regression test output, but none for the one in
convert_tuples_by_name. This makes me think that it isn't user-visible,
unless things go very wrong.Pushed the patch, thanks.
Thanks. I thought you'd change the ereport to elog while at it.
Oh, that didn't occur to me, but because it has errdetail and an errcode
it'd be more controversial. I don't see any reason to do it, frankly ...
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services