On Sat, Aug 15, 2026 at 1:45 PM Andrey Borodin <[email protected]> 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
From 6429f7d583be9999bfb524a4a9d6a27edbdf110c Mon Sep 17 00:00:00 2001 From: Peter Geoghegan <[email protected]> Date: Sat, 15 Aug 2026 13:24:11 -0400 Subject: [PATCH v2 2/4] Add an isolation test for the GIN pending list cleanup gap ginbulkdelete() cleans the GIN pending list from inside its "if (stats == NULL)" block, so it only does so on VACUUM's first index pass. The set of TIDs to delete is rebuilt for every pass, though, so a later pass can be handed a TID whose only index entry is still sitting in the pending list. That pass sweeps the entry tree and the posting trees, leaves the pending entry alone, and the heap pass then frees the line pointer underneath it. A later row recycles the line pointer, and an index scan returns rows a sequential scan does not. A tuple only has to become dead after the first pass to reach this. An aborted UPDATE does that at once: HeapTupleSatisfiesVacuum() reports HEAPTUPLE_DEAD for an aborted xmin whatever OldestXmin is, and the pending list entry aminsert already wrote is not rolled back with it. The test makes VACUUM need more than one index pass by leaving dead line pointers over more pages than a 64kB maintenance_work_mem can track, stops it at a new injection point once the first pass has cleaned the pending list, and rolls back an UPDATE of rows on pages the heap scan has not reached yet. A later pass collects those aborted row versions. Fresh rows then recycle the line pointers. Step scan_incomplete asserts that the heap scan really did stop short. Without it the test would go on passing if a change to the dead item store, or to the page count, ever left VACUUM with a single index pass and so stopped exercising anything. Both counts in the expected output are 0, which is what a ginbulkdelete() that cleans the pending list on every call produces; applied locally, that makes the test pass with no change to the expected output. As it stands the test fails, reporting 50 for via_index against via_heap's 0. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- src/backend/access/gin/ginvacuum.c | 2 + src/test/modules/injection_points/Makefile | 1 + .../expected/gin_pending_cleanup.out | 61 ++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/gin_pending_cleanup.spec | 106 ++++++++++++++++++ 5 files changed, 171 insertions(+) create mode 100644 src/test/modules/injection_points/expected/gin_pending_cleanup.out create mode 100644 src/test/modules/injection_points/specs/gin_pending_cleanup.spec diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c index 58a97e2d2..f5a143f37 100644 --- a/src/backend/access/gin/ginvacuum.c +++ b/src/backend/access/gin/ginvacuum.c @@ -648,6 +648,8 @@ ginbulkdelete(IndexVacuumInfo *info, IndexBulkDeleteResult *stats, false, true, stats); } + INJECTION_POINT("gin-bulkdelete-pending-cleaned", NULL); + /* we'll re-count the tuples each time */ stats->num_index_tuples = 0; gvs.result = stats; diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 54e2857c4..6efdc566b 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -13,6 +13,7 @@ REGRESS = injection_points hashagg reindex_conc vacuum REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress ISOLATION = basic \ + gin_pending_cleanup \ gin_vacuum_relock \ inplace \ reindex_concurrently_deferred \ diff --git a/src/test/modules/injection_points/expected/gin_pending_cleanup.out b/src/test/modules/injection_points/expected/gin_pending_cleanup.out new file mode 100644 index 000000000..009996784 --- /dev/null +++ b/src/test/modules/injection_points/expected/gin_pending_cleanup.out @@ -0,0 +1,61 @@ +Parsed test spec with 2 sessions + +starting permutation: vacuum_gin scan_incomplete ghost_begin ghost_update ghost_rollback release recycle count_via_index count_via_heap +injection_points_attach +----------------------- + +(1 row) + +step vacuum_gin: VACUUM (INDEX_CLEANUP ON) gin_pending; <waiting ...> +step scan_incomplete: + SELECT heap_blks_scanned < heap_blks_total AS scan_incomplete + FROM pg_stat_progress_vacuum WHERE relid = 'gin_pending'::regclass; + +scan_incomplete +--------------- +t +(1 row) + +step ghost_begin: BEGIN; +step ghost_update: UPDATE gin_pending SET tags = ARRAY['ghost'] WHERE id > 11900; +step ghost_rollback: ROLLBACK; +step release: + SELECT injection_points_detach('gin-bulkdelete-pending-cleaned'); + SELECT injection_points_wakeup('gin-bulkdelete-pending-cleaned'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step vacuum_gin: <... completed> +step recycle: INSERT INTO gin_pending + SELECT 900000 + g, ARRAY['recycled'] + FROM generate_series(1, 12000) g; +step count_via_index: + SET enable_seqscan = off; + SET enable_indexscan = on; + SET enable_bitmapscan = on; + SELECT count(*) AS via_index FROM gin_pending WHERE tags @> ARRAY['ghost']; + +via_index +--------- + 0 +(1 row) + +step count_via_heap: + SET enable_seqscan = on; + SET enable_indexscan = off; + SET enable_bitmapscan = off; + SELECT count(*) AS via_heap FROM gin_pending WHERE tags @> ARRAY['ghost']; + +via_heap +-------- + 0 +(1 row) + diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 69a12d3cc..bac87e8b6 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -44,6 +44,7 @@ tests += { 'isolation': { 'specs': [ 'basic', + 'gin_pending_cleanup', 'gin_vacuum_relock', 'inplace', 'reindex_concurrently_deferred', diff --git a/src/test/modules/injection_points/specs/gin_pending_cleanup.spec b/src/test/modules/injection_points/specs/gin_pending_cleanup.spec new file mode 100644 index 000000000..1b326095a --- /dev/null +++ b/src/test/modules/injection_points/specs/gin_pending_cleanup.spec @@ -0,0 +1,106 @@ +# ginbulkdelete() drained the GIN pending list only on VACUUM's first index +# pass, because the call sat inside its "if (stats == NULL)" block. The set of +# TIDs to delete is rebuilt for every pass, though, so a later pass could be +# handed a TID whose only index entry was still sitting in the pending list. +# That pass swept the entry tree and the posting trees, left the pending entry +# alone, and the heap pass then freed the line pointer underneath it. +# +# A tuple only has to become dead after the first pass to reach this. An +# aborted UPDATE does that at once: HeapTupleSatisfiesVacuum() reports +# HEAPTUPLE_DEAD for an aborted xmin whatever OldestXmin is, and the pending +# list entry aminsert already wrote is not rolled back with it. +# +# Both counts below must be 0. Without the fix the index keeps an entry for a +# line pointer that VACUUM freed, a later row recycles that line pointer, and +# via_index reports the recycled rows while via_heap reports none. Nothing +# repairs the index afterwards: VACUUM only removes TIDs it collected, and this +# one is LP_UNUSED by then. +# +# The setup deliberately makes VACUUM need several index passes: the dead line +# pointers cover far more pages than maintenance_work_mem can track. Step +# scan_incomplete asserts that this still holds, so that the test cannot go on +# passing once it has stopped reaching a second pass. + +setup +{ + CREATE EXTENSION injection_points; + + CREATE TABLE gin_pending (id int, tags text[]) + WITH (autovacuum_enabled = off); + INSERT INTO gin_pending + SELECT g, ARRAY['v' || g] FROM generate_series(1, 12000) g; + CREATE INDEX gin_pending_idx ON gin_pending USING gin (tags) + WITH (fastupdate = on, gin_pending_list_limit = 65536); + + -- Free space on the last pages, so that the aborted UPDATE below keeps its + -- new row version on the page the row is already on. + DELETE FROM gin_pending WHERE id > 11400 AND id % 2 = 0; +} + +# VACUUM cannot run alongside other statements in one setup block. INDEX_CLEANUP +# ON so that the bypass cannot skip freeing these line pointers, which is what +# leaves the free space the aborted UPDATE below relies on. +setup { VACUUM (INDEX_CLEANUP ON) gin_pending; } + +setup +{ + -- The dead line pointers the VACUUM below is supposed to remove. They + -- span more pages than maintenance_work_mem can hold at once. + DELETE FROM gin_pending WHERE id <= 11400 AND id % 4 <> 0; +} + +teardown +{ + DROP TABLE gin_pending; + DROP EXTENSION injection_points; +} + +session s_vacuum +setup +{ + SET maintenance_work_mem = '64kB'; + SELECT injection_points_set_local(); + SELECT injection_points_attach('gin-bulkdelete-pending-cleaned', 'wait'); +} +# Stops once the first index pass has drained the pending list. +step vacuum_gin { VACUUM (INDEX_CLEANUP ON) gin_pending; } +# Recycles the line pointers the VACUUM freed. +step recycle { INSERT INTO gin_pending + SELECT 900000 + g, ARRAY['recycled'] + FROM generate_series(1, 12000) g; } +step count_via_index +{ + SET enable_seqscan = off; + SET enable_indexscan = on; + SET enable_bitmapscan = on; + SELECT count(*) AS via_index FROM gin_pending WHERE tags @> ARRAY['ghost']; +} +step count_via_heap +{ + SET enable_seqscan = on; + SET enable_indexscan = off; + SET enable_bitmapscan = off; + SELECT count(*) AS via_heap FROM gin_pending WHERE tags @> ARRAY['ghost']; +} + +session s_writer +# The heap scan must still have pages left, or there is no second index pass +# and no page left holding the row the ghost below lands on. +step scan_incomplete +{ + SELECT heap_blks_scanned < heap_blks_total AS scan_incomplete + FROM pg_stat_progress_vacuum WHERE relid = 'gin_pending'::regclass; +} +step ghost_begin { BEGIN; } +# Leaves a dead row version, and a pending list entry for it, on a page the +# heap scan has not reached yet. +step ghost_update { UPDATE gin_pending SET tags = ARRAY['ghost'] WHERE id > 11900; } +step ghost_rollback { ROLLBACK; } +# Detach before waking, so the remaining index passes do not stop here again. +step release +{ + SELECT injection_points_detach('gin-bulkdelete-pending-cleaned'); + SELECT injection_points_wakeup('gin-bulkdelete-pending-cleaned'); +} + +permutation vacuum_gin scan_incomplete ghost_begin ghost_update ghost_rollback release recycle count_via_index count_via_heap -- 2.53.0
From f807038f25c54f33d5304d126baab14705ff0271 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan <[email protected]> Date: Fri, 14 Aug 2026 02:11:53 -0400 Subject: [PATCH v2 1/4] Add an isolation test for the GIN posting tree relock race ginVacuumPostingTreeLeaves() walks to the leftmost leaf of a posting tree, takes the root under a share lock, drops it, re-takes it in exclusive mode, and never re-checks GinPageIsLeaf(). A concurrent root split keeps the block number and rewrites the page in place: ginPlaceToPage() with a null stack->parent memcpys a freshly built internal page over the root, and GinInitPage built that page with GIN_LEAF cleared. So the page can be internal by the time the exclusive lock arrives. ginVacuumPostingTreeLeaf() then runs on an internal page, the rightlink sweep ends immediately, and the vacuum reports success without having visited a leaf. ginbulkdelete() goes on to free the heap line pointers whose index entries are still present. The other two GIN sites that do the same share-then-exclusive relock both re-check. ginTraverseLock() carries the comment "But root can become non-leaf during relock". ginbulkdelete()'s own entry-tree descent tests blkno == GIN_ROOT_BLKNO && !GinPageIsLeaf(page) and restarts. This site does neither. This adds an injection point in the window where no buffer lock is held, and an isolation test that drives the race deterministically: one session's VACUUM stops there, a second session splits the posting tree root, and the vacuum is then released onto a page that is no longer a leaf. On an assert-enabled build the test does not reach its counts. The first item decoded from the internal page is (0,0), and ginVacuumItemPointers() trips Assert(ItemPointerIsValid(pointer)): TRAP: failed Assert("ItemPointerIsValid(pointer)"), itemptr.h:105 ginVacuumItemPointers ginVacuumPostingTreeLeaf ginbulkdelete The backend dies and the rest of the isolation suite goes with it. On a build without asserts the vacuum runs to completion and the index is left disagreeing with the heap: measured separately, an index scan reports 14000 rows for a predicate where a sequential scan reports 13899. The expected output records correct behavior, so the test fails until the re-check is added rather than encoding the current behavior. Verified both ways: with a re-check applied locally the test passes with both counts at 13899, and without it the assertion above fires. The race window is a few instructions wide, which is why this needs an injection point rather than a plain isolation spec. Ordinary concurrent writers plus autovacuum reach it without any of this machinery. Introduced by fd83c83d0, which replaced the ginTraverseLock() call at this site with a hand-written share-then-exclusive pair. Reproduces on the 14, 18 and master tips; the source is byte-identical on 15, 16, 17 and 19. --- src/backend/access/gin/ginvacuum.c | 10 +++ src/test/modules/injection_points/Makefile | 1 + .../expected/gin_vacuum_relock.out | 52 +++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/gin_vacuum_relock.spec | 87 +++++++++++++++++++ 5 files changed, 151 insertions(+) create mode 100644 src/test/modules/injection_points/expected/gin_vacuum_relock.out create mode 100644 src/test/modules/injection_points/specs/gin_vacuum_relock.spec diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c index 040f21a92..58a97e2d2 100644 --- a/src/backend/access/gin/ginvacuum.c +++ b/src/backend/access/gin/ginvacuum.c @@ -23,6 +23,7 @@ #include "storage/lmgr.h" #include "storage/predicate.h" #include "storage/read_stream.h" +#include "utils/injection_point.h" #include "utils/memutils.h" struct GinVacuumState @@ -398,6 +399,15 @@ ginVacuumPostingTreeLeaves(GinVacuumState *gvs, BlockNumber blkno) if (GinPageIsLeaf(page)) { LockBuffer(buffer, GIN_UNLOCK); + + /* + * No buffer lock is held here, so a concurrent insert can split + * this page. A root split rewrites the page in place and clears + * GIN_LEAF, so the page may no longer be a leaf once the + * exclusive lock is acquired below. + */ + INJECTION_POINT("gin-vacuum-posting-tree-relock", NULL); + LockBuffer(buffer, GIN_EXCLUSIVE); break; } diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 25a3ddd89..54e2857c4 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -13,6 +13,7 @@ REGRESS = injection_points hashagg reindex_conc vacuum REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress ISOLATION = basic \ + gin_vacuum_relock \ inplace \ reindex_concurrently_deferred \ repack \ diff --git a/src/test/modules/injection_points/expected/gin_vacuum_relock.out b/src/test/modules/injection_points/expected/gin_vacuum_relock.out new file mode 100644 index 000000000..bfa5cd559 --- /dev/null +++ b/src/test/modules/injection_points/expected/gin_vacuum_relock.out @@ -0,0 +1,52 @@ +Parsed test spec with 2 sessions + +starting permutation: vacuum_gin split_root release recycle count_via_index count_via_heap +injection_points_attach +----------------------- + +(1 row) + +step vacuum_gin: VACUUM (INDEX_CLEANUP ON) gin_relock; <waiting ...> +step split_root: INSERT INTO gin_relock + SELECT g, ARRAY['public'] + FROM generate_series(20001, 28000) g; +step release: + SELECT injection_points_detach('gin-vacuum-posting-tree-relock'); + SELECT injection_points_wakeup('gin-vacuum-posting-tree-relock'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step vacuum_gin: <... completed> +step recycle: INSERT INTO gin_relock + SELECT 900000 + g, ARRAY['recycled'] + FROM generate_series(1, 400) g; +step count_via_index: + SET enable_seqscan = off; + SET enable_indexscan = on; + SET enable_bitmapscan = on; + SELECT count(*) AS via_index FROM gin_relock WHERE tags @> ARRAY['public']; + +via_index +--------- + 13899 +(1 row) + +step count_via_heap: + SET enable_seqscan = on; + SET enable_indexscan = off; + SET enable_bitmapscan = off; + SELECT count(*) AS via_heap FROM gin_relock WHERE tags @> ARRAY['public']; + +via_heap +-------- + 13899 +(1 row) + diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index aaf0536ba..69a12d3cc 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -44,6 +44,7 @@ tests += { 'isolation': { 'specs': [ 'basic', + 'gin_vacuum_relock', 'inplace', 'reindex_concurrently_deferred', 'repack', diff --git a/src/test/modules/injection_points/specs/gin_vacuum_relock.spec b/src/test/modules/injection_points/specs/gin_vacuum_relock.spec new file mode 100644 index 000000000..84d9d4711 --- /dev/null +++ b/src/test/modules/injection_points/specs/gin_vacuum_relock.spec @@ -0,0 +1,87 @@ +# GIN VACUUM drops its share lock on a posting tree root and re-takes it in +# exclusive mode without re-checking GinPageIsLeaf(). A concurrent root split +# rewrites the page in place with GIN_LEAF cleared, so the page can be internal +# by the time the exclusive lock arrives. ginVacuumPostingTreeLeaf() then runs +# on an internal page, the rightlink sweep ends at once, and the vacuum reports +# success without having visited a leaf. The heap pass frees the line pointers +# whose index entries are still present, later rows recycle them, and an index +# scan returns rows that a sequential scan over the same predicate does not. +# +# Both counts below must be 13899: 6000 rows loaded, 101 deleted, 8000 added. +# +# On an assert-enabled build this does not reach the counts. The first item +# decoded from the internal page is (0,0), and ginVacuumItemPointers() trips +# Assert(ItemPointerIsValid(pointer)), so the backend dies and the rest of the +# isolation suite goes with it. On a build without asserts the vacuum runs to +# completion and the counts diverge, via_index reporting 14000 against +# via_heap's 13899. +# +# The other two GIN sites doing this share-then-exclusive relock both re-check. +# ginTraverseLock() carries the comment "But root can become non-leaf during +# relock", and ginbulkdelete()'s own entry-tree descent tests +# blkno == GIN_ROOT_BLKNO && !GinPageIsLeaf(page) and restarts. + +setup +{ + CREATE EXTENSION injection_points; + + CREATE TABLE gin_relock (id int, tags text[]); + INSERT INTO gin_relock + SELECT g, ARRAY['public'] FROM generate_series(1, 6000) g; + CREATE INDEX gin_relock_idx ON gin_relock USING gin (tags) + WITH (fastupdate = off); + + -- The entries the VACUUM below is supposed to remove. + DELETE FROM gin_relock WHERE id BETWEEN 100 AND 200; +} + +teardown +{ + DROP TABLE gin_relock; + DROP EXTENSION injection_points; +} + +session s_vacuum +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('gin-vacuum-posting-tree-relock', 'wait'); +} +# Stops in the window where no lock is held on the posting tree root. +# INDEX_CLEANUP ON so that VACUUM cannot take the bypass it applies when few +# pages hold dead line pointers. The bypass skips index vacuuming altogether, +# which would stop this test reaching ginbulkdelete() at all. +step vacuum_gin { VACUUM (INDEX_CLEANUP ON) gin_relock; } +# Recycles the line pointers the VACUUM freed. +step recycle { INSERT INTO gin_relock + SELECT 900000 + g, ARRAY['recycled'] + FROM generate_series(1, 400) g; } +step count_via_index +{ + SET enable_seqscan = off; + SET enable_indexscan = on; + SET enable_bitmapscan = on; + SELECT count(*) AS via_index FROM gin_relock WHERE tags @> ARRAY['public']; +} +step count_via_heap +{ + SET enable_seqscan = on; + SET enable_indexscan = off; + SET enable_bitmapscan = off; + SELECT count(*) AS via_heap FROM gin_relock WHERE tags @> ARRAY['public']; +} + +session s_writer +# Splits the posting tree root while the VACUUM holds no lock on it. +step split_root { INSERT INTO gin_relock + SELECT g, ARRAY['public'] + FROM generate_series(20001, 28000) g; } +# Detach before waking, so that a VACUUM which correctly notices the page is no +# longer a leaf and restarts its descent does not stop here a second time. +step release +{ + SELECT injection_points_detach('gin-vacuum-posting-tree-relock'); + SELECT injection_points_wakeup('gin-vacuum-posting-tree-relock'); +} + +permutation vacuum_gin split_root release recycle count_via_index count_via_heap -- 2.53.0
