Re: Cleanup - Removal of unused function parameter from CopyReadBinaryAttribute

On Thu, Jun 18, 2020 at 10:05 PM Fujii Masao
<masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
>
> On 2020/06/18 23:09, Bharath Rupireddy wrote:
> > On Thu, Jun 18, 2020 at 7:01 PM vignesh C <vignesh21(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)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