RE: [Patch] Optimize dropping of relation buffers using dlist - Mailing list pgsql-hackers

From tsunakawa.takay@fujitsu.com
Subject RE: [Patch] Optimize dropping of relation buffers using dlist
Date
Msg-id TYAPR01MB2990E3035182033F94120C94FE210@TYAPR01MB2990.jpnprd01.prod.outlook.com
Whole thread Raw
In response to RE: [Patch] Optimize dropping of relation buffers using dlist  ("k.jamison@fujitsu.com" <k.jamison@fujitsu.com>)
List pgsql-hackers
The code doesn't seem to be working correctly.


(1)
+                for (block_num = 0; block_num <= nblocks; block_num++)

should be

+                for (block_num = firstDelBlock[fork_num]; block_num < nblocks; block_num++)

because:

* You only want to invalidate blocks >= firstDelBlock[fork_num], don't you?
* The relation's block number ranges from 0 to nblocks - 1.


(2)
+                    INIT_BUFFERTAG(newTag, rnode.node, forkNum[fork_num],
+                                   firstDelBlock[block_num]);

Replace firstDelBlock[fork_num] with block_num, because you want to process the current block of per-block loop.  Your
codeaccesses memory out of the bounds of the array, and doesn't invalidate any buffer.
 


(3)
+                    if (RelFileNodeEquals(bufHdr->tag.rnode, rnode.node) &&
+                        bufHdr->tag.forkNum == forkNum[fork_num] &&
+                        bufHdr->tag.blockNum >= firstDelBlock[block_num])
+                        InvalidateBuffer(bufHdr);    /* releases spinlock */
+                    else
+                        UnlockBufHdr(bufHdr, buf_state);

Replace
bufHdr->tag.blockNum >= firstDelBlock[fork_num]
with
bufHdr->tag.blockNum == block_num
because you want to check if the found buffer is for the current block of the loop.


(4)
+                /*
+                 * We've invalidated the nblocks already. Scan the shared buffers
+                 * for each fork.
+                 */
+                if (block_num > nblocks)
+                {
+                    DropRelFileNodeBuffersOfFork(rnode.node, forkNum[fork_num],
+                                                 firstDelBlock[fork_num]);
+                }

This part is unnecessary.  This invalidates all buffers that (2) failed to process, so the regression test succeeds.


Regards
Takayuki Tsunakawa


pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: Force update_process_title=on in crash recovery?
Next
From: Kyotaro Horiguchi
Date:
Subject: Re: [Patch] Optimize dropping of relation buffers using dlist