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

On Sat, 4 Jan 2025 at 02:00, Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)com> wrote:
>
> On Tue, 3 Dec 2024 at 17:21, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> >
> > On Mon, Dec 2, 2024 at 8:18 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> > > Attached is a refined version of a test case I posted earlier on [2],
> > > decisively proving that GiST index-only scans are in fact subtly
> > > broken. Right now it fails, showing a wrong answer to a query. The
> > > patch adds an isolationtest test case to btree_gist, based on a test
> > > case of Andres'.
> >
> > I can confirm that the same bug affects SP-GiST. I modified the
> > original failing GiST isolation test to make it use SP-GiST instead,
> > proving what I already strongly suspected.
> >
> > I have no reason to believe that there are any similar problems in
> > core index AMs other than GiST and SP-GiST, though. Let's go through
> > them all now: nbtree already does everything correctly, and all
> > remaining core index AMs don't support index-only scans *and* don't
> > support scans that don't just use an MVCC snapshot.
>
> I think I have a fix for GiST which can be found attached in patch 0002.
>
> As to how this works: the patch tracks (for IOS) the pages for which
> there are some entries yet to be returned by gistgettuple(), and keeps
> a pin on those pages using a new AMscan tracking mechanism that
> utilizes buffer refcounts. Even if it might not a very elegant
> solution and IMV still has rough edges, it works, and fixes the issue
> with incorrect results from the GiST index.

In the attached v4 of the fix I've opted to replace the bespoke pin
tracker of my previous fix with the default buffer pin tracking
mechanism, which I realised has been designed for the same items and
doesn't require additional memory management on the AM side.
It massively simplifies the code, reduces allocation overhead, and
allowed me to port the fix to SP-GiST much quicker.

> I haven't looked much at SP-GiST yet, so I don't have anything for the
> VACUUM+IOS bug there.

I've attached a fix that uses the same approach as GiST in v4-0003. I
couldn't find any spgist extensions in contrib to copy-paste the tests
to, but manual testing did show vacuum now does wait for the index
scan to finish page processing.

> I'm looking forward to any feedback.

Like patch 0001, the status of that has not changed.

Kind regards,

Matthias van de Meent