On Mon, Oct 19, 2020 at 2:40 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, Oct 18, 2020 at 7:47 AM Hou, Zhijie <houzj.fnst@cn.fujitsu.com> wrote:
> >
> > Hi Vignesh,
> >
> > After having a look over the patch,
> > I have some suggestions for
> > 0003-Allow-copy-from-command-to-process-data-from-file.patch.
> >
> > 1.
> >
> > +static uint32
> > +EstimateCstateSize(ParallelContext *pcxt, CopyState cstate, List *attnamelist,
> > + char **whereClauseStr, char **rangeTableStr,
> > + char **attnameListStr, char **notnullListStr,
> > + char **nullListStr, char **convertListStr)
> > +{
> > + uint32 strsize = MAXALIGN(sizeof(SerializedParallelCopyState));
> > +
> > + strsize += EstimateStringSize(cstate->null_print);
> > + strsize += EstimateStringSize(cstate->delim);
> > + strsize += EstimateStringSize(cstate->quote);
> > + strsize += EstimateStringSize(cstate->escape);
> >
> >
> > It use function EstimateStringSize to get the strlen of null_print, delim, quote and escape.
> > But the length of null_print seems has been stored in null_print_len.
> > And delim/quote/escape must be 1 byte, so I think call strlen again seems unnecessary.
> >
> > How about " strsize += sizeof(uint32) + cstate->null_print_len + 1"
> >
>
> +1. This seems like a good suggestion but add comments for
> delim/quote/escape to indicate that we are considering one-byte for
> each. I think this will obviate the need of function
> EstimateStringSize. Another thing in this regard is that we normally
> use add_size function to compute the size but I don't see that being
> used in this and nearby computation. That helps us to detect overflow
> of addition if any.
>
> EstimateCstateSize()
> {
> ..
> +
> + strsize++;
> ..
> }
>
> Why do we need this additional one-byte increment? Does it make sense
> to add a small comment for the same?
>
Changed it to handle null_print, delim, quote & escape accordingly in
the attached patch, the one byte increment is not required, I have
removed it.
Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com