From a85b5523e679870414bf94e44dee424d73753abb Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Sat, 10 Oct 2026 10:26:18 -0400 Subject: [PATCH] Fix hash index scans that start during a bucket split. A hash index scan whose target bucket is still being populated by a split must read both buckets of the split: forward scans read the bucket being populated (skipping the tuples that the split copied there) and then the bucket being split, and backward scans read them in the opposite order. _hash_readnext and _hash_readprev make the transition from the first bucket to the second, but _hash_next did not: on reaching the end of the first bucket, it ended the scan, so every matching tuple in the second bucket was overlooked. To fix, teach _hash_next to make the same transition, using helper functions shared with _hash_readnext and _hash_readprev. _hash_next only missed the transition when the last page that it read in the first bucket had a match (otherwise _hash_readpage's loop made the required transition). Getting wrong answers also needed a matching tuple to be inserted into the bucket being populated _after_ the split began. Forward scans then lost the older tuples in the bucket being split. An incomplete split would keep the buckets in this state until an inserter finished the split at some indeterminate later time. Note that wrong answers didn't require an incomplete split. Although this fix treats the bug as the omission of a required step in _hash_next, the real problem is the way that page stepping works in general. The logic for stepping to the next page appears both in high-level routines like _hash_next and in low-level routines like _hash_readnext and _hash_readprev. If anything, _hash_next already knew too much about page stepping (cf. nbtree's _bt_next function). All of the logic around stepping really belongs in one routine, modeled on nbtree's _bt_steppage function, but that approach is much too invasive for a backpatched fix. Claude Code found this problem. The committed test case is a simplified version of the one that it wrote to demonstrate this bug. Oversight in commit 7c75ef57, which introduced page-at-a-time processing of hash index scans. Author: Peter Geoghegan Author: Amit Kapila Reviewed-by: Vignesh C Discussion: https://postgr.es/m/CAH2-Wz=4CJK9ysZzbzxBGGTkgAg9ib8cJyCx=9q_d1r8tf0ang@mail.gmail.com Backpatch-through: 14 --- src/backend/access/hash/hashpage.c | 5 + src/backend/access/hash/hashsearch.c | 138 +++++++++++++----- src/test/modules/Makefile | 4 +- src/test/modules/hash/.gitignore | 4 + src/test/modules/hash/Makefile | 28 ++++ .../hash/expected/hash_incomplete_splits.out | 114 +++++++++++++++ src/test/modules/hash/meson.build | 16 ++ .../hash/sql/hash_incomplete_splits.sql | 53 +++++++ src/test/modules/meson.build | 1 + 9 files changed, 323 insertions(+), 40 deletions(-) create mode 100644 src/test/modules/hash/.gitignore create mode 100644 src/test/modules/hash/Makefile create mode 100644 src/test/modules/hash/expected/hash_incomplete_splits.out create mode 100644 src/test/modules/hash/meson.build create mode 100644 src/test/modules/hash/sql/hash_incomplete_splits.sql diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c index b8e5bd005e5..6df4b665a81 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, @@ -1106,6 +1107,8 @@ _hash_splitbucket(Relation rel, BufferGetBlockNumber(bucket_obuf), BufferGetBlockNumber(bucket_nbuf)); + INJECTION_POINT("hash-split-before-relocation", NULL); + /* * Partition the tuples in the old bucket between the old bucket and the * new bucket, advancing along the old bucket's overflow bucket chain and @@ -1453,6 +1456,8 @@ _hash_finish_split(Relation rel, Buffer metabuf, Buffer obuf, Bucket obucket, npageopaque = HashPageGetOpaque(npage); nbucket = npageopaque->hasho_bucket; + INJECTION_POINT("hash-finish-incomplete-split", NULL); + _hash_splitbucket(rel, metabuf, obucket, nbucket, obuf, bucket_nbuf, tidhtab, maxbucket, highmask, lowmask); diff --git a/src/backend/access/hash/hashsearch.c b/src/backend/access/hash/hashsearch.c index 92c15a65be2..8f345e89131 100644 --- a/src/backend/access/hash/hashsearch.c +++ b/src/backend/access/hash/hashsearch.c @@ -21,14 +21,18 @@ #include "storage/predicate.h" #include "utils/rel.h" +static void _hash_readnext(IndexScanDesc scan, Buffer *bufp, + Page *pagep, HashPageOpaque *opaquep); +static void _hash_readprev(IndexScanDesc scan, Buffer *bufp, + Page *pagep, HashPageOpaque *opaquep); +static Buffer _hash_step_to_split_bucket(IndexScanDesc scan); +static Buffer _hash_step_to_populated_bucket(IndexScanDesc scan); static bool _hash_readpage(IndexScanDesc scan, Buffer *bufP, ScanDirection dir); static int _hash_load_qualified_items(IndexScanDesc scan, Page page, OffsetNumber offnum, ScanDirection dir); static inline void _hash_saveitem(HashScanOpaque so, int itemIndex, OffsetNumber offnum, IndexTuple itup); -static void _hash_readnext(IndexScanDesc scan, Buffer *bufp, - Page *pagep, HashPageOpaque *opaquep); /* * _hash_next() -- Get the next item in a scan. @@ -74,6 +78,13 @@ _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) + { + buf = _hash_step_to_split_bucket(scan); + + if (!_hash_readpage(scan, &buf, dir)) + end_of_scan = true; + } else end_of_scan = true; } @@ -103,6 +114,13 @@ _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) + { + buf = _hash_step_to_populated_bucket(scan); + + if (!_hash_readpage(scan, &buf, dir)) + end_of_scan = true; + } else end_of_scan = true; } @@ -110,6 +128,14 @@ _hash_next(IndexScanDesc scan, ScanDirection dir) if (end_of_scan) { + /* + * A scan that started during a bucket split ends in the bucket that + * its direction visits last: forward scans end in the bucket being + * split, backward scans in the bucket being populated. + */ + Assert(!so->hashso_buc_populated || + so->hashso_buc_split == ScanDirectionIsForward(dir)); + _hash_dropscanbuf(rel, so); HashScanPosInvalidate(so->currPos); return false; @@ -161,23 +187,7 @@ _hash_readnext(IndexScanDesc scan, * end of bucket, scan bucket being split if there was a split in * progress at the start of scan. */ - *bufp = so->hashso_split_bucket_buf; - - /* - * buffer for bucket being split must be valid as we acquire the pin - * on it before the start of scan and retain it till end of scan. - */ - Assert(BufferIsValid(*bufp)); - - LockBuffer(*bufp, BUFFER_LOCK_SHARE); - PredicateLockPage(rel, BufferGetBlockNumber(*bufp), scan->xs_snapshot); - - /* - * setting hashso_buc_split to true indicates that we are scanning - * bucket being split. - */ - so->hashso_buc_split = true; - + *bufp = _hash_step_to_split_bucket(scan); block_found = true; } @@ -244,30 +254,82 @@ _hash_readprev(IndexScanDesc scan, * end of bucket, scan bucket being populated if there was a split in * progress at the start of scan. */ - *bufp = so->hashso_bucket_buf; - - /* - * buffer for bucket being populated must be valid as we acquire the - * pin on it before the start of scan and retain it till end of scan. - */ - Assert(BufferIsValid(*bufp)); - - LockBuffer(*bufp, BUFFER_LOCK_SHARE); + *bufp = _hash_step_to_populated_bucket(scan); *pagep = BufferGetPage(*bufp); *opaquep = HashPageGetOpaque(*pagep); - - /* move to the end of bucket chain */ - while (BlockNumberIsValid((*opaquep)->hasho_nextblkno)) - _hash_readnext(scan, bufp, pagep, opaquep); - - /* - * setting hashso_buc_split to false indicates that we are scanning - * bucket being populated. - */ - so->hashso_buc_split = false; } } +/* + * Cross over from the bucket being populated to the bucket being split, on + * whose primary page we have held a pin since _hash_first. Called on + * reaching the end of the populated bucket's chain while moving forward + * through it. + * + * Returns the split bucket's primary page, pinned and share-locked, and + * sets hashso_buc_split to indicate that we are now scanning that bucket. + */ +static Buffer +_hash_step_to_split_bucket(IndexScanDesc scan) +{ + Relation rel = scan->indexRelation; + HashScanOpaque so = (HashScanOpaque) scan->opaque; + Buffer buf = so->hashso_split_bucket_buf; + + /* + * buffer for bucket being split must be valid as we acquire the pin on it + * before the start of scan and retain it till end of scan. + */ + Assert(BufferIsValid(buf)); + + LockBuffer(buf, BUFFER_LOCK_SHARE); + PredicateLockPage(rel, BufferGetBlockNumber(buf), scan->xs_snapshot); + + so->hashso_buc_split = true; + + return buf; +} + +/* + * Cross over from the bucket being split back to the bucket being + * populated, on whose primary page we have held a pin since _hash_first, + * and walk to the end of its chain (backward scans read chains tail to + * head). Called on reaching the start of the split bucket's chain while + * moving backward through it. + * + * Returns the last page in the populated bucket's chain, pinned and + * share-locked, and clears hashso_buc_split: from here on the scan skips + * the moved-by-split tuples, whose originals it reads in the split + * bucket, and stops at the populated bucket's primary page instead of + * crossing again. + */ +static Buffer +_hash_step_to_populated_bucket(IndexScanDesc scan) +{ + HashScanOpaque so = (HashScanOpaque) scan->opaque; + Buffer buf = so->hashso_bucket_buf; + Page page; + HashPageOpaque opaque; + + /* + * buffer for bucket being populated must be valid as we acquire the pin + * on it before the start of scan and retain it till end of scan. + */ + 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); + + so->hashso_buc_split = false; + + return buf; +} + /* * _hash_first() -- Find the first item in a scan. * diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 4e82d6f1517..1ca50975d9c 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -48,9 +48,9 @@ SUBDIRS = \ ifeq ($(enable_injection_points),yes) -SUBDIRS += injection_points gin typcache +SUBDIRS += injection_points gin hash typcache else -ALWAYS_SUBDIRS += injection_points gin typcache +ALWAYS_SUBDIRS += injection_points gin hash typcache endif ifeq ($(with_ssl),openssl) diff --git a/src/test/modules/hash/.gitignore b/src/test/modules/hash/.gitignore new file mode 100644 index 00000000000..5dcb3ff9723 --- /dev/null +++ b/src/test/modules/hash/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/hash/Makefile b/src/test/modules/hash/Makefile new file mode 100644 index 00000000000..977c05ef4d2 --- /dev/null +++ b/src/test/modules/hash/Makefile @@ -0,0 +1,28 @@ +# src/test/modules/hash/Makefile + +EXTRA_INSTALL = src/test/modules/injection_points + +REGRESS = hash_incomplete_splits + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/hash +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global + +# XXX: This test is conditional on enable_injection_points in the +# parent Makefile, so we should never get here in the first place if +# injection points are not enabled. But the buildfarm 'misc-check' +# step doesn't pay attention to the if-condition in the parent +# Makefile. To work around that, disable running the test here too. +ifeq ($(enable_injection_points),yes) +include $(top_srcdir)/contrib/contrib-global.mk +else +check: + @echo "injection points are disabled in this build" +endif + +endif diff --git a/src/test/modules/hash/expected/hash_incomplete_splits.out b/src/test/modules/hash/expected/hash_incomplete_splits.out new file mode 100644 index 00000000000..4e4220a76c1 --- /dev/null +++ b/src/test/modules/hash/expected/hash_incomplete_splits.out @@ -0,0 +1,114 @@ +-- Test hash index scans of a bucket pair whose split is incomplete. +-- +-- A scan whose target bucket is still being populated by a split must read +-- both buckets: the one being populated, skipping the tuples moved there, +-- and the one being split. +CREATE EXTENSION injection_points; +SELECT injection_points_set_local(); + injection_points_set_local +---------------------------- + +(1 row) + +SELECT injection_points_attach('hash-finish-incomplete-split', 'notice'); + injection_points_attach +------------------------- + +(1 row) + +-- Build an index on an analyzed 10-row table, so that it starts out with +-- just two buckets (metapage, two bucket pages, bitmap page). Rows with +-- v = 1 hash to bucket 0, and to bucket 2 once bucket 0 splits. +CREATE TABLE hash_split_test (v int4) WITH (autovacuum_enabled = false); +INSERT INTO hash_split_test SELECT 1 FROM generate_series(1, 10); +ANALYZE hash_split_test; +CREATE INDEX hash_split_index ON hash_split_test USING hash (v); +SELECT pg_relation_size('hash_split_index') / + current_setting('block_size')::int AS index_pages; + index_pages +------------- + 4 +(1 row) + +-- 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 g FROM generate_series(1000001, 1005000) 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 row goes to the new bucket, which is still flagged as being populated +INSERT INTO hash_split_test VALUES (1); +-- Forward and backward index scans must find it there, 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 = 1; + QUERY PLAN +------------------------------------------------------------ + Aggregate + -> Index Scan using hash_split_index on hash_split_test + Index Cond: (v = 1) +(3 rows) + +SELECT count(*) FROM hash_split_test WHERE v = 1; + count +------- + 11 +(1 row) + +BEGIN; +DECLARE c SCROLL CURSOR FOR SELECT v FROM hash_split_test WHERE v = 1; +MOVE FORWARD ALL IN c; +FETCH BACKWARD ALL FROM c; + v +--- + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 +(11 rows) + +COMMIT; +-- An insert into bucket 0 (v = 5 hashes there) finishes the split. The +-- scan must still find all 11 rows. +INSERT INTO hash_split_test VALUES (5); +NOTICE: notice triggered for injection point hash-finish-incomplete-split +SELECT count(*) FROM hash_split_test WHERE v = 1; + count +------- + 11 +(1 row) + +-- The heap agrees +SET enable_seqscan = on; +SET enable_indexscan = off; +SELECT count(*) FROM hash_split_test WHERE v = 1; + count +------- + 11 +(1 row) + +SELECT injection_points_detach('hash-finish-incomplete-split'); + injection_points_detach +------------------------- + +(1 row) + +DROP TABLE hash_split_test; +DROP EXTENSION injection_points; diff --git a/src/test/modules/hash/meson.build b/src/test/modules/hash/meson.build new file mode 100644 index 00000000000..284ebab70ff --- /dev/null +++ b/src/test/modules/hash/meson.build @@ -0,0 +1,16 @@ +# Copyright (c) 2022-2026, PostgreSQL Global Development Group + +if not get_option('injection_points') + subdir_done() +endif + +tests += { + 'name': 'hash', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'regress': { + 'sql': [ + 'hash_incomplete_splits', + ], + }, +} diff --git a/src/test/modules/hash/sql/hash_incomplete_splits.sql b/src/test/modules/hash/sql/hash_incomplete_splits.sql new file mode 100644 index 00000000000..f82da7f369e --- /dev/null +++ b/src/test/modules/hash/sql/hash_incomplete_splits.sql @@ -0,0 +1,53 @@ +-- Test hash index scans of a bucket pair whose split is incomplete. +-- +-- A scan whose target bucket is still being populated by a split must read +-- both buckets: the one being populated, skipping the tuples moved there, +-- and the one being split. +CREATE EXTENSION injection_points; +SELECT injection_points_set_local(); + +SELECT injection_points_attach('hash-finish-incomplete-split', 'notice'); + +-- Build an index on an analyzed 10-row table, so that it starts out with +-- just two buckets (metapage, two bucket pages, bitmap page). Rows with +-- v = 1 hash to bucket 0, and to bucket 2 once bucket 0 splits. +CREATE TABLE hash_split_test (v int4) WITH (autovacuum_enabled = false); +INSERT INTO hash_split_test SELECT 1 FROM generate_series(1, 10); +ANALYZE hash_split_test; +CREATE INDEX hash_split_index ON hash_split_test USING hash (v); +SELECT pg_relation_size('hash_split_index') / + current_setting('block_size')::int AS index_pages; + +-- 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 g FROM generate_series(1000001, 1005000) g; +SELECT injection_points_detach('hash-split-before-relocation'); + +-- This row goes to the new bucket, which is still flagged as being populated +INSERT INTO hash_split_test VALUES (1); + +-- Forward and backward index scans must find it there, 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 = 1; +SELECT count(*) FROM hash_split_test WHERE v = 1; +BEGIN; +DECLARE c SCROLL CURSOR FOR SELECT v FROM hash_split_test WHERE v = 1; +MOVE FORWARD ALL IN c; +FETCH BACKWARD ALL FROM c; +COMMIT; + +-- An insert into bucket 0 (v = 5 hashes there) finishes the split. The +-- scan must still find all 11 rows. +INSERT INTO hash_split_test VALUES (5); +SELECT count(*) FROM hash_split_test WHERE v = 1; + +-- The heap agrees +SET enable_seqscan = on; +SET enable_indexscan = off; +SELECT count(*) FROM hash_split_test WHERE v = 1; + +SELECT injection_points_detach('hash-finish-incomplete-split'); +DROP TABLE hash_split_test; +DROP EXTENSION injection_points; diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build index 9a957351ab6..543a9ea49aa 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -6,6 +6,7 @@ subdir('delay_execution') subdir('dummy_index_am') subdir('dummy_seclabel') subdir('gin') +subdir('hash') subdir('injection_points') subdir('ldap_password_func') subdir('libpq_pipeline')