Обсуждение: Cleanup - Removal of unused function parameter from CopyReadBinaryAttribute

Hi,

While checking copy from code I found that the function parameter
column_no is not used in CopyReadBinaryAttribute. I felt this could be
removed.
Attached patch contains the changes for the same.
Thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jun 18, 2020 at 7:01 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> While checking copy from code I found that the function parameter
> column_no is not used in CopyReadBinaryAttribute. I felt this could be
> removed.
> Attached patch contains the changes for the same.
> Thoughts?
>

I don't see any problem in removing this extra parameter.

However another thought, can it be used to report a bit meaningful
error for field size < 0 check?

if (fld_size < 0)
        ereport(ERROR,
                (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
                 errmsg("invalid field size for column %d", column_no)));

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2020/06/18 23:09, Bharath Rupireddy wrote:
> On Thu, Jun 18, 2020 at 7:01 PM vignesh C <vignesh21@gmail.com> wrote:
>>
>> Hi,
>>
>> While checking copy from code I found that the function parameter
>> column_no is not used in CopyReadBinaryAttribute. I felt this could be
>> removed.
>> Attached patch contains the changes for the same.
>> Thoughts?
>>
> 
> I don't see any problem in removing this extra parameter.
> 
> However another thought, can it be used to report a bit meaningful
> error for field size < 0 check?

column_no was used for that purpose in the past, but commit 0e319c7ad7
changed that. If we want to use column_no in the log message again,
it's better to check why commit 0e319c7ad7 got rid of column_no from
the message.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Thu, Jun 18, 2020 at 10:05 PM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
>
>
> On 2020/06/18 23:09, Bharath Rupireddy wrote:
> > On Thu, Jun 18, 2020 at 7:01 PM vignesh C <vignesh21@gmail.com> wrote:
> >>
> >> Hi,
> >>
> >> While checking copy from code I found that the function parameter
> >> column_no is not used in CopyReadBinaryAttribute. I felt this could be
> >> removed.
> >> Attached patch contains the changes for the same.
> >> Thoughts?
> >>
> >
> > I don't see any problem in removing this extra parameter.
> >
> > However another thought, can it be used to report a bit meaningful
> > error for field size < 0 check?
>
> column_no was used for that purpose in the past, but commit 0e319c7ad7
> changed that.
>

Yeah, but not sure why?  By looking at the commit message and change
it is difficult to say why it has been removed?  Tom has made that
change but I don't think he would remember it, in any case, adding him
in the email to see if he remembers anything related to it.

commit 0e319c7ad7665673103f0b10752700fd2f33acd3
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Mon Sep 29 22:06:40 2003 +0000

    Improve context display for failures during COPY IN, as recently
    discussed on pghackers.
..
..
@@ -1917,7 +2019,7 @@ CopyReadBinaryAttribute(int column_no, FmgrInfo
*flinfo, Oid typelem,
  if (fld_size < 0)
  ereport(ERROR,
  (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
- errmsg("invalid size for field %d", column_no)));
+ errmsg("invalid field size")));

  /* reset attribute_buf to empty, and load raw data in it */
  attribute_buf.len = 0;
@@ -1944,8 +2046,7 @@ CopyReadBinaryAttribute(int column_no, FmgrInfo
*flinfo, Oid typelem,
  if (attribute_buf.cursor != attribute_buf.len)
  ereport(ERROR,
  (errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
- errmsg("incorrect binary data format in field %d",
- column_no)));
+ errmsg("incorrect binary data format")));

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Thu, Jun 18, 2020 at 10:05 PM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
>> column_no was used for that purpose in the past, but commit 0e319c7ad7
>> changed that.

> Yeah, but not sure why?  By looking at the commit message and change
> it is difficult to say why it has been removed?  Tom has made that
> change but I don't think he would remember it, in any case, adding him
> in the email to see if he remembers anything related to it.

Hm, no, that commit is nearly old enough to vote :-(

However, I dug around in the archives, and I found what seems to be
the relevant pghackers thread:

https://www.postgresql.org/message-id/flat/28188.1064615075%40sss.pgh.pa.us#8e0c07452bb7e729829d456cfb0ec485

Looking at that, I think I concluded that these error cases are not useful
indications of problems within the specific column's data, but most likely
indicate corruption at the level of the overall COPY line format; ergo the
line-level context display is sufficient.  You could quibble with that
conclusion of course, but if you agree with it, then the column_no
parameter is useless here.  I probably just failed to notice at the time
that the parameter was otherwise unused, else I would have removed it.

            regards, tom lane

On Fri, Jun 19, 2020 at 9:18 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > On Thu, Jun 18, 2020 at 10:05 PM Fujii Masao
> > <masao.fujii@oss.nttdata.com> wrote:
> >> column_no was used for that purpose in the past, but commit 0e319c7ad7
> >> changed that.
>
> > Yeah, but not sure why?  By looking at the commit message and change
> > it is difficult to say why it has been removed?  Tom has made that
> > change but I don't think he would remember it, in any case, adding him
> > in the email to see if he remembers anything related to it.
>
> Hm, no, that commit is nearly old enough to vote :-(
>
> However, I dug around in the archives, and I found what seems to be
> the relevant pghackers thread:
>
> https://www.postgresql.org/message-id/flat/28188.1064615075%40sss.pgh.pa.us#8e0c07452bb7e729829d456cfb0ec485
>
> Looking at that, I think I concluded that these error cases are not useful
> indications of problems within the specific column's data, but most likely
> indicate corruption at the level of the overall COPY line format; ergo the
> line-level context display is sufficient.  You could quibble with that
> conclusion of course, but if you agree with it, then the column_no
> parameter is useless here.
>

I don't see any problem with your conclusion and the fact that we
haven't came across any case which requires column_no in such messages
favors your conclusion.

>  I probably just failed to notice at the time
> that the parameter was otherwise unused, else I would have removed it.
>

No issues, I can take care of this (probably in HEAD only).

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jun 18, 2020 at 10:05 PM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2020/06/18 23:09, Bharath Rupireddy wrote:
> > On Thu, Jun 18, 2020 at 7:01 PM vignesh C <vignesh21@gmail.com> wrote:
> >>
> >> Hi,
> >>
> >> While checking copy from code I found that the function parameter
> >> column_no is not used in CopyReadBinaryAttribute. I felt this could be
> >> removed.
> >> Attached patch contains the changes for the same.
> >> Thoughts?
> >>
> >
> > I don't see any problem in removing this extra parameter.
> >
> > However another thought, can it be used to report a bit meaningful
> > error for field size < 0 check?
>
> column_no was used for that purpose in the past, but commit 0e319c7ad7
> changed that. If we want to use column_no in the log message again,
> it's better to check why commit 0e319c7ad7 got rid of column_no from
> the message.

I noticed that displaying of column information is present and it is
done in a different way. Basically cstate->cur_attname is set with the
column name before calling CopyReadBinaryAttribute function. If there
is any error in CopyReadBinaryAttribute function,
CopyFromErrorCallback will be called. CopyFromErrorCallback function
takes care of displaying the column name by using cstate->cur_attname.
I tried simulating this and it displays the column name neatly in the
error message.:
postgres=# copy t1 from '/home/db/copydata/t1_copy.bin' with (format 'binary');
ERROR:  invalid field size
CONTEXT:  COPY t1, line 1, column c1
I feel we can safely remove the parameter as in the patch.

Regards,
Vignesh

> > > I don't see any problem in removing this extra parameter.
> > >
> > > However another thought, can it be used to report a bit meaningful
> > > error for field size < 0 check?
> >
> > column_no was used for that purpose in the past, but commit 0e319c7ad7
> > changed that. If we want to use column_no in the log message again,
> > it's better to check why commit 0e319c7ad7 got rid of column_no from
> > the message.
>
> I noticed that displaying of column information is present and it is
> done in a different way. Basically cstate->cur_attname is set with the
> column name before calling CopyReadBinaryAttribute function. If there
> is any error in CopyReadBinaryAttribute function,
> CopyFromErrorCallback will be called. CopyFromErrorCallback function
> takes care of displaying the column name by using cstate->cur_attname.
> I tried simulating this and it displays the column name neatly in the
> error message.:
> postgres=# copy t1 from '/home/db/copydata/t1_copy.bin' with (format 'binary');
> ERROR:  invalid field size
> CONTEXT:  COPY t1, line 1, column c1
> I feel we can safely remove the parameter as in the patch.
>

thanks for this information.

+1 for this patch.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com