Re: GIN VACUUM can corrupt internal posting tree pages

On Sat, Aug 15, 2026 at 1:45 PM Andrey Borodin <x4mmm(at)yandex-team(dot)ru> wrote:
> The race also seems to have been introduced by fd83c83d0. Before that
> change this path used ginTraverseLock(), which explicitly rechecks
> GinPageIsLeaf() after the share-to-exclusive relock. Restoring that
> call should fix the race without changing the intended locking model.

Fortunately, I don't think this will be hard to fix: we just need to
retest if the page is still a leaf page. Same as everywhere else.

I just found another bug in the pending/list fastupdate=on path:
ginInsertCleanup is only called by the first pass through
ginbulkdelete. So there's an independent bug, with the same underlying
symptom (namely, GIN VACUUM can sometimes miss dead TIDs that it is
required to remove from the index).

Attached V2 has a second new patch that adds another isolation test
demonstrating the ginInsertCleanup bug (no real change to 0001 here).

Fortunately, this second bug also seems easy enough to fix: it looks
like we just need to consistently call ginInsertCleanup within
ginbulkdelete.

> This looks strikingly similar to BUG #16792 [0].

Yeah, I remember that whole saga. In fact, that was what spurred me to
look for bugs in this area.

> The current gin_index_check() would probably not detect the corruption
> shown by this test. In a nearby thread I propose index-all-keys-match,
> which probably could find this.

Right, gin_index_check is unlikely to catch either bug (it won't catch
them without verifying agreement with the heap, in whatever way).

It's particularly hard to test whether an index contains TIDs that
point to an LP_UNUSED item in the heap, because such a test is
inherently race-prone. We do at least manage to test for that in
passing during deletion of index tuples that were marked LP_DEAD, as
they're about to be deleted (see index_delete_check_htid). But GIN
doesn't support LP_DEAD marking index tuples at all, so it'll never
get even that limited coverage.

I don't think that bt_index_parent_check is capable of detecting when
a TID in an nbtree index points to an LP_UNUSED item, although I guess
it should be safe to add that check. Such a check would require
bt_index_parent_check to assume that there can't have been a
concurrent VACUUM race (it's not safe for bt_index_check to have such
a check, since it only takes an AccessShareLock). Ideally amcheck
would be able to thoroughly detect TID-points-to-LP_UNUSED corruption
in some way.

I assume that your index-all-keys-match can't detect LP_UNUSED
references from indexes, either. It could perhaps detect when an
LP_UNUSED item was recycled for another row with a distinct key to the
original dangling index tuple key, which is definitely an improvement.
But it's still not enough to make amcheck watertight. (Of course, I
understand that your patch is for GIN amcheck, not nbtree amcheck, but
the underlying principles are the same for both AMs.)

--
Peter Geoghegan