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

Hi Kirk san,


(1)
+ * This returns an InvalidBlockNumber when smgr_cached_nblocks is not
+ * available and when not in recovery path.

+	/*
+	 * We cannot believe the result from smgr_nblocks is always accurate
+	 * because lseek of buggy Linux kernels doesn't account for a recent
+	 * write.
+	 */
+	if (!InRecovery && result == InvalidBlockNumber)
+		return InvalidBlockNumber;
+

These are unnecessary, because mdnblocks() never returns InvalidBlockNumber and conseuently smgrnblocks() doesn't return InvalidBlockNumber.


(2)
+smgrnblocks(SMgrRelation reln, ForkNumber forknum, bool *isCached)

I think it's better to make the argument name iscached so that camel case aligns with forknum, which is not forkNum.


(3)
+	 * This is caused by buggy Linux kernels that might not have accounted
+	 * the recent write.  If a fork's nblocks is invalid, exit loop.

"accounted for" is the right English?
I think The second sentence should be described in terms of its meaning, not the program logic.  For example, something like "Give up the optimization if the block count of any fork cannot be trusted."
Likewise, express the following part in semantics.

+	 * Do explicit hashtable lookup if the total of nblocks of relation's forks
+	 * is not invalid and the nblocks to be invalidated is less than the


(4)
+		if (nForkBlocks[i] == InvalidBlockNumber)
+		{
+			nTotalBlocks = InvalidBlockNumber;
+			break;
+		}

Use isCached in if condition because smgrnblocks() doesn't return InvalidBlockNumber.


(5)
+		nBlocksToInvalidate = nTotalBlocks - firstDelBlock[i];

should be

+		nBlocksToInvalidate += (nForkBlocks[i] - firstDelBlock[i]);



(6)
+					bufHdr->tag.blockNum >= firstDelBlock[j])
+					InvalidateBuffer(bufHdr);	/* releases spinlock */

The right side of >= should be cur_block.



 
Regards
Takayuki Tsunakawa