From 2fcb7219e30bacd9803521cc4b4f2ac9879f9a8a Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Thu, 8 Oct 2026 22:11:19 -0400 Subject: [PATCH v1] Fix hash bucket split buffer pin leak. _hash_finish_split walks the new bucket to collect the TIDs that the interrupted split already moved, keeping a pin on the bucket's primary page, and then takes conditional cleanup locks on both buckets. When either lock could not be taken, it gave up without releasing that pin. Claude Code found this problem. Oversight in commit 6d46f4783, which introduced this way of finishing an incomplete split. Backpatch-through: 14 --- src/backend/access/hash/hashpage.c | 4 ++ src/test/modules/hash/.gitignore | 2 + src/test/modules/hash/Makefile | 2 + .../hash/expected/finish-incomplete-split.out | 49 +++++++++++++ src/test/modules/hash/meson.build | 6 ++ .../hash/specs/finish-incomplete-split.spec | 72 +++++++++++++++++++ 6 files changed, 135 insertions(+) create mode 100644 src/test/modules/hash/expected/finish-incomplete-split.out create mode 100644 src/test/modules/hash/specs/finish-incomplete-split.spec diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c index fc11ed6b7..c381b1203 100644 --- a/src/backend/access/hash/hashpage.c +++ b/src/backend/access/hash/hashpage.c @@ -1444,14 +1444,18 @@ _hash_finish_split(Relation rel, Buffer metabuf, Buffer obuf, Bucket obucket, * up and next insertion on old bucket will try again to complete the * split. */ + INJECTION_POINT("hash-finish-split-before-cleanup-locks", NULL); + if (!ConditionalLockBufferForCleanup(obuf)) { + _hash_dropbuf(rel, bucket_nbuf); hash_destroy(tidhtab); return; } if (!ConditionalLockBufferForCleanup(bucket_nbuf)) { LockBuffer(obuf, BUFFER_LOCK_UNLOCK); + _hash_dropbuf(rel, bucket_nbuf); hash_destroy(tidhtab); return; } diff --git a/src/test/modules/hash/.gitignore b/src/test/modules/hash/.gitignore index 5dcb3ff97..0de307e70 100644 --- a/src/test/modules/hash/.gitignore +++ b/src/test/modules/hash/.gitignore @@ -1,4 +1,6 @@ # Generated subdirectories /log/ +/output_iso/ /results/ /tmp_check/ +/tmp_check_iso/ diff --git a/src/test/modules/hash/Makefile b/src/test/modules/hash/Makefile index 977c05ef4..3d5f258a0 100644 --- a/src/test/modules/hash/Makefile +++ b/src/test/modules/hash/Makefile @@ -4,6 +4,8 @@ EXTRA_INSTALL = src/test/modules/injection_points REGRESS = hash_incomplete_splits +ISOLATION = finish-incomplete-split + ifdef USE_PGXS PG_CONFIG = pg_config PGXS := $(shell $(PG_CONFIG) --pgxs) diff --git a/src/test/modules/hash/expected/finish-incomplete-split.out b/src/test/modules/hash/expected/finish-incomplete-split.out new file mode 100644 index 000000000..46f8a7899 --- /dev/null +++ b/src/test/modules/hash/expected/finish-incomplete-split.out @@ -0,0 +1,49 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_incomplete s1_insert s2_pin s2_wakeup s1_count s2_commit +injection_points_attach +----------------------- + +(1 row) + +step s1_incomplete: + INSERT INTO hash_split_test SELECT g FROM generate_series(1000001, 1005000) g; + +ERROR: error triggered for injection point hash-split-before-relocation +step s1_insert: + INSERT INTO hash_split_test VALUES (5); + +step s2_pin: + BEGIN; + DECLARE c CURSOR FOR SELECT v FROM hash_split_test WHERE v = 6; + FETCH 1 FROM c; + +v +- +6 +(1 row) + +step s2_wakeup: + SELECT injection_points_detach('hash-finish-split-before-cleanup-locks'); + SELECT injection_points_wakeup('hash-finish-split-before-cleanup-locks'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step s1_insert: <... completed> +step s1_count: + SELECT count(*) FROM hash_split_test WHERE v = 5; + +count +----- + 1 +(1 row) + +step s2_commit: COMMIT; diff --git a/src/test/modules/hash/meson.build b/src/test/modules/hash/meson.build index 284ebab70..bb22ec1e2 100644 --- a/src/test/modules/hash/meson.build +++ b/src/test/modules/hash/meson.build @@ -13,4 +13,10 @@ tests += { 'hash_incomplete_splits', ], }, + 'isolation': { + 'specs': [ + 'finish-incomplete-split', + ], + 'runningcheck': false, + }, } diff --git a/src/test/modules/hash/specs/finish-incomplete-split.spec b/src/test/modules/hash/specs/finish-incomplete-split.spec new file mode 100644 index 000000000..e38a99d5c --- /dev/null +++ b/src/test/modules/hash/specs/finish-incomplete-split.spec @@ -0,0 +1,72 @@ +# Test an attempt to finish an incomplete hash bucket split that must give up. +# +# _hash_finish_split walks the new bucket, keeping a pin on its primary page, +# then takes conditional cleanup locks on both buckets. When another backend +# holds a pin on the old bucket at that moment it has to give up, and it must +# release the pin on the new bucket: the resource owner reports a pin that is +# still held when the statement ends, and a leaked pin also makes every later +# attempt fail, since a cleanup lock needs a single pin. + +setup +{ + CREATE EXTENSION injection_points; + CREATE TABLE hash_split_test (v int4) WITH (autovacuum_enabled = false); + INSERT INTO hash_split_test VALUES (6); + ANALYZE hash_split_test; + CREATE INDEX hash_split_index ON hash_split_test USING hash (v); +} + +teardown +{ + DROP TABLE hash_split_test; + DROP EXTENSION injection_points; +} + +session s1 +# The error point leaves the first split incomplete. It stays attached as a +# guard: an attempt to finish the split that got as far as moving tuples +# would hit it and fail. +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('hash-split-before-relocation', 'error'); + SELECT injection_points_attach('hash-finish-split-before-cleanup-locks', 'wait'); +} +step s1_incomplete +{ + INSERT INTO hash_split_test SELECT g FROM generate_series(1000001, 1005000) g; +} +# v = 5 hashes to bucket 0, so inserting it tries to finish the split first +step s1_insert +{ + INSERT INTO hash_split_test VALUES (5); +} +step s1_count +{ + SELECT count(*) FROM hash_split_test WHERE v = 5; +} + +session s2 +setup +{ + SET enable_seqscan = off; + SET enable_bitmapscan = off; +} +# Pin bucket 0's primary page through the cursor while s1 is paused; v = 6 +# is in bucket 0 +step s2_pin +{ + BEGIN; + DECLARE c CURSOR FOR SELECT v FROM hash_split_test WHERE v = 6; + FETCH 1 FROM c; +} +step s2_wakeup +{ + SELECT injection_points_detach('hash-finish-split-before-cleanup-locks'); + SELECT injection_points_wakeup('hash-finish-split-before-cleanup-locks'); +} +step s2_commit { COMMIT; } + +# s1_count runs after s1_insert completes, which keeps the position of the +# completion report stable +permutation s1_incomplete s1_insert s2_pin s2_wakeup s1_count s2_commit -- 2.55.0