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
