public inbox for [email protected]help / color / mirror / Atom feed
Hash index bucket split bug 4+ messages / 2 participants [nested] [flat]
* Hash index bucket split bug @ 2026-07-08 20:39 Peter Geoghegan <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Peter Geoghegan @ 2026-07-08 20:39 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]>; +Cc: Amit Kapila <[email protected]>; Robert Haas <[email protected]> There's a bug in the logic for hash index scans: we fail to fully account for concurrent bucket splits. This can lead to incorrect results, as an affected scan may fail to return all matching rows. This is due to an oversight in commit 7c75ef57, so all stable branches are affected. The underlying problem is that _hash_next fails to step from one bucket of an in-progress to the other. In other words, it lacks the required transition logic that _hash_readnext already has (and _hash_readprev along with it). Note that the transition ordinarily happens anyway, inside _hash_readpage's no-matches loop, which is why this went unnoticed for so long: _hash_next only fails to make the transition when the last page it reads in the first bucket contains at least one matching tuple. In practice that means a tuple inserted into the bucket being populated after the split began. The attached bug fix patch v1-0001-* simply adds the missing handling to _hash_next (copied from _hash_readnext and _hash_readprev). I stumbled upon this bug when I asked Claude code to review a patch of mine that adds support for the new amgetbatch interface to the hash index AM (allowing hash indexes to perform I/O prefetching of heap blocks). I had Claude rewrite its original reproducer into a regression test and an isolation test, both of which appear in the second patch. Both tests are useful in their own way. The regression test runs quickly and so might be worth committing alongside the fix -- but it depends on leaving behind an incomplete split. The isolation test demonstrates that the bug doesn't hinge upon an incomplete split. The incomplete split case is particularly nasty, though, because the potential for wrong answers persists until an inserter completes the split. -- Peter Geoghegan Attachments: [application/octet-stream] v1-0001-Fix-hash-index-scans-concurrent-with-bucket-split.patch (2.7K, ../../CAH2-Wz=4CJK9ysZzbzxBGGTkgAg9ib8cJyCx=9q_d1r8tf0ang@mail.gmail.com/2-v1-0001-Fix-hash-index-scans-concurrent-with-bucket-split.patch) download | inline diff: From a25ac1014587bc29c9ee4f2d6406d25afbf759ee Mon Sep 17 00:00:00 2001 From: Peter Geoghegan <[email protected]> Date: Wed, 8 Jul 2026 14:18:05 -0400 Subject: [PATCH v1 1/2] Fix hash index scans concurrent with bucket splits. Oversight in commit 7c75ef57, which introduced page-at-a-time processing of hash index scans. --- src/backend/access/hash/hashsearch.c | 54 ++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/backend/access/hash/hashsearch.c b/src/backend/access/hash/hashsearch.c index 89d1c5bc6..7882a531f 100644 --- a/src/backend/access/hash/hashsearch.c +++ b/src/backend/access/hash/hashsearch.c @@ -75,6 +75,28 @@ _hash_next(IndexScanDesc scan, ScanDirection dir) if (!_hash_readpage(scan, &buf, dir)) end_of_scan = true; } + else if (so->hashso_buc_populated && !so->hashso_buc_split) + { + /* + * end of bucket being populated: continue the scan in the + * bucket being split, on whose primary page we have held a + * pin since _hash_first + */ + buf = so->hashso_split_bucket_buf; + Assert(BufferIsValid(buf)); + LockBuffer(buf, BUFFER_LOCK_SHARE); + PredicateLockPage(rel, BufferGetBlockNumber(buf), + scan->xs_snapshot); + + /* + * setting hashso_buc_split to true indicates that we are + * scanning bucket being split. + */ + so->hashso_buc_split = true; + + if (!_hash_readpage(scan, &buf, dir)) + end_of_scan = true; + } else end_of_scan = true; } @@ -104,6 +126,38 @@ _hash_next(IndexScanDesc scan, ScanDirection dir) if (!_hash_readpage(scan, &buf, dir)) end_of_scan = true; } + else if (so->hashso_buc_populated && so->hashso_buc_split) + { + Page page; + HashPageOpaque opaque; + + /* + * start of bucket being split: continue the scan from the + * last page in the chain of the bucket being populated, on + * whose primary page we have held a pin since _hash_first + */ + buf = so->hashso_bucket_buf; + Assert(BufferIsValid(buf)); + LockBuffer(buf, BUFFER_LOCK_SHARE); + page = BufferGetPage(buf); + opaque = HashPageGetOpaque(page); + + /* move to the end of bucket chain */ + while (BlockNumberIsValid(opaque->hasho_nextblkno)) + _hash_readnext(scan, &buf, &page, &opaque); + + /* + * setting hashso_buc_split to false indicates that we are + * scanning the bucket being populated. Only set it after + * the chain walk above; otherwise _hash_readnext would + * advance to the bucket being split on reaching the end of + * the chain, instead of stopping there. + */ + so->hashso_buc_split = false; + + if (!_hash_readpage(scan, &buf, dir)) + end_of_scan = true; + } else end_of_scan = true; } -- 2.53.0 [application/octet-stream] v1-0002-Add-tests-for-hash-index-scans-concurrent-with-bu.patch (16.9K, ../../CAH2-Wz=4CJK9ysZzbzxBGGTkgAg9ib8cJyCx=9q_d1r8tf0ang@mail.gmail.com/3-v1-0002-Add-tests-for-hash-index-scans-concurrent-with-bu.patch) download | inline diff: From 3131a69bd8b7fffa450795fae573ee2fb8c47288 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan <[email protected]> Date: Wed, 8 Jul 2026 15:18:06 -0400 Subject: [PATCH v1 2/2] Add tests for hash index scans concurrent with bucket splits. Test the bug fixed by the preceding commit at two levels, using two new injection points in _hash_splitbucket: an error-mode point placed before tuple relocation begins (where existing code can fail anyway), and a wait-mode point placed after tuple relocation, where the splitter holds no buffer content locks. A regress test uses the error-mode point to interrupt a split partway through, then verifies that forward, backward, and bitmap scans of the affected bucket pair all agree with a seqscan. The bug isn't specific to interrupted splits: interrupting a split is just the most convenient way to hold the bucket pair in the same state that scans observe while any split is in progress, making the test deterministic without a second session. An isolation test proves that claim directly: it pauses a splitter on the wait-mode point and runs the same scans from a second session while the split remains genuinely in progress, then lets the split finish and scans once more. Co-Authored-By: Claude Fable 5 <[email protected]> --- src/backend/access/hash/hashpage.c | 5 + src/test/modules/injection_points/Makefile | 3 +- .../injection_points/expected/hash-split.out | 70 ++++++++++ .../injection_points/expected/hash_split.out | 131 ++++++++++++++++++ src/test/modules/injection_points/meson.build | 2 + .../injection_points/specs/hash-split.spec | 81 +++++++++++ .../injection_points/sql/hash_split.sql | 80 +++++++++++ 7 files changed, 371 insertions(+), 1 deletion(-) create mode 100644 src/test/modules/injection_points/expected/hash-split.out create mode 100644 src/test/modules/injection_points/expected/hash_split.out create mode 100644 src/test/modules/injection_points/specs/hash-split.spec create mode 100644 src/test/modules/injection_points/sql/hash_split.sql diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c index 8099b0d02..c2e7213c4 100644 --- a/src/backend/access/hash/hashpage.c +++ b/src/backend/access/hash/hashpage.c @@ -35,6 +35,7 @@ #include "port/pg_bitutils.h" #include "storage/predicate.h" #include "storage/smgr.h" +#include "utils/injection_point.h" #include "utils/rel.h" static bool _hash_alloc_buckets(Relation rel, BlockNumber firstblock, @@ -1104,6 +1105,8 @@ _hash_splitbucket(Relation rel, npage = BufferGetPage(nbuf); nopaque = HashPageGetOpaque(npage); + INJECTION_POINT("hash-split-before-relocation", NULL); + /* Copy the predicate locks from old bucket to new bucket. */ PredicateLockPageSplit(rel, BufferGetBlockNumber(bucket_obuf), @@ -1252,6 +1255,8 @@ _hash_splitbucket(Relation rel, /* be tidy */ for (i = 0; i < nitups; i++) pfree(itups[i]); + + INJECTION_POINT("hash-split-after-relocation", NULL); break; } diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index c01d2fb09..23e00207b 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -9,10 +9,11 @@ EXTENSION = injection_points DATA = injection_points--1.0.sql PGFILEDESC = "injection_points - facility for injection points" -REGRESS = injection_points hashagg reindex_conc vacuum +REGRESS = injection_points hashagg reindex_conc vacuum hash_split REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress ISOLATION = basic \ + hash-split \ inplace \ repack \ repack_temporal \ diff --git a/src/test/modules/injection_points/expected/hash-split.out b/src/test/modules/injection_points/expected/hash-split.out new file mode 100644 index 000000000..d087a2c69 --- /dev/null +++ b/src/test/modules/injection_points/expected/hash-split.out @@ -0,0 +1,70 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_split s2_insert s2_scan s2_back s2_detach s2_wakeup s2_scan +injection_points_attach +----------------------- + +(1 row) + +step s1_split: + INSERT INTO hash_split_test + SELECT 1000000 + g FROM hash_split_geom, generate_series(1, 320 * nbuckets) g; + <waiting ...> +step s2_insert: + INSERT INTO hash_split_test SELECT k FROM hash_split_key; + +step s2_scan: + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + +count +----- + 11 +(1 row) + +step s2_back: + BEGIN; + DECLARE c SCROLL CURSOR FOR + SELECT v = (SELECT k FROM hash_split_key) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + MOVE FORWARD ALL IN c; + FETCH BACKWARD ALL FROM c; + COMMIT; + +?column? +-------- +t +t +t +t +t +t +t +t +t +t +t +(11 rows) + +step s2_detach: SELECT injection_points_detach('hash-split-after-relocation'); +injection_points_detach +----------------------- + +(1 row) + +step s2_wakeup: SELECT injection_points_wakeup('hash-split-after-relocation'); +injection_points_wakeup +----------------------- + +(1 row) + +step s1_split: <... completed> +step s2_scan: + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + +count +----- + 11 +(1 row) + diff --git a/src/test/modules/injection_points/expected/hash_split.out b/src/test/modules/injection_points/expected/hash_split.out new file mode 100644 index 000000000..0754618c2 --- /dev/null +++ b/src/test/modules/injection_points/expected/hash_split.out @@ -0,0 +1,131 @@ +-- Test hash index scans of a bucket pair whose split is incomplete +-- +-- The behavior tested here is not specific to incomplete splits: it +-- applies to any scan that runs while the bucket pair's split-in-progress +-- flags are set, which is the case for the duration of every split. +-- Interrupting a split is merely the most convenient way to hold the +-- bucket pair in that state: it makes the tests below deterministic, +-- without any need for a second session. The hash-split isolation test +-- provides equivalent coverage for scans that run during a live split. +CREATE EXTENSION injection_points; +SELECT injection_points_set_local(); + injection_points_set_local +---------------------------- + +(1 row) + +CREATE TABLE hash_split_test (v int4) WITH (autovacuum_enabled = false); +CREATE INDEX hash_split_index ON hash_split_test USING hash (v); +-- Determine the layout of the (never-split) index: one metapage, nbuckets +-- bucket pages, and one bitmap page. The index's first bucket split will +-- split bucket 0, creating bucket nbuckets. +CREATE TEMP TABLE hash_split_geom AS + SELECT pg_relation_size('hash_split_index') / + current_setting('block_size')::bigint - 2 AS nbuckets; +-- sanity: initial bucket counts are always powers of two +SELECT nbuckets > 0 AND (nbuckets & (nbuckets - 1)) = 0 AS nbuckets_ok + FROM hash_split_geom; + nbuckets_ok +------------- + t +(1 row) + +-- Choose a value that maps to bucket 0 before the split and to the new +-- bucket afterwards +CREATE TEMP TABLE hash_split_key AS + SELECT min(v)::int4 AS k + FROM generate_series(1, 10000) v, hash_split_geom + WHERE (hashint4(v::int4) & (2 * nbuckets - 1)) = nbuckets; +INSERT INTO hash_split_test + SELECT k FROM hash_split_key, generate_series(1, 10); +-- Error out during the first bucket split, leaving it incomplete +SELECT injection_points_attach('hash-split-before-relocation', 'error'); + injection_points_attach +------------------------- + +(1 row) + +INSERT INTO hash_split_test + SELECT 1000000 + g FROM hash_split_geom, generate_series(1, 350 * nbuckets) g; +ERROR: error triggered for injection point hash-split-before-relocation +SELECT injection_points_detach('hash-split-before-relocation'); + injection_points_detach +------------------------- + +(1 row) + +-- This insertion goes to the new bucket, which is still flagged as being +-- populated by the incomplete split +INSERT INTO hash_split_test SELECT k FROM hash_split_key; +-- Scans of that bucket must visit both buckets of the incomplete split: +-- the fresh row from the new bucket, plus the 10 older rows that remain +-- in bucket 0 +SET enable_seqscan = off; +SET enable_bitmapscan = off; +EXPLAIN (COSTS OFF) + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + QUERY PLAN +------------------------------------------------------------ + Aggregate + InitPlan expr_1 + -> Seq Scan on hash_split_key + Disabled: true + -> Index Scan using hash_split_index on hash_split_test + Index Cond: (v = (InitPlan expr_1).col1) +(6 rows) + +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + count +------- + 11 +(1 row) + +-- Backward scans must visit both buckets too, in the opposite order +BEGIN; +DECLARE c SCROLL CURSOR FOR + SELECT v = (SELECT k FROM hash_split_key) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +MOVE FORWARD ALL IN c; +FETCH BACKWARD ALL FROM c; + ?column? +---------- + t + t + t + t + t + t + t + t + t + t + t +(11 rows) + +COMMIT; +-- Same count via a bitmap scan, which shares the underlying scan code +SET enable_indexscan = off; +SET enable_bitmapscan = on; +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + count +------- + 11 +(1 row) + +-- And the ground truth, via the heap +SET enable_bitmapscan = off; +RESET enable_seqscan; +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + count +------- + 11 +(1 row) + +RESET enable_indexscan; +RESET enable_bitmapscan; +DROP TABLE hash_split_test; +DROP EXTENSION injection_points; diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 59dba1cb0..a0b0c4827 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -36,6 +36,7 @@ tests += { 'hashagg', 'reindex_conc', 'vacuum', + 'hash_split', ], 'regress_args': ['--dlpath', meson.project_build_root() / 'src/test/regress'], # The injection points are cluster-wide, so disable installcheck @@ -44,6 +45,7 @@ tests += { 'isolation': { 'specs': [ 'basic', + 'hash-split', 'inplace', 'repack', 'repack_temporal', diff --git a/src/test/modules/injection_points/specs/hash-split.spec b/src/test/modules/injection_points/specs/hash-split.spec new file mode 100644 index 000000000..cfdc0d944 --- /dev/null +++ b/src/test/modules/injection_points/specs/hash-split.spec @@ -0,0 +1,81 @@ +# Test hash index scans that run while a bucket split is in progress. +# +# The interesting window opens once the split has relocated tuples to the +# new bucket, before the buckets' split-in-progress flags are cleared. A +# concurrent scan whose value belongs to the new bucket must visit both +# buckets of the pair: it skips the moved-by-split tuples in the new +# bucket, so it has to read their authoritative copies from the old +# bucket, plus any tuples inserted into the new bucket after the split +# began. +# +# s1 performs the split, pausing inside that window on a wait injection +# point (placed where the splitter holds no buffer content locks). s2 +# then inserts a matching row (which goes to the new bucket) and scans, +# both forwards and backwards. + +setup +{ + CREATE EXTENSION injection_points; + CREATE TABLE hash_split_test (v int4) WITH (autovacuum_enabled = false); + CREATE INDEX hash_split_index ON hash_split_test USING hash (v); + CREATE TABLE hash_split_geom AS + SELECT pg_relation_size('hash_split_index') / + current_setting('block_size')::bigint - 2 AS nbuckets; + CREATE TABLE hash_split_key AS + SELECT min(v)::int4 AS k + FROM generate_series(1, 10000) v, hash_split_geom + WHERE (hashint4(v::int4) & (2 * nbuckets - 1)) = nbuckets; + INSERT INTO hash_split_test + SELECT k FROM hash_split_key, generate_series(1, 10); +} + +teardown +{ + DROP TABLE hash_split_test, hash_split_geom, hash_split_key; + DROP EXTENSION injection_points; +} + +session s1 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('hash-split-after-relocation', 'wait'); +} +step s1_split +{ + INSERT INTO hash_split_test + SELECT 1000000 + g FROM hash_split_geom, generate_series(1, 320 * nbuckets) g; +} + +session s2 +setup +{ + SET enable_seqscan = off; + SET enable_bitmapscan = off; +} +step s2_insert +{ + INSERT INTO hash_split_test SELECT k FROM hash_split_key; +} +step s2_scan +{ + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +} +step s2_back +{ + BEGIN; + DECLARE c SCROLL CURSOR FOR + SELECT v = (SELECT k FROM hash_split_key) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + MOVE FORWARD ALL IN c; + FETCH BACKWARD ALL FROM c; + COMMIT; +} +step s2_detach { SELECT injection_points_detach('hash-split-after-relocation'); } +step s2_wakeup { SELECT injection_points_wakeup('hash-split-after-relocation'); } + +# Scan during the paused split, then let the split finish and scan again. +# The detach must happen before the wakeup: the same INSERT can trigger +# another split, which must not hit the injection point again. +permutation s1_split s2_insert s2_scan s2_back s2_detach s2_wakeup s2_scan diff --git a/src/test/modules/injection_points/sql/hash_split.sql b/src/test/modules/injection_points/sql/hash_split.sql new file mode 100644 index 000000000..010b65882 --- /dev/null +++ b/src/test/modules/injection_points/sql/hash_split.sql @@ -0,0 +1,80 @@ +-- Test hash index scans of a bucket pair whose split is incomplete +-- +-- The behavior tested here is not specific to incomplete splits: it +-- applies to any scan that runs while the bucket pair's split-in-progress +-- flags are set, which is the case for the duration of every split. +-- Interrupting a split is merely the most convenient way to hold the +-- bucket pair in that state: it makes the tests below deterministic, +-- without any need for a second session. The hash-split isolation test +-- provides equivalent coverage for scans that run during a live split. +CREATE EXTENSION injection_points; +SELECT injection_points_set_local(); + +CREATE TABLE hash_split_test (v int4) WITH (autovacuum_enabled = false); +CREATE INDEX hash_split_index ON hash_split_test USING hash (v); + +-- Determine the layout of the (never-split) index: one metapage, nbuckets +-- bucket pages, and one bitmap page. The index's first bucket split will +-- split bucket 0, creating bucket nbuckets. +CREATE TEMP TABLE hash_split_geom AS + SELECT pg_relation_size('hash_split_index') / + current_setting('block_size')::bigint - 2 AS nbuckets; +-- sanity: initial bucket counts are always powers of two +SELECT nbuckets > 0 AND (nbuckets & (nbuckets - 1)) = 0 AS nbuckets_ok + FROM hash_split_geom; + +-- Choose a value that maps to bucket 0 before the split and to the new +-- bucket afterwards +CREATE TEMP TABLE hash_split_key AS + SELECT min(v)::int4 AS k + FROM generate_series(1, 10000) v, hash_split_geom + WHERE (hashint4(v::int4) & (2 * nbuckets - 1)) = nbuckets; + +INSERT INTO hash_split_test + SELECT k FROM hash_split_key, generate_series(1, 10); + +-- Error out during the first bucket split, leaving it incomplete +SELECT injection_points_attach('hash-split-before-relocation', 'error'); +INSERT INTO hash_split_test + SELECT 1000000 + g FROM hash_split_geom, generate_series(1, 350 * nbuckets) g; +SELECT injection_points_detach('hash-split-before-relocation'); + +-- This insertion goes to the new bucket, which is still flagged as being +-- populated by the incomplete split +INSERT INTO hash_split_test SELECT k FROM hash_split_key; + +-- Scans of that bucket must visit both buckets of the incomplete split: +-- the fresh row from the new bucket, plus the 10 older rows that remain +-- in bucket 0 +SET enable_seqscan = off; +SET enable_bitmapscan = off; +EXPLAIN (COSTS OFF) + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + +-- Backward scans must visit both buckets too, in the opposite order +BEGIN; +DECLARE c SCROLL CURSOR FOR + SELECT v = (SELECT k FROM hash_split_key) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +MOVE FORWARD ALL IN c; +FETCH BACKWARD ALL FROM c; +COMMIT; + +-- Same count via a bitmap scan, which shares the underlying scan code +SET enable_indexscan = off; +SET enable_bitmapscan = on; +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +-- And the ground truth, via the heap +SET enable_bitmapscan = off; +RESET enable_seqscan; +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +RESET enable_indexscan; +RESET enable_bitmapscan; + +DROP TABLE hash_split_test; +DROP EXTENSION injection_points; -- 2.53.0 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Hash index bucket split bug @ 2026-07-09 10:45 Amit Kapila <[email protected]> parent: Peter Geoghegan <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Amit Kapila @ 2026-07-09 10:45 UTC (permalink / raw) To: Peter Geoghegan <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Robert Haas <[email protected]> On Thu, Jul 9, 2026 at 2:10 AM Peter Geoghegan <[email protected]> wrote: > > Both tests are useful in their own way. The regression test runs > quickly and so might be worth committing alongside the fix -- but it > depends on leaving behind an incomplete split. The isolation test > demonstrates that the bug doesn't hinge upon an incomplete split. The > incomplete split case is particularly nasty, though, because the > potential for wrong answers persists until an inserter completes the > split. > I am getting following regression diff even after applying the patch: -step s1_split: <... completed> step s2_scan: SELECT count(*) FROM hash_split_test WHERE v = (SELECT k FROM hash_split_key); @@ -68,3 +67,4 @@ 11 (1 row) I will review the code later. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Hash index bucket split bug @ 2026-07-09 15:24 Peter Geoghegan <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Peter Geoghegan @ 2026-07-09 15:24 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Robert Haas <[email protected]> On Thu, Jul 9, 2026 at 6:45 AM Amit Kapila <[email protected]> wrote: > I am getting following regression diff even after applying the patch: > -step s1_split: <... completed> > step s2_scan: > SELECT count(*) FROM hash_split_test > WHERE v = (SELECT k FROM hash_split_key); > @@ -68,3 +67,4 @@ > 11 > (1 row) This is just a minor test flakiness issue with the isolation test. Apparently, we need to force a deterministic report position by adding a no-op step to the splitter's session. Attached v2 has a more stable version of the isolation test (no other changes compared to V1). Does this version work for you? The isolation test is just for illustrative purposes. I don't think that it's committable. -- Peter Geoghegan Attachments: [application/octet-stream] v2-0001-Fix-hash-index-scans-concurrent-with-bucket-split.patch (2.7K, ../../CAH2-WznKyrDzTA+wvBYw0zbUe9f2JyUOG-Jm368Cs03sgEB_1Q@mail.gmail.com/2-v2-0001-Fix-hash-index-scans-concurrent-with-bucket-split.patch) download | inline diff: From a25ac1014587bc29c9ee4f2d6406d25afbf759ee Mon Sep 17 00:00:00 2001 From: Peter Geoghegan <[email protected]> Date: Wed, 8 Jul 2026 14:18:05 -0400 Subject: [PATCH v2 1/2] Fix hash index scans concurrent with bucket splits. Oversight in commit 7c75ef57, which introduced page-at-a-time processing of hash index scans. --- src/backend/access/hash/hashsearch.c | 54 ++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/backend/access/hash/hashsearch.c b/src/backend/access/hash/hashsearch.c index 89d1c5bc6..7882a531f 100644 --- a/src/backend/access/hash/hashsearch.c +++ b/src/backend/access/hash/hashsearch.c @@ -75,6 +75,28 @@ _hash_next(IndexScanDesc scan, ScanDirection dir) if (!_hash_readpage(scan, &buf, dir)) end_of_scan = true; } + else if (so->hashso_buc_populated && !so->hashso_buc_split) + { + /* + * end of bucket being populated: continue the scan in the + * bucket being split, on whose primary page we have held a + * pin since _hash_first + */ + buf = so->hashso_split_bucket_buf; + Assert(BufferIsValid(buf)); + LockBuffer(buf, BUFFER_LOCK_SHARE); + PredicateLockPage(rel, BufferGetBlockNumber(buf), + scan->xs_snapshot); + + /* + * setting hashso_buc_split to true indicates that we are + * scanning bucket being split. + */ + so->hashso_buc_split = true; + + if (!_hash_readpage(scan, &buf, dir)) + end_of_scan = true; + } else end_of_scan = true; } @@ -104,6 +126,38 @@ _hash_next(IndexScanDesc scan, ScanDirection dir) if (!_hash_readpage(scan, &buf, dir)) end_of_scan = true; } + else if (so->hashso_buc_populated && so->hashso_buc_split) + { + Page page; + HashPageOpaque opaque; + + /* + * start of bucket being split: continue the scan from the + * last page in the chain of the bucket being populated, on + * whose primary page we have held a pin since _hash_first + */ + buf = so->hashso_bucket_buf; + Assert(BufferIsValid(buf)); + LockBuffer(buf, BUFFER_LOCK_SHARE); + page = BufferGetPage(buf); + opaque = HashPageGetOpaque(page); + + /* move to the end of bucket chain */ + while (BlockNumberIsValid(opaque->hasho_nextblkno)) + _hash_readnext(scan, &buf, &page, &opaque); + + /* + * setting hashso_buc_split to false indicates that we are + * scanning the bucket being populated. Only set it after + * the chain walk above; otherwise _hash_readnext would + * advance to the bucket being split on reaching the end of + * the chain, instead of stopping there. + */ + so->hashso_buc_split = false; + + if (!_hash_readpage(scan, &buf, dir)) + end_of_scan = true; + } else end_of_scan = true; } -- 2.53.0 [application/octet-stream] v2-0002-Add-tests-for-hash-index-scans-concurrent-with-bu.patch (17.2K, ../../CAH2-WznKyrDzTA+wvBYw0zbUe9f2JyUOG-Jm368Cs03sgEB_1Q@mail.gmail.com/3-v2-0002-Add-tests-for-hash-index-scans-concurrent-with-bu.patch) download | inline diff: From 86090e0249806587125b781fab537b5cc086d3d8 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan <[email protected]> Date: Wed, 8 Jul 2026 15:18:06 -0400 Subject: [PATCH v2 2/2] Add tests for hash index scans concurrent with bucket splits. Test the bug fixed by the preceding commit at two levels, using two new injection points in _hash_splitbucket: an error-mode point placed before tuple relocation begins (where existing code can fail anyway), and a wait-mode point placed after tuple relocation, where the splitter holds no buffer content locks. A regress test uses the error-mode point to interrupt a split partway through, then verifies that forward, backward, and bitmap scans of the affected bucket pair all agree with a seqscan. The bug isn't specific to interrupted splits: interrupting a split is just the most convenient way to hold the bucket pair in the same state that scans observe while any split is in progress, making the test deterministic without a second session. An isolation test proves that claim directly: it pauses a splitter on the wait-mode point and runs the same scans from a second session while the split remains genuinely in progress, then lets the split finish and scans once more. Co-Authored-By: Claude Fable 5 <[email protected]> --- src/backend/access/hash/hashpage.c | 5 + src/test/modules/injection_points/Makefile | 3 +- .../injection_points/expected/hash-split.out | 71 ++++++++++ .../injection_points/expected/hash_split.out | 131 ++++++++++++++++++ src/test/modules/injection_points/meson.build | 2 + .../injection_points/specs/hash-split.spec | 85 ++++++++++++ .../injection_points/sql/hash_split.sql | 80 +++++++++++ 7 files changed, 376 insertions(+), 1 deletion(-) create mode 100644 src/test/modules/injection_points/expected/hash-split.out create mode 100644 src/test/modules/injection_points/expected/hash_split.out create mode 100644 src/test/modules/injection_points/specs/hash-split.spec create mode 100644 src/test/modules/injection_points/sql/hash_split.sql diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c index 8099b0d02..c2e7213c4 100644 --- a/src/backend/access/hash/hashpage.c +++ b/src/backend/access/hash/hashpage.c @@ -35,6 +35,7 @@ #include "port/pg_bitutils.h" #include "storage/predicate.h" #include "storage/smgr.h" +#include "utils/injection_point.h" #include "utils/rel.h" static bool _hash_alloc_buckets(Relation rel, BlockNumber firstblock, @@ -1104,6 +1105,8 @@ _hash_splitbucket(Relation rel, npage = BufferGetPage(nbuf); nopaque = HashPageGetOpaque(npage); + INJECTION_POINT("hash-split-before-relocation", NULL); + /* Copy the predicate locks from old bucket to new bucket. */ PredicateLockPageSplit(rel, BufferGetBlockNumber(bucket_obuf), @@ -1252,6 +1255,8 @@ _hash_splitbucket(Relation rel, /* be tidy */ for (i = 0; i < nitups; i++) pfree(itups[i]); + + INJECTION_POINT("hash-split-after-relocation", NULL); break; } diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index c01d2fb09..23e00207b 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -9,10 +9,11 @@ EXTENSION = injection_points DATA = injection_points--1.0.sql PGFILEDESC = "injection_points - facility for injection points" -REGRESS = injection_points hashagg reindex_conc vacuum +REGRESS = injection_points hashagg reindex_conc vacuum hash_split REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress ISOLATION = basic \ + hash-split \ inplace \ repack \ repack_temporal \ diff --git a/src/test/modules/injection_points/expected/hash-split.out b/src/test/modules/injection_points/expected/hash-split.out new file mode 100644 index 000000000..49620b79c --- /dev/null +++ b/src/test/modules/injection_points/expected/hash-split.out @@ -0,0 +1,71 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_split s2_insert s2_scan s2_back s2_detach s2_wakeup s1_noop s2_scan +injection_points_attach +----------------------- + +(1 row) + +step s1_split: + INSERT INTO hash_split_test + SELECT 1000000 + g FROM hash_split_geom, generate_series(1, 320 * nbuckets) g; + <waiting ...> +step s2_insert: + INSERT INTO hash_split_test SELECT k FROM hash_split_key; + +step s2_scan: + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + +count +----- + 11 +(1 row) + +step s2_back: + BEGIN; + DECLARE c SCROLL CURSOR FOR + SELECT v = (SELECT k FROM hash_split_key) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + MOVE FORWARD ALL IN c; + FETCH BACKWARD ALL FROM c; + COMMIT; + +?column? +-------- +t +t +t +t +t +t +t +t +t +t +t +(11 rows) + +step s2_detach: SELECT injection_points_detach('hash-split-after-relocation'); +injection_points_detach +----------------------- + +(1 row) + +step s2_wakeup: SELECT injection_points_wakeup('hash-split-after-relocation'); +injection_points_wakeup +----------------------- + +(1 row) + +step s1_split: <... completed> +step s1_noop: +step s2_scan: + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + +count +----- + 11 +(1 row) + diff --git a/src/test/modules/injection_points/expected/hash_split.out b/src/test/modules/injection_points/expected/hash_split.out new file mode 100644 index 000000000..0754618c2 --- /dev/null +++ b/src/test/modules/injection_points/expected/hash_split.out @@ -0,0 +1,131 @@ +-- Test hash index scans of a bucket pair whose split is incomplete +-- +-- The behavior tested here is not specific to incomplete splits: it +-- applies to any scan that runs while the bucket pair's split-in-progress +-- flags are set, which is the case for the duration of every split. +-- Interrupting a split is merely the most convenient way to hold the +-- bucket pair in that state: it makes the tests below deterministic, +-- without any need for a second session. The hash-split isolation test +-- provides equivalent coverage for scans that run during a live split. +CREATE EXTENSION injection_points; +SELECT injection_points_set_local(); + injection_points_set_local +---------------------------- + +(1 row) + +CREATE TABLE hash_split_test (v int4) WITH (autovacuum_enabled = false); +CREATE INDEX hash_split_index ON hash_split_test USING hash (v); +-- Determine the layout of the (never-split) index: one metapage, nbuckets +-- bucket pages, and one bitmap page. The index's first bucket split will +-- split bucket 0, creating bucket nbuckets. +CREATE TEMP TABLE hash_split_geom AS + SELECT pg_relation_size('hash_split_index') / + current_setting('block_size')::bigint - 2 AS nbuckets; +-- sanity: initial bucket counts are always powers of two +SELECT nbuckets > 0 AND (nbuckets & (nbuckets - 1)) = 0 AS nbuckets_ok + FROM hash_split_geom; + nbuckets_ok +------------- + t +(1 row) + +-- Choose a value that maps to bucket 0 before the split and to the new +-- bucket afterwards +CREATE TEMP TABLE hash_split_key AS + SELECT min(v)::int4 AS k + FROM generate_series(1, 10000) v, hash_split_geom + WHERE (hashint4(v::int4) & (2 * nbuckets - 1)) = nbuckets; +INSERT INTO hash_split_test + SELECT k FROM hash_split_key, generate_series(1, 10); +-- Error out during the first bucket split, leaving it incomplete +SELECT injection_points_attach('hash-split-before-relocation', 'error'); + injection_points_attach +------------------------- + +(1 row) + +INSERT INTO hash_split_test + SELECT 1000000 + g FROM hash_split_geom, generate_series(1, 350 * nbuckets) g; +ERROR: error triggered for injection point hash-split-before-relocation +SELECT injection_points_detach('hash-split-before-relocation'); + injection_points_detach +------------------------- + +(1 row) + +-- This insertion goes to the new bucket, which is still flagged as being +-- populated by the incomplete split +INSERT INTO hash_split_test SELECT k FROM hash_split_key; +-- Scans of that bucket must visit both buckets of the incomplete split: +-- the fresh row from the new bucket, plus the 10 older rows that remain +-- in bucket 0 +SET enable_seqscan = off; +SET enable_bitmapscan = off; +EXPLAIN (COSTS OFF) + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + QUERY PLAN +------------------------------------------------------------ + Aggregate + InitPlan expr_1 + -> Seq Scan on hash_split_key + Disabled: true + -> Index Scan using hash_split_index on hash_split_test + Index Cond: (v = (InitPlan expr_1).col1) +(6 rows) + +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + count +------- + 11 +(1 row) + +-- Backward scans must visit both buckets too, in the opposite order +BEGIN; +DECLARE c SCROLL CURSOR FOR + SELECT v = (SELECT k FROM hash_split_key) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +MOVE FORWARD ALL IN c; +FETCH BACKWARD ALL FROM c; + ?column? +---------- + t + t + t + t + t + t + t + t + t + t + t +(11 rows) + +COMMIT; +-- Same count via a bitmap scan, which shares the underlying scan code +SET enable_indexscan = off; +SET enable_bitmapscan = on; +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + count +------- + 11 +(1 row) + +-- And the ground truth, via the heap +SET enable_bitmapscan = off; +RESET enable_seqscan; +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + count +------- + 11 +(1 row) + +RESET enable_indexscan; +RESET enable_bitmapscan; +DROP TABLE hash_split_test; +DROP EXTENSION injection_points; diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 59dba1cb0..a0b0c4827 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -36,6 +36,7 @@ tests += { 'hashagg', 'reindex_conc', 'vacuum', + 'hash_split', ], 'regress_args': ['--dlpath', meson.project_build_root() / 'src/test/regress'], # The injection points are cluster-wide, so disable installcheck @@ -44,6 +45,7 @@ tests += { 'isolation': { 'specs': [ 'basic', + 'hash-split', 'inplace', 'repack', 'repack_temporal', diff --git a/src/test/modules/injection_points/specs/hash-split.spec b/src/test/modules/injection_points/specs/hash-split.spec new file mode 100644 index 000000000..1a8e95c4c --- /dev/null +++ b/src/test/modules/injection_points/specs/hash-split.spec @@ -0,0 +1,85 @@ +# Test hash index scans that run while a bucket split is in progress. +# +# The interesting window opens once the split has relocated tuples to the +# new bucket, before the buckets' split-in-progress flags are cleared. A +# concurrent scan whose value belongs to the new bucket must visit both +# buckets of the pair: it skips the moved-by-split tuples in the new +# bucket, so it has to read their authoritative copies from the old +# bucket, plus any tuples inserted into the new bucket after the split +# began. +# +# s1 performs the split, pausing inside that window on a wait injection +# point (placed where the splitter holds no buffer content locks). s2 +# then inserts a matching row (which goes to the new bucket) and scans, +# both forwards and backwards. + +setup +{ + CREATE EXTENSION injection_points; + CREATE TABLE hash_split_test (v int4) WITH (autovacuum_enabled = false); + CREATE INDEX hash_split_index ON hash_split_test USING hash (v); + CREATE TABLE hash_split_geom AS + SELECT pg_relation_size('hash_split_index') / + current_setting('block_size')::bigint - 2 AS nbuckets; + CREATE TABLE hash_split_key AS + SELECT min(v)::int4 AS k + FROM generate_series(1, 10000) v, hash_split_geom + WHERE (hashint4(v::int4) & (2 * nbuckets - 1)) = nbuckets; + INSERT INTO hash_split_test + SELECT k FROM hash_split_key, generate_series(1, 10); +} + +teardown +{ + DROP TABLE hash_split_test, hash_split_geom, hash_split_key; + DROP EXTENSION injection_points; +} + +session s1 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('hash-split-after-relocation', 'wait'); +} +step s1_split +{ + INSERT INTO hash_split_test + SELECT 1000000 + g FROM hash_split_geom, generate_series(1, 320 * nbuckets) g; +} +step s1_noop { } + +session s2 +setup +{ + SET enable_seqscan = off; + SET enable_bitmapscan = off; +} +step s2_insert +{ + INSERT INTO hash_split_test SELECT k FROM hash_split_key; +} +step s2_scan +{ + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +} +step s2_back +{ + BEGIN; + DECLARE c SCROLL CURSOR FOR + SELECT v = (SELECT k FROM hash_split_key) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + MOVE FORWARD ALL IN c; + FETCH BACKWARD ALL FROM c; + COMMIT; +} +step s2_detach { SELECT injection_points_detach('hash-split-after-relocation'); } +step s2_wakeup { SELECT injection_points_wakeup('hash-split-after-relocation'); } + +# Scan during the paused split, then let the split finish and scan again. +# The detach must happen before the wakeup: the same INSERT can trigger +# another split, which must not hit the injection point again. The no-op +# step in the splitter's session forces the isolation tester to wait for +# s1_split to finish, keeping the position of its completion report stable +# regardless of how long the remaining inserts take (see basic.spec). +permutation s1_split s2_insert s2_scan s2_back s2_detach s2_wakeup s1_noop s2_scan diff --git a/src/test/modules/injection_points/sql/hash_split.sql b/src/test/modules/injection_points/sql/hash_split.sql new file mode 100644 index 000000000..010b65882 --- /dev/null +++ b/src/test/modules/injection_points/sql/hash_split.sql @@ -0,0 +1,80 @@ +-- Test hash index scans of a bucket pair whose split is incomplete +-- +-- The behavior tested here is not specific to incomplete splits: it +-- applies to any scan that runs while the bucket pair's split-in-progress +-- flags are set, which is the case for the duration of every split. +-- Interrupting a split is merely the most convenient way to hold the +-- bucket pair in that state: it makes the tests below deterministic, +-- without any need for a second session. The hash-split isolation test +-- provides equivalent coverage for scans that run during a live split. +CREATE EXTENSION injection_points; +SELECT injection_points_set_local(); + +CREATE TABLE hash_split_test (v int4) WITH (autovacuum_enabled = false); +CREATE INDEX hash_split_index ON hash_split_test USING hash (v); + +-- Determine the layout of the (never-split) index: one metapage, nbuckets +-- bucket pages, and one bitmap page. The index's first bucket split will +-- split bucket 0, creating bucket nbuckets. +CREATE TEMP TABLE hash_split_geom AS + SELECT pg_relation_size('hash_split_index') / + current_setting('block_size')::bigint - 2 AS nbuckets; +-- sanity: initial bucket counts are always powers of two +SELECT nbuckets > 0 AND (nbuckets & (nbuckets - 1)) = 0 AS nbuckets_ok + FROM hash_split_geom; + +-- Choose a value that maps to bucket 0 before the split and to the new +-- bucket afterwards +CREATE TEMP TABLE hash_split_key AS + SELECT min(v)::int4 AS k + FROM generate_series(1, 10000) v, hash_split_geom + WHERE (hashint4(v::int4) & (2 * nbuckets - 1)) = nbuckets; + +INSERT INTO hash_split_test + SELECT k FROM hash_split_key, generate_series(1, 10); + +-- Error out during the first bucket split, leaving it incomplete +SELECT injection_points_attach('hash-split-before-relocation', 'error'); +INSERT INTO hash_split_test + SELECT 1000000 + g FROM hash_split_geom, generate_series(1, 350 * nbuckets) g; +SELECT injection_points_detach('hash-split-before-relocation'); + +-- This insertion goes to the new bucket, which is still flagged as being +-- populated by the incomplete split +INSERT INTO hash_split_test SELECT k FROM hash_split_key; + +-- Scans of that bucket must visit both buckets of the incomplete split: +-- the fresh row from the new bucket, plus the 10 older rows that remain +-- in bucket 0 +SET enable_seqscan = off; +SET enable_bitmapscan = off; +EXPLAIN (COSTS OFF) + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + +-- Backward scans must visit both buckets too, in the opposite order +BEGIN; +DECLARE c SCROLL CURSOR FOR + SELECT v = (SELECT k FROM hash_split_key) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +MOVE FORWARD ALL IN c; +FETCH BACKWARD ALL FROM c; +COMMIT; + +-- Same count via a bitmap scan, which shares the underlying scan code +SET enable_indexscan = off; +SET enable_bitmapscan = on; +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +-- And the ground truth, via the heap +SET enable_bitmapscan = off; +RESET enable_seqscan; +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +RESET enable_indexscan; +RESET enable_bitmapscan; + +DROP TABLE hash_split_test; +DROP EXTENSION injection_points; -- 2.53.0 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Hash index bucket split bug @ 2026-07-09 16:31 Peter Geoghegan <[email protected]> parent: Peter Geoghegan <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Peter Geoghegan @ 2026-07-09 16:31 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Robert Haas <[email protected]> On Thu, Jul 9, 2026 at 11:24 AM Peter Geoghegan <[email protected]> wrote: > Attached v2 has a more stable version of the isolation test (no other > changes compared to V1). Does this version work for you? I ran this against CI, and saw failures on 32-bit meson tied to tuple alignment. When MAXALIGN is 4, the assumptions about page layout underlying the test case break. Attached is V3, which directly addresses the alignment issue, and fully passes CI. -- Peter Geoghegan Attachments: [application/x-patch] v3-0001-Fix-hash-index-scans-concurrent-with-bucket-split.patch (2.7K, ../../CAH2-Wz=qCx-2fpB0vec3fTGOnY8=mJ3O+6JY5V+V+mWDxV=fXQ@mail.gmail.com/2-v3-0001-Fix-hash-index-scans-concurrent-with-bucket-split.patch) download | inline diff: From a25ac1014587bc29c9ee4f2d6406d25afbf759ee Mon Sep 17 00:00:00 2001 From: Peter Geoghegan <[email protected]> Date: Wed, 8 Jul 2026 14:18:05 -0400 Subject: [PATCH v3 1/2] Fix hash index scans concurrent with bucket splits. Oversight in commit 7c75ef57, which introduced page-at-a-time processing of hash index scans. --- src/backend/access/hash/hashsearch.c | 54 ++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/backend/access/hash/hashsearch.c b/src/backend/access/hash/hashsearch.c index 89d1c5bc6..7882a531f 100644 --- a/src/backend/access/hash/hashsearch.c +++ b/src/backend/access/hash/hashsearch.c @@ -75,6 +75,28 @@ _hash_next(IndexScanDesc scan, ScanDirection dir) if (!_hash_readpage(scan, &buf, dir)) end_of_scan = true; } + else if (so->hashso_buc_populated && !so->hashso_buc_split) + { + /* + * end of bucket being populated: continue the scan in the + * bucket being split, on whose primary page we have held a + * pin since _hash_first + */ + buf = so->hashso_split_bucket_buf; + Assert(BufferIsValid(buf)); + LockBuffer(buf, BUFFER_LOCK_SHARE); + PredicateLockPage(rel, BufferGetBlockNumber(buf), + scan->xs_snapshot); + + /* + * setting hashso_buc_split to true indicates that we are + * scanning bucket being split. + */ + so->hashso_buc_split = true; + + if (!_hash_readpage(scan, &buf, dir)) + end_of_scan = true; + } else end_of_scan = true; } @@ -104,6 +126,38 @@ _hash_next(IndexScanDesc scan, ScanDirection dir) if (!_hash_readpage(scan, &buf, dir)) end_of_scan = true; } + else if (so->hashso_buc_populated && so->hashso_buc_split) + { + Page page; + HashPageOpaque opaque; + + /* + * start of bucket being split: continue the scan from the + * last page in the chain of the bucket being populated, on + * whose primary page we have held a pin since _hash_first + */ + buf = so->hashso_bucket_buf; + Assert(BufferIsValid(buf)); + LockBuffer(buf, BUFFER_LOCK_SHARE); + page = BufferGetPage(buf); + opaque = HashPageGetOpaque(page); + + /* move to the end of bucket chain */ + while (BlockNumberIsValid(opaque->hasho_nextblkno)) + _hash_readnext(scan, &buf, &page, &opaque); + + /* + * setting hashso_buc_split to false indicates that we are + * scanning the bucket being populated. Only set it after + * the chain walk above; otherwise _hash_readnext would + * advance to the bucket being split on reaching the end of + * the chain, instead of stopping there. + */ + so->hashso_buc_split = false; + + if (!_hash_readpage(scan, &buf, dir)) + end_of_scan = true; + } else end_of_scan = true; } -- 2.53.0 [application/x-patch] v3-0002-Add-tests-for-hash-index-scans-concurrent-with-bu.patch (18.4K, ../../CAH2-Wz=qCx-2fpB0vec3fTGOnY8=mJ3O+6JY5V+V+mWDxV=fXQ@mail.gmail.com/3-v3-0002-Add-tests-for-hash-index-scans-concurrent-with-bu.patch) download | inline diff: From 2624652ff90fee21e9d7f7cf97a9aa938c4c5acd Mon Sep 17 00:00:00 2001 From: Peter Geoghegan <[email protected]> Date: Wed, 8 Jul 2026 15:18:06 -0400 Subject: [PATCH v3 2/2] Add tests for hash index scans concurrent with bucket splits. Test the bug fixed by the preceding commit at two levels, using two new injection points in _hash_splitbucket: an error-mode point placed before tuple relocation begins (where existing code can fail anyway), and a wait-mode point placed after tuple relocation, where the splitter holds no buffer content locks. A regress test uses the error-mode point to interrupt a split partway through, then verifies that forward, backward, and bitmap scans of the affected bucket pair all agree with a seqscan. The bug isn't specific to interrupted splits: interrupting a split is just the most convenient way to hold the bucket pair in the same state that scans observe while any split is in progress, making the test deterministic without a second session. An isolation test proves that claim directly: it pauses a splitter on the wait-mode point and runs the same scans from a second session while the split remains genuinely in progress, then lets the split finish and scans once more. Co-Authored-By: Claude Fable 5 <[email protected]> --- src/backend/access/hash/hashpage.c | 5 + src/test/modules/injection_points/Makefile | 3 +- .../injection_points/expected/hash-split.out | 71 +++++++++ .../injection_points/expected/hash_split.out | 137 ++++++++++++++++++ src/test/modules/injection_points/meson.build | 2 + .../injection_points/specs/hash-split.spec | 91 ++++++++++++ .../injection_points/sql/hash_split.sql | 86 +++++++++++ 7 files changed, 394 insertions(+), 1 deletion(-) create mode 100644 src/test/modules/injection_points/expected/hash-split.out create mode 100644 src/test/modules/injection_points/expected/hash_split.out create mode 100644 src/test/modules/injection_points/specs/hash-split.spec create mode 100644 src/test/modules/injection_points/sql/hash_split.sql diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c index 8099b0d02..c2e7213c4 100644 --- a/src/backend/access/hash/hashpage.c +++ b/src/backend/access/hash/hashpage.c @@ -35,6 +35,7 @@ #include "port/pg_bitutils.h" #include "storage/predicate.h" #include "storage/smgr.h" +#include "utils/injection_point.h" #include "utils/rel.h" static bool _hash_alloc_buckets(Relation rel, BlockNumber firstblock, @@ -1104,6 +1105,8 @@ _hash_splitbucket(Relation rel, npage = BufferGetPage(nbuf); nopaque = HashPageGetOpaque(npage); + INJECTION_POINT("hash-split-before-relocation", NULL); + /* Copy the predicate locks from old bucket to new bucket. */ PredicateLockPageSplit(rel, BufferGetBlockNumber(bucket_obuf), @@ -1252,6 +1255,8 @@ _hash_splitbucket(Relation rel, /* be tidy */ for (i = 0; i < nitups; i++) pfree(itups[i]); + + INJECTION_POINT("hash-split-after-relocation", NULL); break; } diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index c01d2fb09..23e00207b 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -9,10 +9,11 @@ EXTENSION = injection_points DATA = injection_points--1.0.sql PGFILEDESC = "injection_points - facility for injection points" -REGRESS = injection_points hashagg reindex_conc vacuum +REGRESS = injection_points hashagg reindex_conc vacuum hash_split REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress ISOLATION = basic \ + hash-split \ inplace \ repack \ repack_temporal \ diff --git a/src/test/modules/injection_points/expected/hash-split.out b/src/test/modules/injection_points/expected/hash-split.out new file mode 100644 index 000000000..78ba43454 --- /dev/null +++ b/src/test/modules/injection_points/expected/hash-split.out @@ -0,0 +1,71 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_split s2_insert s2_scan s2_back s2_detach s2_wakeup s1_noop s2_scan +injection_points_attach +----------------------- + +(1 row) + +step s1_split: + INSERT INTO hash_split_test + SELECT 1000000 + g FROM hash_split_geom, generate_series(1, fillrows) g; + <waiting ...> +step s2_insert: + INSERT INTO hash_split_test SELECT k FROM hash_split_key; + +step s2_scan: + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + +count +----- + 11 +(1 row) + +step s2_back: + BEGIN; + DECLARE c SCROLL CURSOR FOR + SELECT v = (SELECT k FROM hash_split_key) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + MOVE FORWARD ALL IN c; + FETCH BACKWARD ALL FROM c; + COMMIT; + +?column? +-------- +t +t +t +t +t +t +t +t +t +t +t +(11 rows) + +step s2_detach: SELECT injection_points_detach('hash-split-after-relocation'); +injection_points_detach +----------------------- + +(1 row) + +step s2_wakeup: SELECT injection_points_wakeup('hash-split-after-relocation'); +injection_points_wakeup +----------------------- + +(1 row) + +step s1_split: <... completed> +step s1_noop: +step s2_scan: + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + +count +----- + 11 +(1 row) + diff --git a/src/test/modules/injection_points/expected/hash_split.out b/src/test/modules/injection_points/expected/hash_split.out new file mode 100644 index 000000000..6be113b9d --- /dev/null +++ b/src/test/modules/injection_points/expected/hash_split.out @@ -0,0 +1,137 @@ +-- Test hash index scans of a bucket pair whose split is incomplete +-- +-- The behavior tested here is not specific to incomplete splits: it +-- applies to any scan that runs while the bucket pair's split-in-progress +-- flags are set, which is the case for the duration of every split. +-- Interrupting a split is merely the most convenient way to hold the +-- bucket pair in that state: it makes the tests below deterministic, +-- without any need for a second session. The hash-split isolation test +-- provides equivalent coverage for scans that run during a live split. +CREATE EXTENSION injection_points; +SELECT injection_points_set_local(); + injection_points_set_local +---------------------------- + +(1 row) + +CREATE TABLE hash_split_test (v int4) WITH (autovacuum_enabled = false); +CREATE INDEX hash_split_index ON hash_split_test USING hash (v); +-- Determine the layout of the (never-split) index: one metapage, nbuckets +-- bucket pages, and one bitmap page. The index's first bucket split will +-- split bucket 0, creating bucket nbuckets. +CREATE TEMP TABLE hash_split_geom AS + SELECT nbuckets, + (current_setting('block_size')::bigint / 16) * nbuckets AS fillrows + FROM (SELECT pg_relation_size('hash_split_index') / + current_setting('block_size')::bigint - 2 AS nbuckets) g; +-- sanity: initial bucket counts are always powers of two +SELECT nbuckets > 0 AND (nbuckets & (nbuckets - 1)) = 0 AS nbuckets_ok + FROM hash_split_geom; + nbuckets_ok +------------- + t +(1 row) + +-- Choose a value that maps to bucket 0 before the split and to the new +-- bucket afterwards +CREATE TEMP TABLE hash_split_key AS + SELECT min(v)::int4 AS k + FROM generate_series(1, 10000) v, hash_split_geom + WHERE (hashint4(v::int4) & (2 * nbuckets - 1)) = nbuckets; +INSERT INTO hash_split_test + SELECT k FROM hash_split_key, generate_series(1, 10); +-- Error out during the first bucket split, leaving it incomplete. +-- fillrows is guaranteed to cross the split threshold of ffactor * nbuckets +-- tuples: a hash index entry occupies at least 16 bytes (line pointer +-- included), and the default fillfactor targets 75% page fullness, so +-- ffactor can never exceed block_size / 16 tuples per bucket. +SELECT injection_points_attach('hash-split-before-relocation', 'error'); + injection_points_attach +------------------------- + +(1 row) + +INSERT INTO hash_split_test + SELECT 1000000 + g FROM hash_split_geom, generate_series(1, fillrows) g; +ERROR: error triggered for injection point hash-split-before-relocation +SELECT injection_points_detach('hash-split-before-relocation'); + injection_points_detach +------------------------- + +(1 row) + +-- This insertion goes to the new bucket, which is still flagged as being +-- populated by the incomplete split +INSERT INTO hash_split_test SELECT k FROM hash_split_key; +-- Scans of that bucket must visit both buckets of the incomplete split: +-- the fresh row from the new bucket, plus the 10 older rows that remain +-- in bucket 0 +SET enable_seqscan = off; +SET enable_bitmapscan = off; +EXPLAIN (COSTS OFF) + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + QUERY PLAN +------------------------------------------------------------ + Aggregate + InitPlan expr_1 + -> Seq Scan on hash_split_key + Disabled: true + -> Index Scan using hash_split_index on hash_split_test + Index Cond: (v = (InitPlan expr_1).col1) +(6 rows) + +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + count +------- + 11 +(1 row) + +-- Backward scans must visit both buckets too, in the opposite order +BEGIN; +DECLARE c SCROLL CURSOR FOR + SELECT v = (SELECT k FROM hash_split_key) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +MOVE FORWARD ALL IN c; +FETCH BACKWARD ALL FROM c; + ?column? +---------- + t + t + t + t + t + t + t + t + t + t + t +(11 rows) + +COMMIT; +-- Same count via a bitmap scan, which shares the underlying scan code +SET enable_indexscan = off; +SET enable_bitmapscan = on; +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + count +------- + 11 +(1 row) + +-- And the ground truth, via the heap +SET enable_bitmapscan = off; +RESET enable_seqscan; +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + count +------- + 11 +(1 row) + +RESET enable_indexscan; +RESET enable_bitmapscan; +DROP TABLE hash_split_test; +DROP EXTENSION injection_points; diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 59dba1cb0..a0b0c4827 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -36,6 +36,7 @@ tests += { 'hashagg', 'reindex_conc', 'vacuum', + 'hash_split', ], 'regress_args': ['--dlpath', meson.project_build_root() / 'src/test/regress'], # The injection points are cluster-wide, so disable installcheck @@ -44,6 +45,7 @@ tests += { 'isolation': { 'specs': [ 'basic', + 'hash-split', 'inplace', 'repack', 'repack_temporal', diff --git a/src/test/modules/injection_points/specs/hash-split.spec b/src/test/modules/injection_points/specs/hash-split.spec new file mode 100644 index 000000000..24390b9d2 --- /dev/null +++ b/src/test/modules/injection_points/specs/hash-split.spec @@ -0,0 +1,91 @@ +# Test hash index scans that run while a bucket split is in progress. +# +# The interesting window opens once the split has relocated tuples to the +# new bucket, before the buckets' split-in-progress flags are cleared. A +# concurrent scan whose value belongs to the new bucket must visit both +# buckets of the pair: it skips the moved-by-split tuples in the new +# bucket, so it has to read their authoritative copies from the old +# bucket, plus any tuples inserted into the new bucket after the split +# began. +# +# s1 performs the split, pausing inside that window on a wait injection +# point (placed where the splitter holds no buffer content locks). s2 +# then inserts a matching row (which goes to the new bucket) and scans, +# both forwards and backwards. + +setup +{ + CREATE EXTENSION injection_points; + CREATE TABLE hash_split_test (v int4) WITH (autovacuum_enabled = false); + CREATE INDEX hash_split_index ON hash_split_test USING hash (v); + CREATE TABLE hash_split_geom AS + SELECT nbuckets, + (current_setting('block_size')::bigint / 16) * nbuckets AS fillrows + FROM (SELECT pg_relation_size('hash_split_index') / + current_setting('block_size')::bigint - 2 AS nbuckets) g; + CREATE TABLE hash_split_key AS + SELECT min(v)::int4 AS k + FROM generate_series(1, 10000) v, hash_split_geom + WHERE (hashint4(v::int4) & (2 * nbuckets - 1)) = nbuckets; + INSERT INTO hash_split_test + SELECT k FROM hash_split_key, generate_series(1, 10); +} + +teardown +{ + DROP TABLE hash_split_test, hash_split_geom, hash_split_key; + DROP EXTENSION injection_points; +} + +session s1 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('hash-split-after-relocation', 'wait'); +} +# fillrows is guaranteed to cross the split threshold of ffactor * nbuckets +# tuples: a hash index entry occupies at least 16 bytes (line pointer +# included), and the default fillfactor targets 75% page fullness, so +# ffactor can never exceed block_size / 16 tuples per bucket +step s1_split +{ + INSERT INTO hash_split_test + SELECT 1000000 + g FROM hash_split_geom, generate_series(1, fillrows) g; +} +step s1_noop { } + +session s2 +setup +{ + SET enable_seqscan = off; + SET enable_bitmapscan = off; +} +step s2_insert +{ + INSERT INTO hash_split_test SELECT k FROM hash_split_key; +} +step s2_scan +{ + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +} +step s2_back +{ + BEGIN; + DECLARE c SCROLL CURSOR FOR + SELECT v = (SELECT k FROM hash_split_key) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + MOVE FORWARD ALL IN c; + FETCH BACKWARD ALL FROM c; + COMMIT; +} +step s2_detach { SELECT injection_points_detach('hash-split-after-relocation'); } +step s2_wakeup { SELECT injection_points_wakeup('hash-split-after-relocation'); } + +# Scan during the paused split, then let the split finish and scan again. +# The detach must happen before the wakeup: the same INSERT can trigger +# another split, which must not hit the injection point again. The no-op +# step in the splitter's session forces the isolation tester to wait for +# s1_split to finish, keeping the position of its completion report stable +# regardless of how long the remaining inserts take (see basic.spec). +permutation s1_split s2_insert s2_scan s2_back s2_detach s2_wakeup s1_noop s2_scan diff --git a/src/test/modules/injection_points/sql/hash_split.sql b/src/test/modules/injection_points/sql/hash_split.sql new file mode 100644 index 000000000..f49c33549 --- /dev/null +++ b/src/test/modules/injection_points/sql/hash_split.sql @@ -0,0 +1,86 @@ +-- Test hash index scans of a bucket pair whose split is incomplete +-- +-- The behavior tested here is not specific to incomplete splits: it +-- applies to any scan that runs while the bucket pair's split-in-progress +-- flags are set, which is the case for the duration of every split. +-- Interrupting a split is merely the most convenient way to hold the +-- bucket pair in that state: it makes the tests below deterministic, +-- without any need for a second session. The hash-split isolation test +-- provides equivalent coverage for scans that run during a live split. +CREATE EXTENSION injection_points; +SELECT injection_points_set_local(); + +CREATE TABLE hash_split_test (v int4) WITH (autovacuum_enabled = false); +CREATE INDEX hash_split_index ON hash_split_test USING hash (v); + +-- Determine the layout of the (never-split) index: one metapage, nbuckets +-- bucket pages, and one bitmap page. The index's first bucket split will +-- split bucket 0, creating bucket nbuckets. +CREATE TEMP TABLE hash_split_geom AS + SELECT nbuckets, + (current_setting('block_size')::bigint / 16) * nbuckets AS fillrows + FROM (SELECT pg_relation_size('hash_split_index') / + current_setting('block_size')::bigint - 2 AS nbuckets) g; +-- sanity: initial bucket counts are always powers of two +SELECT nbuckets > 0 AND (nbuckets & (nbuckets - 1)) = 0 AS nbuckets_ok + FROM hash_split_geom; + +-- Choose a value that maps to bucket 0 before the split and to the new +-- bucket afterwards +CREATE TEMP TABLE hash_split_key AS + SELECT min(v)::int4 AS k + FROM generate_series(1, 10000) v, hash_split_geom + WHERE (hashint4(v::int4) & (2 * nbuckets - 1)) = nbuckets; + +INSERT INTO hash_split_test + SELECT k FROM hash_split_key, generate_series(1, 10); + +-- Error out during the first bucket split, leaving it incomplete. +-- fillrows is guaranteed to cross the split threshold of ffactor * nbuckets +-- tuples: a hash index entry occupies at least 16 bytes (line pointer +-- included), and the default fillfactor targets 75% page fullness, so +-- ffactor can never exceed block_size / 16 tuples per bucket. +SELECT injection_points_attach('hash-split-before-relocation', 'error'); +INSERT INTO hash_split_test + SELECT 1000000 + g FROM hash_split_geom, generate_series(1, fillrows) g; +SELECT injection_points_detach('hash-split-before-relocation'); + +-- This insertion goes to the new bucket, which is still flagged as being +-- populated by the incomplete split +INSERT INTO hash_split_test SELECT k FROM hash_split_key; + +-- Scans of that bucket must visit both buckets of the incomplete split: +-- the fresh row from the new bucket, plus the 10 older rows that remain +-- in bucket 0 +SET enable_seqscan = off; +SET enable_bitmapscan = off; +EXPLAIN (COSTS OFF) + SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); + +-- Backward scans must visit both buckets too, in the opposite order +BEGIN; +DECLARE c SCROLL CURSOR FOR + SELECT v = (SELECT k FROM hash_split_key) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +MOVE FORWARD ALL IN c; +FETCH BACKWARD ALL FROM c; +COMMIT; + +-- Same count via a bitmap scan, which shares the underlying scan code +SET enable_indexscan = off; +SET enable_bitmapscan = on; +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +-- And the ground truth, via the heap +SET enable_bitmapscan = off; +RESET enable_seqscan; +SELECT count(*) FROM hash_split_test + WHERE v = (SELECT k FROM hash_split_key); +RESET enable_indexscan; +RESET enable_bitmapscan; + +DROP TABLE hash_split_test; +DROP EXTENSION injection_points; -- 2.53.0 ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-07-09 16:31 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-07-08 20:39 Hash index bucket split bug Peter Geoghegan <[email protected]> 2026-07-09 10:45 ` Amit Kapila <[email protected]> 2026-07-09 15:24 ` Peter Geoghegan <[email protected]> 2026-07-09 16:31 ` Peter Geoghegan <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox