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')