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

On Friday, October 9, 2020 11:12 AM, Horiguchi-san wrote:
> I have some comments on the latest patch.

Thank you for the feedback!
I've attached the latest patches.

> visibilitymap_prepare_truncate(Relation rel, BlockNumber nheapblocks) {
> BlockNumber newnblocks;
> + bool cached;
>
> All the added variables added by 0002 is useless because all the caller sites
> are not interested in the value. smgrnblocks should accept NULL as isCached.
> (I'm agree with Tsunakawa-san that the camel-case name is not common
> there.)
>
> + nForkBlocks[i] = smgrnblocks(smgr_reln, forkNum[i],
> &isCached);
> +
> + if (!isCached)
>
> "is cached" is not the property that code is interested in. No other callers to
> smgrnblocks are interested in that property. The need for caching is purely
> internal of smgrnblocks().
> On the other hand, we are going to utilize the property of "accuracy"
> that is a biproduct of reducing fseek calls, and, again, not interested in how it
> is achieved.
> So I suggest that the name should be "accurite" or something that is not
> suggest the mechanism used under the hood.

I changed the bool param to "accurate" per your suggestion.
And I also removed the additional variables "bool cached" from the modified functions.
Now NULL values are accepted for the new boolean parameter

> + if (nTotalBlocks != InvalidBlockNumber &&
> + nBlocksToInvalidate <
> BUF_DROP_FULL_SCAN_THRESHOLD)
>
> I don't think nTotalBlocks is useful. What we need here is only total blocks for
> every forks (nForkBlocks[]) and the total number of buffers to be invalidated
> for all forks (nBlocksToInvalidate).

Alright. I also removed nTotalBlocks in v24-0003 patch.

for (i = 0; i < nforks; i++)
{
if (nForkBlocks[i] != InvalidBlockNumber &&
nBlocksToInvalidate < BUF_DROP_FULL_SCAN_THRESHOLD)
{
Optimization loop
}
else
break;
}
if (i >= nforks)
return;
{ usual buffer invalidation process }

> > > > The right side of >= should be cur_block.
> > > Fixed.
> > >= should be =, shouldn't it?
>
> It's just from a paranoia. What we are going to invalidate is blocks blockNum
> of which >= curBlock. Although actually there's no chance of any other
> processes having replaced the buffer with another page (with lower blockid)
> of the same relation after BufTableLookup(), that condition makes it sure not
> to leave blocks to be invalidated left alone.
> Sorry. What we are going to invalidate is blocks that are blocNum >=
> firstDelBlock[i]. So what I wanted to suggest was the condition should be
>
> + if (RelFileNodeEquals(bufHdr->tag.rnode,
> rnode.node) &&
> + bufHdr->tag.forkNum ==
> forkNum[j] &&
> + bufHdr->tag.blockNum >=
> firstDelBlock[j])

I used bufHdr->tag.blockNum >= firstDelBlock[i] in the latest patch.

> > Please measure and let us see just the recovery performance again because
> the critical part of the patch was modified. If the performance is good as the
> previous one, and there's no review interaction with others in progress, I'll
> mark the patch as ready for committer in a few days.
>
> The performance is expected to be kept since smgrnblocks() is called in a
> non-hot code path and actually it is called at most four times per a buffer
> drop in this patch. But it's better making it sure.

Hmm. When I repeated the performance measurement for non-recovery,
I got almost similar execution results for both master and patched.

Execution Time (in seconds)
| s_b | master | patched | %reg |
|-------|--------|---------|--------|
| 128MB | 15.265 | 14.769 | -3.36% |
| 1GB | 14.808 | 14.618 | -1.30% |
| 20GB | 24.673 | 24.425 | -1.02% |
| 100GB | 74.298 | 74.813 | 0.69% |

That is considering that I removed the recovery-related checks in the patch and just
executed the commands on a standalone server.
- if (InRecovery && reln->smgr_cached_nblocks[forknum] != InvalidBlockNumber)
+ if (reln->smgr_cached_nblocks[forknum] != InvalidBlockNumber)

OTOH, I also measured the recovery performance by having hot standby and executing failover.
The results were good and almost similar to the previously reported recovery performance.

Recovery Time (in seconds)
| s_b | master | patched | %reg |
|-------|--------|---------|--------|
| 128MB | 3.043 | 2.977 | -2.22% |
| 1GB | 3.417 | 3.41 | -0.21% |
| 20GB | 20.597 | 2.409 | -755% |
| 100GB | 66.862 | 2.409 | -2676% |

For 20GB s_b, from 20.597 s (Master) to 2.409 s (Patched).
For 100GB s_b, from 66.862 s (Master) to 2.409 s (Patched).
This is mainly benefits for large shared_buffers setting,
without compromising when shared_buffers is set to default or lower value.

If you could take a look again and if you have additional feedback or comments, I'd appreciate it.
Thank you for your time

Regards,
Kirk Jamison