From 37f2ef91d334161e88ea0c27f446fc13a831d60c Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Fri, 14 Aug 2026 02:11:53 -0400 Subject: [PATCH 1/2] 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 | 84 +++++++++++++++++++ 5 files changed, 148 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..6c7c70edb --- /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 gin_relock; +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..cbd116e55 --- /dev/null +++ b/src/test/modules/injection_points/specs/gin_vacuum_relock.spec @@ -0,0 +1,84 @@ +# 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. +step vacuum_gin { VACUUM 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