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

On Thu, 9 Jan 2025 at 22:00, Michail Nikolaev
<michail(dot)nikolaev(at)gmail(dot)com> wrote:
>
> Hello.
>
> One thing I think we could add to the patches is to adapt the 10-years-old comment below with notice about IOS:
>
> /*
> * We save the LSN of the page as we read it, so that we know whether it
> * safe to apply LP_DEAD hints to the page later. This allows us to drop
> * the pin for MVCC scans, which allows vacuum to avoid blocking.
> */
> so->curPageLSN = BufferGetLSNAtomic(buffer);

I don't quite read it as covering IOS. To me, the comment is more
along the lines of (extensively extended):

"""
We'd like to use kill_prior_tuple, but that requires us to apply
changes to the page when we're already done with it for all intents
and purposes (because we scan the page once and buffer results). We
can either keep a pin on the buffer, or re-acquire that page after
finishing producing the tuples from this page.
Pinning the page blocks vacuum [^1], so instead we drop the pin, then
collect LP_DEAD marks, and then later we re-acquire the page to mark
the tuples dead. However, in the meantime the page may have changed;
by keeping a tab on changes in LSN we have a cheap method of detecting
changes in the page iself. [^2]
"""

... and that doesn't seem to cover much of IOS. MVCC index scans
aren't that special, practically every user query has MVCC. I think
this "MVCC scan" even means non-IOS scan, as I can't think of a reason
why dropping a pin would otherwise be a valid behaviour (see the this
thread's main issue).

[^1] Well, it should. In practice, the code in HEAD doesn't, but this
patchset fixes that disagreement.
[^2] If the page changed, i.e. the LSN changed, GIST accepts that it
can't use the collected LP_DEAD marks. We may be able to improve on
that (or not) by matching LP_DEAD offsets and TIDs with the on-page
TIDs, but that's far outside the scope of this patch; we'd first have
to build an understanding about why it's correct to assume vacuum
hasn't finished reaping old tuples and other sessions also finished
inserting new ones with the same TIDs in the meantime.

> Also, I think it is a good idea to add "Assert(!scan->xs_want_itup);" to gistkillitems.

Why would it be incorrect or invalid to kill items in an index-only scan?
If we hit the heap (due to ! VM_ALL_VISIBLE) and detected the heap
tuple was dead, why couldn't we mark it as dead in the index? IOS
assumes a high rate of all-visible pages, but it's hardly unheard of
to access pages with dead tuples.

Kind regards,

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