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

Amit Kapila <amit.kapila16@gmail.com>

From: Amit Kapila <amit.kapila16@gmail.com>
To: "k.jamison@fujitsu.com" <k.jamison@fujitsu.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>, Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>, Thomas Munro <thomas.munro@gmail.com>, Robert Haas <robertmhaas@gmail.com>, Tomas Vondra <tomas.vondra@2ndquadrant.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2020-12-23T08:51:08Z
Lists: pgsql-hackers
On Wed, Dec 23, 2020 at 1:07 PM k.jamison@fujitsu.com
<k.jamison@fujitsu.com> wrote:
>
> On Tuesday, December 22, 2020 9:11 PM, Amit Kapila wrote:
>
> > In this code, I am slightly worried about the additional cost of each time
> > checking smgrexists. Consider a case where there are many relations and only
> > one or few of them have not cached the information, in such a case we will
> > pay the cost of smgrexists for many relations without even going to the
> > optimized path. Can we avoid that in some way or at least reduce its usage to
> > only when it is required? One idea could be that we first check if the nblocks
> > information is cached and if so then we don't need to call smgrnblocks,
> > otherwise, check if it exists. For this, we need an API like
> > smgrnblocks_cahced, something we discussed earlier but preferred the
> > current API. Do you have any better ideas?
>
> Right. I understand the point that let's say there are 100 relations, and
> the first 99 non-local relations happen to enter the optimization path, but the last
> one does not, calling smgrexist() would be too costly and waste of time in that case.
> The only solution I could think of as you mentioned is to reintroduce the new API
> which we discussed before: smgrnblocks_cached().
> It's possible that we call smgrexist() only if smgrnblocks_cached() returns
> InvalidBlockNumber.
> So if everyone agrees, we can add that API smgrnblocks_cached() which will
> Include the "cached" flag parameter, and remove the additional flag modifications
> from smgrnblocks(). In this case, both DropRelFileNodeBuffers() and
> DropRelFileNodesAllBuffers() will use the new API.
>

Yeah, let's do it that way unless anyone has a better idea to suggest.
--
With Regards,
Amit Kapila.



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