Re: _mdfd_getseg can be expensive

On Wed, Aug 31, 2016 at 2:09 PM, Peter Geoghegan <pg@heroku.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