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

On Wednesday, December 9, 2020 10:58 AM, Tsunakawa, Takayuki wrote:
> From: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
> > At Tue, 8 Dec 2020 16:28:41 +0530, Amit Kapila
> > <amit(dot)kapila16(at)gmail(dot)com> wrote in
> > I also can't think of a way to use an optimized path for such cases
> > > but I don't agree with your comment on if it is common enough that
> > > we leave this optimization entirely for the truncate path.
> >
> > An ugly way to cope with it would be to let other smgr functions
> > manage the cached value, for example, by calling smgrnblocks while
> > InRecovery. Or letting smgr remember the maximum block number ever
> > accessed. But we cannot fully rely on that since smgr can be closed
> > midst of a session and smgr doesn't offer such persistence. In the
> > first place smgr doesn't seem to be the place to store such persistent
> > information.
>
> Yeah, considering the future evolution of this patch to operations during
> normal running, I don't think that would be a good fit, either.
>
> Then, the as we're currently targeting just recovery, the options we can take
> are below. Which would vote for? My choice would be (3) > (2) > (1).
>
>
> (1)
> Use the cached flag in both VACUUM (0003) and TRUNCATE (0004).
> This brings the most safety and code consistency.
> But this would not benefit from optimization for TRUNCATE in unexpectedly
> many cases -- when TOAST storage exists but it's not written, or FSM/VM is
> not updated after checkpoint.
>
>
> (2)
> Use the cached flag in VACUUM (0003), but use InRecovery instead of the
> cached flag in TRUNCATE (0004).
> This benefits from the optimization in all cases.
> But this lacks code consistency.
> You may be afraid of safety if the startup process smgrclose()s the relation
> after the shared buffer flushing hits disk full. However, startup process
> doesn't smgrclose(), so it should be safe. Just in case the startup process
> smgrclose()s, the worst consequence is PANIC shutdown after repeated
> failure of checkpoints due to lingering orphaned dirty shared buffers. Accept
> it as Thomas-san's devil's suggestion.
>
>
> (3)
> Do not use the cached flag in either VACUUM (0003) or TRUNCATE (0004).
> This benefits from the optimization in all cases.
> The code is consistent and smaller.
> As for the safety, this is the same as (2), but it applies to VACUUM as well.

If we want code consistency, then we'd fall in either 1 or 3.
And if we want to take the benefits of optimization for both DropRelFileNodeBuffers
and DropRelFileNodesAllBuffers, then I'd choose 3.
However, if the reviewers and committer want to make use of the "cached" flag,
then we can live with "cached" value in place there even if it's not common to
get the optimization for TRUNCATE path. So only VACUUM would take the most
benefit.
My vote is also (3), then (2), and (1).

Regards,
Kirk Jamison