Re: Strange coding in _mdfd_openseg()

At Wed, 3 Apr 2019 17:14:36 +1300, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote in <CA+hUKG+NBw+uSzxF1os-SO6gUuw=cqO5DAybk6KnHKzgGvxhxA(at)mail(dot)gmail(dot)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