Re: Strange coding in _mdfd_openseg()

On Thu, Apr 04, 2019 at 12:15:52PM +0900, Kyotaro HORIGUCHI wrote:
> 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>
> > 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.

Agreed. The rest of md.c won't cope with a hole in this array, so allowing
less-than-or-equal here is futile. The patch in the original post looks fine.