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