Re: Strange coding in _mdfd_openseg()

Hello.

At Wed, 3 Apr 2019 13:47:46 -0700, Andres Freund <andres(at)anarazel(dot)de> wrote in <20190403204746(dot)2yumq7c2mirmodsg(at)alap3(dot)anarazel(dot)de>
> Hi,
>
> On 2019-04-04 09:24:49 +1300, Thomas Munro wrote:
> > On Wed, Apr 3, 2019 at 5:34 PM Kyotaro HORIGUCHI
> > <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > > I may be missing something, but it seems possible that
> > > _mdfd_getseg calls it with segno > opensegs.
> > >
> > > | for (nextsegno = reln->md_num_open_segs[forknum];
> >
> > Here nextsegno starts out equal to opensegs.
> >
> > > | nextsegno <= targetseg; nextsegno++)
> >
> > Here we add one to nextsegno...
> >
> > > | ...
> > > | v = _mdfd_openseg(reln, forknum, nextsegno, flags);
> >
> > ... after adding one to opensegs. So they're always equal. This fits
> > a general programming pattern when appending to an array, the next
> > element's index is the same as the number of elements. But I claim
> > the coding is weird, because _mdfd_openseg's *looks* like it can
> > handle opening segments in any order, except that the author
> > accidentally wrote "<=" instead of ">=". In fact it can't open them
> > in any order, because we don't support "holes" in the array. So I
> > think it should really be "==", and it should be an assertion, not a
> > condition.
>
> Yea, I totally agree it's weird. I'm not sure if I'd go for an assertion
> of equality, or just invert the >= (which I agree I probably just
> screwed up and didn't notice when reviewing the patch because it looked
> close enough to correct and it didn't have a measurable effect).

I looked there and agreed. _mdfd_openseg is always called just to
add one new segment after the last opened segment by the two
callers. So I think == is better.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center