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

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)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