Re: Parallel copy

On Fri, Oct 9, 2020 at 10:42 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Oct 8, 2020 at 12:14 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Mon, Sep 28, 2020 at 12:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > >
> > > I am convinced by the reason given by Kyotaro-San in that another
> > > thread [1] and performance data shown by Peter that this can't be an
> > > independent improvement and rather in some cases it can do harm. Now,
> > > if you need it for a parallel-copy path then we can change it
> > > specifically to the parallel-copy code path but I don't understand
> > > your reason completely.
> > >
> >
> > Whenever we need data to be populated, we will get a new data block &
> > pass it to CopyGetData to populate the data. In case of file copy, the
> > server will completely fill the data block. We expect the data to be
> > filled completely. If data is available it will completely load the
> > complete data block in case of file copy. There is no scenario where
> > even if data is present a partial data block will be returned except
> > for EOF or no data available. But in case of STDIN data copy, even
> > though there is 8K data available in data block & 8K data available in
> > STDIN, CopyGetData will return as soon as libpq buffer data is more
> > than the minread. We will pass new data block every time to load data.
> > Every time we pass an 8K data block but CopyGetData loads a few bytes
> > in the new data block & returns. I wanted to keep the same data
> > population logic for both file copy & STDIN copy i.e copy full 8K data
> > blocks & then the populated data can be required. There is an
> > alternative solution I can have some special handling in case of STDIN
> > wherein the existing data block can be passed with the index from
> > where the data should be copied. Thoughts?
> >
>
> What you are proposing as an alternative solution, isn't that what we
> are doing without the patch? IIUC, you require this because of your
> corresponding changes to handle COPY_NEW_FE in CopyReadLine(), is that
> right? If so, what is the difficulty in making it behave similar to
> the non-parallel case?
>

The alternate solution is similar to how existing copy handles STDIN copies, I have made changes in the v7 patch attached in [1] to have parallel copy handle STDIN data similar to non parallel copy, so the original comment on why this change is required has been removed from 001 patch:
> > + if (cstate->copy_dest == COPY_NEW_FE)
> > + minread = RAW_BUF_SIZE - nbytes;
> > +
> >   inbytes = CopyGetData(cstate, cstate->raw_buf + nbytes,
> > -   1, RAW_BUF_SIZE - nbytes);
> > +   minread, RAW_BUF_SIZE - nbytes);
> >
> > No comment to explain why this change is done?

[1] https://www.postgresql.org/message-id/CALDaNm1n1xW43neXSGs%3Dc7zt-mj%2BJHHbubWBVDYT9NfCoF8TuQ%40mail.gmail.com

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