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

tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com>

From: "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>
To: "k.jamison@fujitsu.com" <k.jamison@fujitsu.com>, "k.jamison@fujitsu.com" <k.jamison@fujitsu.com>
Cc: 'Amit Kapila' <amit.kapila16@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, Andres Freund <andres@anarazel.de>, Robert Haas <robertmhaas@gmail.com>, Tomas Vondra <tomas.vondra@2ndquadrant.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2020-09-16T02:38:17Z
Lists: 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 code accesses 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

Commits

  1. Fix size overflow in calculation introduced by commits d6ad34f3 and bea449c6.

  2. Optimize DropRelFileNodesAllBuffers() for recovery.

  3. Optimize DropRelFileNodeBuffers() for recovery.

  4. Cache smgrnblocks() results in recovery.

  5. Add a check to prevent overwriting valid data if smgrnblocks() gives a