On Wed, Aug 19, 2020 at 12:20 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Aug 19, 2020 at 10:10 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Mon, Aug 17, 2020 at 6:29 PM Dilip Kumar <dilipbal...@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. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com