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

On Fri, Aug 21, 2020 at 10:33 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Fri, Aug 21, 2020 at 9:14 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > 2.
> > + /*
> > + * If the new location is smaller then the current location in file then
> > + * we need to set the curFile and the curOffset to the new values and also
> > + * reset the pos and nbytes. Otherwise nothing to do.
> > + */
> > + else if ((newFile < file->curFile) ||
> > + newOffset < file->curOffset + file->pos)
> > + {
> > + file->curFile = newFile;
> > + file->curOffset = newOffset;
> > + file->pos = 0;
> > + file->nbytes = 0;
> > + }
> >
> > Shouldn't there be && instead of || because if newFile is greater than
> > curFile then there is no meaning to update it?
>
> I think this condition is wrong it should be,
>
> else if ((newFile < file->curFile) || ((newFile == file->curFile) &&
> (newOffset < file->curOffset + file->pos)
>
> Basically, either new file is smaller otherwise if it is the same
> then-new offset should be smaller.
>

I think we don't need to use file->pos for that as that is required
only for the current buffer, otherwise, such a condition should
suffice the need. However, I was not happy with the way code and
conditions were arranged in BufFileTruncateShared, so I have
re-arranged them and change quite a few comments in that API. Apart
from that I have updated the docs and ran pgindent for the first
patch. Do let me know if you have any more comments on the first
patch?

--
With Regards,
Amit Kapila.