On Thu, Apr 04, 2019 at 12:15:52PM +0900, Kyotaro HORIGUCHI wrote: > At Wed, 3 Apr 2019 13:47:46 -0700, Andres Freund <and...@anarazel.de> wrote > in <20190403204746.2yumq7c2mirmo...@alap3.anarazel.de> > > On 2019-04-04 09:24:49 +1300, Thomas Munro wrote: > > > On Wed, Apr 3, 2019 at 5:34 PM Kyotaro HORIGUCHI > > > <horiguchi.kyot...@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.
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.
