GIN VACUUM can corrupt internal posting tree pages
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253423psql -h localhost -U postgresBuilt from patchset v6 (message #6), August 19, 2026 at 03:49 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253423_6 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253423_6 && git checkout t253423_6Patchset v6 (message #6) is on t253423_6
Opus 5 flagged a bug that can corrupt GIN indexes. All stable branches
are affected. It authored the attached patch, which adds a failing
isolation test demonstrating the problem. With assertions disabled,
the test shows a bitmap scan query that returns incorrect answers due
to this corruption (with asserts enabled, I get an
"Assert(ItemPointerIsValid(pointer))" failure instead).
ginVacuumPostingTreeLeaves() walks to the leftmost leaf of a posting
tree, takes the root under a share lock, drops it, and then re-takes
it in exclusive mode. Unlike similar code paths, it never re-checks
whether the page is still a leaf page after acquiring that exclusive
lock. It's therefore possible for VACUUM to treat an internal page as
a leaf page containing removable heap TIDs. I think that the specific
problem shown by the test case is a failure to visit all index pages
that might have TIDs that VACUUM is required to remove, to make it
safe to mark the line pointers LP_UNUSED in the heap. But the specific
scenario the test shows doesn't seem important; I imagine that this
oversight can have all kinds of unpleasant consequences.
--
Peter Geoghegan
Hi Peter,
Yes, I agree that this is real corruption.
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.
This looks strikingly similar to BUG #16792 [0]/messages/by-id/16792-b1913b6b4e098331@postgresql.org.
That report involved PostgreSQL 11 under heavy update and autovacuum
load. GIN scans returned rows that did not match the indexed value,
REINDEX fixed the problem temporarily, and it returned after some
weeks. Heikki eventually established that the index contained an
extra entry for an old key and developed a version of the GIN amcheck
code while investigating it, but AFAICS the underlying cause was not
found.
A stale posting-tree TID surviving VACUUM and later referring to a
recycled heap line pointer would explain the extra match in that report.
The old case also had the correct new key missing, so I cannot say that
it was necessarily the same bug. But the version, workload and symptoms
seem close enough to make the connection worth noting.
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.
Thanks for finding this!
Best regards, Andrey Borodin.
On Sat, Aug 15, 2026 at 1:45 PM Andrey Borodin <x4mmm@yandex-team.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
Attachments:
t253423_3v2-0002-Add-an-isolation-test-for-the-GIN-pending-list-cl.patchapplication/x-patch; name=v2-0002-Add-an-isolation-test-for-the-GIN-pending-list-cl.patchDownload+171-1
v2-0001-Add-an-isolation-test-for-the-GIN-posting-tree-re.patchapplication/x-patch; name=v2-0001-Add-an-isolation-test-for-the-GIN-posting-tree-re.patchDownload+151-1
index-all-keys-match can't detect LP_UNUSED references
index-all-keys-match does handle this partially.
bt_index_parent_check() version holds ShareLock and, when the MVCC fetch
finds nothing, inspects the heap line pointer and reports out-of-range
offsets and LP_UNUSED. bt_index_check() skips that because concurrent
VACUUM can remove the copied index entry and then reclaim its heap slot.
ShareLock still permits opportunistic pruning by SELECT. As far as I can
see, that can only make heap-only HOT chain members LP_UNUSED, while a valid
index TID points to the chain root. Do you see another pruning path that
would make this check unsafe under ShareLock?
It's not "watertight" of course. The inspection is reached only after a
Bloom filter miss, so a false positive can still hide a dangling TID.
without verifying agreement with the heap
More generally, I think many of the most valuable corruption checks are at
the boundaries between independently maintained representations: adjacent
or parent and child pages, an index and its heap, a relation and its
metadata, or heap visibility information and pg_xact. Local checks cannot
detect contradictions across such boundaries.
That is the direction in which I would like to extend amcheck.
consistently call ginInsertCleanup within ginbulkdelete
The pending-list fix also seems to be in the right place. Each
ginbulkdelete() pass has a different set of dead TIDs, so ginInsertCleanup()
has to run before every pass, not only when stats is initially allocated.
Doing it in ginvacuumcleanup() would be too late, after the relevant deletion
callback is gone.
Attached V2 has a second new patch
Are you planning to post fixes with these tests, or are the test patches
only intended as reproducers? I can prepare fixes along the lines you
outlined, but do not want to duplicate your work.
Best regards, Andrey Borodin.
On Sun, Aug 16, 2026 at 7:26 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
Attached V2 has a second new patch
Are you planning to post fixes with these tests, or are the test patches
only intended as reproducers? I can prepare fixes along the lines you
outlined, but do not want to duplicate your work.
Go ahead and post fixes.
--
Peter Geoghegan
On Mon, Aug 17, 2026 at 2:15 PM Peter Geoghegan <pg@bowt.ie> wrote:
Go ahead and post fixes.
I attach my own fixes for both bugs.
I couldn't resist the temptation to clean up some ginInsertCleanup
related comments in v1-0002-*. I've always found the distinction
between forceCleanup and full_clean confusing. I propose renaming
full_clean to must_empty_list (plus my comment fix ups) to make all
this less confusing.
2016 bug fix commit e2c79e14 seems related to my v1-0002-* patch:
"Previously, ginInsertCleanup could exit early if it detects that
someone else is cleaning up the pending list, without waiting for that
someone else to finish the job. But in this case vacuum could miss
tuples to be deleted."
But it missed that the existing call to ginInsertCleanup still only
happened on the first call to ginbulkdelete, which isn't necessarily
the only one. So I think that it makes sense to think of my v1-0002-*
as fixing an oversight in that 2016 commit. It's certainly a very
similar bug: both bugs involve ginInsertCleanup failing to force TIDs
into the main entry tree, making ginbulkdelete fail to reliably remove
all of the TIDs that VACUUM needs it to remove from the index.
--
Peter Geoghegan