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

	From: Amit Kapila <amit.kapila16@gmail.com>
> if (RelFileNodeEquals(bufHdr->tag.rnode, rnode.node) &&
> + bufHdr->tag.forkNum == forkNum[j] &&
> + bufHdr->tag.blockNum >= firstDelBlock[j])
> 
> Here, I think you need to use 'i' not 'j' for forkNum and
> firstDelBlock as those are arrays w.r.t forks. That might fix the
> problem but I am not sure as I haven't tried to reproduce it.


(1)
+					INIT_BUFFERTAG(newTag, rnode.node, forkNum[j], firstDelBlock[j]);

And you need to use i here, too.

I advise you to suspect any character, any word, and any sentence.  I've found many bugs for others so far.  I'm afraid you're just seeing the code flow.


(2)
+					LWLockAcquire(newPartitionLock, LW_SHARED);
+					buf_id = BufTableLookup(&newTag, newHash);
+					LWLockRelease(newPartitionLock);
+
+					bufHdr = GetBufferDescriptor(buf_id);

Check the result of BufTableLookup() and do nothing if the block is not in the shared buffers.


(3)
+			else
+			{
+				for (j = BUF_DROP_FULLSCAN_THRESHOLD; j < NBuffers; j++)
+				{

What's the meaning of this loop?  I don't understand the start condition.  Should j be initialized to 0?


(4)
+#define BUF_DROP_FULLSCAN_THRESHOLD		(NBuffers / 2)

Wasn't it 500 instead of 2?  Anyway, I think we need to discuss this threshold later.


(5)
+			if (((int)nblocks) < BUF_DROP_FULLSCAN_THRESHOLD)

It's better to define BUF_DROP_FULLSCAN_THRESHOLD as an uint32 value instead of casting the type here, as these values are blocks.


Regards
Takayuki Tsunakawa