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

On Thu, Aug 20, 2020 at 2:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Aug 20, 2020 at 1:41 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Wed, Aug 19, 2020 at 1:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Wed, Aug 19, 2020 at 12:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Wed, Aug 19, 2020 at 10:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > > On Mon, Aug 17, 2020 at 6:29 PM Dilip Kumar <dilipbalaut@gmail.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.
> >
> > I think there are a couple of more problems in the truncate APIs,
> > basically, if the curFile and curOffset are already smaller than the
> > truncate location the truncate should not change that.  So the
> > truncate should only change the curFile and curOffset if it is
> > truncating the part of the file where the curFile or curOffset is
> > pointing.
> >
>
> Right, I think this can happen if one has changed those by BufFileSeek
> before doing truncate. We should fix that case as well.

Right.

> >  I will work on those along with your other comments and
> > submit the updated patch.

I have fixed this in the attached patch along with your other
comments.  I have also attached a contrib module that is just used for
testing the truncate API.


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