On Fri, Oct 9, 2020 at 10:42 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Oct 8, 2020 at 12:14 AM vignesh C <vignes...@gmail.com> wrote: > > > > On Mon, Sep 28, 2020 at 12:19 PM Amit Kapila <amit.kapil...@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
