Re: [SP-]GiST IOS visibility bug (was: Why doens't GiST require super-exclusive lock)

On Mon, Aug 3, 2026 at 6:40 AM Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)com> wrote:
>
> Attached v3, which fixes that, and hopefully also fixes the
> fallthrough compiler warning that cfbot has been reporting. Thanks for
> the report!

Haibo Yan, Surya Poondla, and I reviewed this patch as part of the
Patch Review Workshop.

Our understanding is that the four patches here (currently at v3) are
intended for backpatches only, and [0] holds the intended fix for
master. We have only reviewed the v3 backpatch patches.

The patches no longer apply cleanly. Here are v4 patches that apply to
REL_19_STABLE.

We can reproduce the problem (see gist-ios-vacuum.{spec,out},
attached). The patches fix it.

We don't think the final cleanup lock is required (although we left it
in v4 for now). If we drop it, the test above still passes, as do many
other stress tests we had Claude perform. Unlike nbtree, {SP-,}GiST
check the visibility map right away, while still holding a leaf-page
share lock. Also they drop the pin after that read, instead of holding
it for later. So (1) there is no pin to conflict with the cleanup lock
(2) we are already checking the VM safely: either vacuum removed the
tuple before our read and we never saw it, or vacuum removed the tuple
after our read (since it takes an exclusive lock on the page), so it
changes the VM after we checked it. The relevant ordering seems to be:

leaf SHARE lock
-> read index entry
-> check/cache visibility
-> release SHARE lock
-> VACUUM gets EXCLUSIVE lock
-> remove index entry
-> LP_DEAD -> LP_UNUSED / VM all-visible

Since a cleanup lock additionally waits for outstanding buffer pins,
it can introduce stalls that the previous ordinary EXCLUSIVE lock did
not. Our pgbench results were fairly noisy, especially for GiST, but
we observed slowdowns of up to 26%.

Another hint that the lock is not required here is that WAL redo and
replication don't take it.

@@ -381,6 +388,13 @@ spgrescan(IndexScanDesc scan, ScanKey scankey,
int nscankeys,
if (scankey && scan->numberOfKeys > 0)
memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));

+ /* prepare index-only scan requirements */
+ if (scan->xs_want_itup)
+ {
+ if (so->visrecheck == NULL)
+ so->visrecheck = palloc(MaxIndexTuplesPerPage);
+ }
+
/* initialize order-by data if needed */
if (orderbys && scan->numberOfOrderBys > 0)
{

This is allocated during rescan and, unlike the neighboring per-tuple
arrays, is not explicitly freed or reused as inline storage. Every
other similar field in SpGistScanOpaqueData is an inline fixed-size
array, so should this be one as well? Also, the struct is private, so
I think shifting the fields is acceptable, buf if not, we should add
this field (and the rest) to the end.

The comment on table_index_vischeck_tuple doesn't seem to match the
function. It is talking about checkop->checktids and TMVC_* constants.
Also the comment on TMVC_Result talks about checking multiple tuples
at once, but that seems to be leftover from somewhere else.

TMVC_Result needs to be added to typedefs.list.

xs_visrecheck is a somewhat misleading name. TMVC_MaybeVisible
actually means that a later VM lookup must not be done and that the
heap must be visited. So this is really a cached visibility result
rather than a "recheck" flag. Can we name it something like
xs_visresult (ideally on master too)? And we'd rather store a
TMVC_Result than an undocumented uint8.

SP-GiST also has an assertion that an IOS result has acquired a
visibility result before it is returned, while the corresponding GiST
path doesn't. Adding the same invariant assertion there seems
worthwhile.

RelationGetIndexScan initializes the other xs_* fields, but not
xs_visrecheck. I don't think there is a bug there, but we might as
well do it for consistency.

There are some testing gaps:

- In v3 we fixed the item->heapPtr bug, but no test fails if we undo that fix.
- The tests pass even with enable_indexonlyscan=false.
- Some mutations that lose queued KNN results can pass as well,
because the expected tail of the cursor is empty.

We think the tests should at least verify that an IOS was selected,
verify the intended VACUUM/visibility transition, exercise multiple
leaf pages/KNN state, and include a direct regression test for the
SP-GiST wrong-TID case.

[0] https://postgr.es/m/flat/CAH2-Wz=PqOziyRSrnN5jAtfXWXY7-BJcHz9S355LH8Dt=5qxWQ(at)mail(dot)gmail(dot)com

Yours,

--
Paul ~{:-)
pj(at)illuminatedcomputing(dot)com