Re: Why doesn't GiST VACUUM require a super-exclusive lock, like nbtree VACUUM?

On Wed, 5 Mar 2025 at 19:19, Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)com> wrote:
>
> On Wed, 5 Mar 2025 at 10:04, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> >
> > On 28/02/2025 03:53, Peter Geoghegan wrote:
> > > On Sat, Feb 8, 2025 at 8:47 AM Michail Nikolaev
> > > <michail(dot)nikolaev(at)gmail(dot)com> wrote:
> > >> Just some commit messages + few cleanups.
> > >
> > > I'm worried about this:
> > >
> > > +These longer pin lifetimes can cause buffer exhaustion with messages like "no
> > > +unpinned buffers available" when the index has many pages that have similar
> > > +ordering; but future work can figure out how to best work that out.
> > >
> > > I think that we should have some kind of upper bound on the number of
> > > pins that can be acquired at any one time, in order to completely
> > > avoid these problems. Solving that problem will probably require GiST
> > > expertise that I don't have right now.
> >
> > +1. With no limit, it seems pretty easy to hold thousands of buffer pins
> > with this.
> >
> > The index can set IndexScanDesc->xs_recheck to indicate that the quals
> > must be rechecked. Perhaps we should have a similar flag to indicate
> > that the visibility must be rechecked.

Added as xs_visrecheck in 0001.

> > Here's a completely different line of attack: Instead of holding buffer
> > pins for longer, what if we checked the visibility map earlier? We could
> > check the visibility map already when we construct the
> > GISTSearchHeapItem, and set a flag in IndexScanDesc to tell
> > IndexOnlyNext() that we have already done that. IndexOnlyNext() would
> > have three cases:
>
> I don't like integrating a heap-specific thing like VM_ALL_VISIBLE()
> to indexes, but given that IOS code already uses that exact code my
> dislike is not to the point of a -1. I'd like it better if we had a
> TableAM API for higher-level visibility checks (e.g.
> table_tids_could_be_invisible?()) which gives us those responses
> instead; dropping the requirement to maintain VM in pg's preferred
> format to support efficient IOS.

Here's a patchset that uses that approach. Naming of functions, types,
fields and arguments TBD. The patch works and passes the new
VACUUM-conflict tests, though I suspect the SP-GIST tests to have
bugs, as an intermediate version of my 0003 patch didn't trigger the
tests to fail, even though it did not hold a pin on (all) sorted
items' data when it was being checked for visibility and/or returned
from the scan.

Patch 0001 details the important changes, while 0002/0003 use this new
API to make GIST and SP-GIST's IOS work correctly when concurrent
VACUUM is/was running.
0004 is the existing patch with tests (v8-0001).

> With VM-checking in the index, we would potentially have another
> benefit: By checking all tids on the page at once, we can deduplicate
> and reduce the VM lookups. The gains might not be all that impressive,
> but could be significant in certain hot cases.

That is also included in this, but any performance impact hasn't been
tested nor validated.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)