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

On Mon, Oct 5, 2020 at 6:59 AM k(dot)jamison(at)fujitsu(dot)com
<k(dot)jamison(at)fujitsu(dot)com> wrote:
>
> On Friday, October 2, 2020 11:45 AM, Horiguchi-san wrote:
>
> > Thaks for the new version.
>
> Thank you for your thoughtful reviews!
> I've attached an updated patch addressing the comments below.
>

Few comments:
===============
1.
@@ -2990,10 +3002,80 @@ DropRelFileNodeBuffers(RelFileNodeBackend
rnode, ForkNumber *forkNum,
return;
}

+ /*
+ * Get the total number of cached blocks and to-be-invalidated blocks
+ * of the relation. The cached value returned by smgrcachednblocks
+ * could be smaller than the actual number of existing buffers of the
+ * file. This is caused by buggy Linux kernels that might not have
+ * accounted the recent write. If a fork's nblocks is invalid, exit loop.
+ */
+ for (i = 0; i < nforks; i++)
+ {
+ /* Get the total nblocks for a relation's fork */
+ nForkBlocks = smgrcachednblocks(smgr_reln, forkNum[i]);
+
+ if (nForkBlocks == InvalidBlockNumber)
+ {
+ nTotalBlocks = InvalidBlockNumber;
+ break;
+ }
+ nTotalBlocks += nForkBlocks;
+ nBlocksToInvalidate = nTotalBlocks - firstDelBlock[i];
+ }
+
+ /*
+ * Do explicit hashtable probe if the total of nblocks of relation's forks
+ * is not invalid and the nblocks to be invalidated is less than the
+ * full-scan threshold of buffer pool. Otherwise, full scan is executed.
+ */
+ if (nTotalBlocks != InvalidBlockNumber &&
+ nBlocksToInvalidate < BUF_DROP_FULL_SCAN_THRESHOLD)
+ {
+ for (j = 0; j < nforks; j++)
+ {
+ BlockNumber curBlock;
+
+ nForkBlocks = smgrcachednblocks(smgr_reln, forkNum[j]);
+
+ for (curBlock = firstDelBlock[j]; curBlock < nForkBlocks; curBlock++)

What if one or more of the forks doesn't have cached value? I think
the patch will skip such forks and will invalidate/unpin buffers for
others. You probably need a local array of nForkBlocks which will be
formed first time and then used in the second loop. You also in some
way need to handle the case where that local array doesn't have cached
blocks.

2. Also, the other thing is I have asked for some testing to avoid the
small regression we have for a smaller number of shared buffers which
I don't see the results nor any change in the code. I think it is
better if you post the pending/open items each time you post a new
version of the patch.

--
With Regards,
Amit Kapila.