Обсуждение: 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

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