Re: _mdfd_getseg can be expensive

On Wed, Aug 31, 2016 at 2:09 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> The only thing that stuck out to any degree is that we don't grow the
> "reln->md_seg_fds[forknum]" array within the new _fdvec_resize()
> function geometrically.

That new function looks like this:

> +static void
> +_fdvec_resize(SMgrRelation reln,
> + ForkNumber forknum,
> + int nseg)
> {
> - return (MdfdVec *) MemoryContextAlloc(MdCxt, sizeof(MdfdVec));
> + if (nseg == 0)
> + {
> + if (reln->md_num_open_segs[forknum] > 0)
> + {
> + pfree(reln->md_seg_fds[forknum]);
> + reln->md_seg_fds[forknum] = NULL;
> + }
> + }
> + else if (reln->md_num_open_segs[forknum] == 0)
> + {
> + reln->md_seg_fds[forknum] =
> + MemoryContextAlloc(MdCxt, sizeof(MdfdVec) * nseg);
> + }
> + else
> + {
> + reln->md_seg_fds[forknum] =
> + repalloc(reln->md_seg_fds[forknum],
> + sizeof(MdfdVec) * nseg);
> + }
> +
> + reln->md_num_open_segs[forknum] = nseg;
> }

Another tiny tweak that you might consider occurs to me here: It
couldn't hurt to "Assert(reln->md_seg_fds[forknum] == NULL)" within
the "else if (reln->md_num_open_segs[forknum] == 0)" path here, prior
to the MemoryContextAlloc(). If it's worth setting it to NULL when
there are 0 segs (i.e., "reln->md_seg_fds[forknum] = NULL"), then
perhaps it's worth checking that things are found that way later.

I guess that this is at odds with remarks in my last mail about
considering geometric allocations for the "reln->md_seg_fds[forknum]"
array. Both feedback items are more or less just things that bothered
me ever so slightly, which I don't necessarily expect you to act on,
but assume you'd like to hear.

--
Peter Geoghegan