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

On Wednesday, September 23, 2020 11:26 AM, Tsunakawa, Takayuki wrote:

> I looked at v14.
Thank you for checking it!

> (1)
> +        /* Get the total number of blocks for the supplied relation's
> fork */
> +        for (j = 0; j < nforks; j++)
> +        {
> +            BlockNumber        block =
> smgrnblocks(smgr_reln, forkNum[j]);
> +            nblocks += block;
> +        }
>
> Why do you sum all forks?

I revised the patch based from my understanding of Horiguchi-san's comment,
but I could be wrong.
Quoting:

"
+            /* Get the number of blocks for the supplied relation's fork */
+            nblocks = smgrnblocks(smgr_reln, forkNum[fork_num]);
+            Assert(BlockNumberIsValid(nblocks));
+
+            if (nblocks < BUF_DROP_FULLSCAN_THRESHOLD)

As mentioned upthread, the criteria whether we do full-scan or
lookup-drop is how large portion of NBUFFERS this relation-drop can be
going to invalidate.  So the nblocks above should be the sum of number
of blocks to be truncated (not just the total number of blocks) of all
designated forks.  Then once we decided to do lookup-drop method, we
do that for all forks."

> (2)
> +            if ((nblocks / (uint32)NBuffers) <
> BUF_DROP_FULLSCAN_THRESHOLD &&
> +                BlockNumberIsValid(nblocks))
> +            {
>
> The division by NBuffers is not necessary, because both sides of = are
> number of blocks.

Again I based it from my understanding of the comment above,
so nblocks is the sum of all blocks to be truncated for all forks.


> Why is BlockNumberIsValid(nblocks)) call needed?

I thought we need to ensure that nblocks is not invalid, so I also added

> (3)
>      if (reln->smgr_cached_nblocks[forknum] == blocknum)
>          reln->smgr_cached_nblocks[forknum] = blocknum + 1;
>      else
> +    {
> +        /*
> +         * DropRelFileNodeBuffers relies on the behavior that
> cached nblocks
> +         * won't be invalidated by file extension while recovering.
> +         */
> +        Assert(!InRecovery);
>          reln->smgr_cached_nblocks[forknum] =
> InvalidBlockNumber;
> +    }
>
> I think this change is not directly related to this patch and can be a separate
> patch, but I want to leave the decision up to a committer.
>
This is noted. Once we clarified the above comments, I'll put it in a separate patch if it's necessary,

Thank you very much for the reviews.

Best regards,
Kirk Jamison