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'rejust 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
areblocks.
 


Regards
Takayuki Tsunakawa