At Wed, 3 Apr 2019 17:14:36 +1300, Thomas Munro <thomas.mu...@gmail.com> wrote in <CA+hUKG+NBw+uSzxF1os-SO6gUuw=cqo5daybk6knhkzggvx...@mail.gmail.com> > Hello, > > I think the following conditional code is misleading, and I wonder if > it would be better like so: > > --- a/src/backend/storage/smgr/md.c > +++ b/src/backend/storage/smgr/md.c > @@ -1787,8 +1787,13 @@ _mdfd_openseg(SMgrRelation reln, ForkNumber > forknum, BlockNumber segno, > if (fd < 0) > return NULL; > > - if (segno <= reln->md_num_open_segs[forknum]) > - _fdvec_resize(reln, forknum, segno + 1); > + /* > + * Segments are always opened in order from lowest to highest, > so we must > + * be adding a new one at the end. > + */ > + Assert(segno == reln->md_num_open_segs[forknum]); > + > + _fdvec_resize(reln, forknum, segno + 1); > > /* fill the entry */ > v = &reln->md_seg_fds[forknum][segno]; > > I think the condition is always true, and with == it would also always > be true. If that weren't the case, the call to _fdvec_resize() code > would effectively leak vfds.
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]; | nextsegno <= targetseg; nextsegno++) | ... | v = _mdfd_openseg(reln, forknum, nextsegno, flags); regards. -- Kyotaro Horiguchi NTT Open Source Software Center
