Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

On Wed, Aug 19, 2020 at 12:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Aug 19, 2020 at 10:10 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Mon, Aug 17, 2020 at 6:29 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > >
> > > In last patch v49-0001, there is one issue, Basically, I have called
> > > BufFileFlush in all the cases. But, ideally, we can not call this if
> > > the underlying files are deleted/truncated because those files/blocks
> > > might not exist now. So I think if the truncate position is within
> > > the same buffer we just need to adjust the buffer, otherwise we just
> > > need to set the currFile and currOffset to the absolute number and set
> > > the pos and nbytes 0. Attached patch fixes this issue.
> > >
> >
> > Few comments on the latest patch v50-0001-Extend-the-BufFile-interface
> > 1.
> > +
> > + /*
> > + * If the truncate point is within existing buffer then we can just
> > + * adjust pos-within-buffer, without flushing buffer. Otherwise,
> > + * we don't need to do anything because we have already deleted/truncated
> > + * the underlying files.
> > + */
> > + if (curFile == file->curFile &&
> > + curOffset >= file->curOffset &&
> > + curOffset <= file->curOffset + file->nbytes)
> > + {
> > + file->pos = (int) (curOffset - file->curOffset);
> > + return;
> > + }
> >
> > I think in this case you have set the position correctly but what
> > about file->nbytes? In BufFileSeek, it was okay not to update 'nbytes'
> > because the contents of the buffer are still valid but I don't think
> > the same is true here.
> >
>
> I think you need to set 'nbytes' to curOffset as per your current
> patch as that is the new size of the file.
> --- a/src/backend/storage/file/buffile.c
> +++ b/src/backend/storage/file/buffile.c
> @@ -912,6 +912,7 @@ BufFileTruncateShared(BufFile *file, int fileno,
> off_t offset)
> curOffset <= file->curOffset + file->nbytes)
> {
> file->pos = (int) (curOffset - file->curOffset);
> + file->nbytes = (int) curOffset;
> return;
> }
>
> Also, what about file 'numFiles', that can also change due to the
> removal of certain files, shouldn't that be also set in this case

Right, we need to set the numFile. I will fix this as well.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com