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

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?

--
With Regards,
Amit Kapila.