Re: Strange coding in _mdfd_openseg()

Hello.

At Wed, 3 Apr 2019 13:47:46 -0700, Andres Freund <andres@anarazel.de> wrote in
<20190403204746.2yumq7c2mirmodsg@alap3.anarazel.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.kyotaro@lab.ntt.co.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