RE: [Patch] Optimize dropping of relation buffers using dlist

On Tuesday, September 29, 2020 10:35 AM, Horiguchi-san wrote:

> FWIW, I (and maybe Amit) am thinking that the property we need here is not it
> is cached or not but the accuracy of the returned file length, and that the
> "cached" property should be hidden behind the API.
>
> Another reason for not adding this function is the cached value is not really
> reliable on non-recovery environment.
>
> > So in the new function, it goes something like:
> >     if (InRecovery)
> >     {
> >         if (reln->smgr_cached_nblocks[forknum] !=
> InvalidBlockNumber)
> >             return reln->smgr_cached_nblocks[forknum];
> >         else
> >             return InvalidBlockNumber;
> >     }
>
> If we add the new function, it should reutrn InvalidBlockNumber without
> consulting smgr_nblocks().

So here's how I revised it
smgrcachednblocks(SMgrRelation reln, ForkNumber forknum)
{
    if (InRecovery)
    {
        if (reln->smgr_cached_nblocks[forknum] != InvalidBlockNumber)
            return reln->smgr_cached_nblocks[forknum];
    }
    return InvalidBlockNumber;


> Hmm. The current loop in DropRelFileNodeBuffers looks like this:
>
>     if (InRecovery)
>        for (for each forks)
>           if (the fork meets the criteria)
>              <optimized dropping>
>           else
>              <full scan>
>
> I think this is somewhat different from the current discussion. Whether we
> sum-up the number of blcoks for all forks or just use that of the main fork, we
> should take full scan if we failed to know the accurate size for any one of the
> forks. (In other words, it is stupid that we run a full scan for more than one
> fork at a
> drop.)
>
> Come to think of that, we can naturally sum-up all forks' blocks since anyway
> we need to call smgrnblocks for all forks to know the optimzation is usable.

I understand. We really don't have to enter the optimization when we know the
file size is inaccurate. That also makes the patch simpler.

> So that block would be something like this:
>
>     for (forks of the rel)
>       /* the function returns InvalidBlockNumber if !InRecovery */
>       if (smgrnblocks returned InvalidBlockNumber)
>          total_blocks = InvalidBlockNumber;
>          break;
>       total_blocks += nbloks of this fork
>
>     /* <we could rely on the fact that InvalidBlockNumber is zero> */
>     if (total_blocks != InvalidBlockNumber && total_blocks < threshold)
>           for (forks of the rel)
>           for (blocks of the fork)
>              <try dropping the buffer for the block>
>     else
>        <full scan dropping>

I followed this logic in the attached patch.
Thank you very much for the thoughtful reviews.

Performance measurement for large shared buffers to follow.

Best regards,
Kirk Jamison