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