On Mon, Dec 16, 2019 at 3:26 PM Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
>
> On Sat, 14 Dec 2019 at 11:59, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > I have also made minor changes related to below code in patch:
> > - else if (readBytes != sizeof(ReorderBufferDiskChange))
> > +
> > + file->curOffset += readBytes;
> > +
> > + if (readBytes !=
> > sizeof(ReorderBufferDiskChange))
> >
> > Why the size is added before the error check?
> The logic was : even though it's an error that the readBytes does not
> match the expected size, the file read is successful so update the vfd
> offset as early as possible. In our case, this might not matter much,
> but who knows, in the future, in the exception block (say, in
> ReorderBufferIterTXNFinish, someone assumes that the file offset is
> correct and does something with that, then we will get in trouble,
> although I agree that it's very unlikely.
>
I am not sure if there is any such need, but even if it is there, I
think updating after a *short* read (read less than expected) doesn't
seem like a good idea because there is clearly some problem with the
read call. Also, in the case below that case where we read the actual
change data, the offset is updated after the check of *short* read. I
don't see any advantage in such an inconsistency. I still feel it is
better to update the offset after all error checks.
>
> > and see if you can run perltidy for the test file.
> Hmm, I tried perltidy, and it seems to mostly add a space after ( and
> a space before ) if there's already; so "('postgres'," is replaced by
> "(<space> 'postgres',". And this is going to be inconsistent with
> other places. And it replaces tab with spaces. Do you think we should
> try perltidy, or have we before been using this tool for the tap tests
> ?
>
See text in src/test/perl/README (Note that all tests and test tools
should have perltidy run on them before patches are submitted, using
perltidy - profile=src/tools/pgindent/perltidyrc). It is recommended
to use perltidy.
Now, if it is making the added code inconsistent with nearby code,
then I suggest to leave it.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com