From f5eb854ab6d6281ec2d3143657944bdda6676341 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Tue, 17 Mar 2026 14:54:41 -0400 Subject: [PATCH] Fix use of wrong variable in _hash_kill_items() In 82467f627bd I somehow ended up using 'so->currPos.buf' instead of the 'buf' variable, which is incorrect when the buffer is not already pinned. At the very least this can lead to assertion failures Unfortunately this shows that this code path was not covered. Expand src/test/modules/index/specs/killtuples.spec to test it. Until now the 'result' step always reported either a 0 or 1 buffer accesses, but when exercising hash overflows, more buffers are accessed. To avoid depending on the precise number of accesses, change the result step to return whether there were any heap accesses. That makes the change a lot more verbose, but still seems worth it. Reported-by: Alexander Kuzmenkov Reported-by: Alexander Lakhin Reported-by: Heikki Linnakangas Discussion: https://postgr.es/m/vjtmvwvbxt7w5uyacxpzibpj65ewcb7uqaqbhd4arvnjbp5jqz%405ksdh6fsyqve Discussion: https://postgr.es/m/b9de8d05-3b02-4a27-9b0b-03972fa4bfd3@iki.fi --- src/backend/access/hash/hashutil.c | 4 +- .../modules/index/expected/killtuples.out | 219 ++++++++++++------ src/test/modules/index/specs/killtuples.spec | 15 +- 3 files changed, 163 insertions(+), 75 deletions(-) diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c index 3e16119d027..081adbc88a6 100644 --- a/src/backend/access/hash/hashutil.c +++ b/src/backend/access/hash/hashutil.c @@ -600,7 +600,7 @@ _hash_kill_items(IndexScanDesc scan) * update the page while just holding a share lock. If we * are not allowed, there's no point continuing. */ - if (!BufferBeginSetHintBits(so->currPos.buf)) + if (!BufferBeginSetHintBits(buf)) goto unlock_page; } @@ -621,7 +621,7 @@ _hash_kill_items(IndexScanDesc scan) if (killedsomething) { opaque->hasho_flag |= LH_PAGE_HAS_DEAD_TUPLES; - BufferFinishSetHintBits(so->currPos.buf, true, true); + BufferFinishSetHintBits(buf, true, true); } unlock_page: diff --git a/src/test/modules/index/expected/killtuples.out b/src/test/modules/index/expected/killtuples.out index be7ddd756ef..a3db2c40936 100644 --- a/src/test/modules/index/expected/killtuples.out +++ b/src/test/modules/index/expected/killtuples.out @@ -17,10 +17,10 @@ Index Scan using kill_prior_tuple_btree on kill_prior_tuple (actual rows=1.00 lo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); @@ -33,10 +33,10 @@ Index Scan using kill_prior_tuple_btree on kill_prior_tuple (actual rows=1.00 lo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step delete: DELETE FROM kill_prior_tuple; @@ -51,10 +51,10 @@ Index Scan using kill_prior_tuple_btree on kill_prior_tuple (actual rows=0.00 lo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); @@ -67,10 +67,10 @@ Index Scan using kill_prior_tuple_btree on kill_prior_tuple (actual rows=0.00 lo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 0 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +f (1 row) step drop_table: DROP TABLE IF EXISTS kill_prior_tuple; @@ -93,10 +93,10 @@ Index Scan using kill_prior_tuple_gist on kill_prior_tuple (actual rows=1.00 loo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); @@ -109,10 +109,10 @@ Index Scan using kill_prior_tuple_gist on kill_prior_tuple (actual rows=1.00 loo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step delete: DELETE FROM kill_prior_tuple; @@ -127,10 +127,10 @@ Index Scan using kill_prior_tuple_gist on kill_prior_tuple (actual rows=0.00 loo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); @@ -143,10 +143,10 @@ Index Scan using kill_prior_tuple_gist on kill_prior_tuple (actual rows=0.00 loo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 0 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +f (1 row) step drop_table: DROP TABLE IF EXISTS kill_prior_tuple; @@ -170,10 +170,10 @@ Index Scan using kill_prior_tuple_gist on kill_prior_tuple (actual rows=1.00 loo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); @@ -186,10 +186,10 @@ Index Scan using kill_prior_tuple_gist on kill_prior_tuple (actual rows=1.00 loo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step delete: DELETE FROM kill_prior_tuple; @@ -204,10 +204,10 @@ Index Scan using kill_prior_tuple_gist on kill_prior_tuple (actual rows=0.00 loo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); @@ -220,10 +220,10 @@ Index Scan using kill_prior_tuple_gist on kill_prior_tuple (actual rows=0.00 loo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step drop_table: DROP TABLE IF EXISTS kill_prior_tuple; @@ -246,10 +246,10 @@ Index Scan using kill_prior_tuple_hash on kill_prior_tuple (actual rows=1.00 loo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); @@ -262,10 +262,10 @@ Index Scan using kill_prior_tuple_hash on kill_prior_tuple (actual rows=1.00 loo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step delete: DELETE FROM kill_prior_tuple; @@ -280,10 +280,10 @@ Index Scan using kill_prior_tuple_hash on kill_prior_tuple (actual rows=0.00 loo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); @@ -296,10 +296,85 @@ Index Scan using kill_prior_tuple_hash on kill_prior_tuple (actual rows=0.00 loo (3 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 0 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +f +(1 row) + +step drop_table: DROP TABLE IF EXISTS kill_prior_tuple; + +starting permutation: create_table fill_same create_hash flush disable_seq disable_bitmap measure access flush result measure access flush result delete flush measure access flush result measure access flush result drop_table +step create_table: CREATE TEMPORARY TABLE kill_prior_tuple(key int not null, cat text not null); +step fill_same: INSERT INTO kill_prior_tuple(key, cat) SELECT 1, 'a' FROM generate_series(1, 408) g(i); +step create_hash: CREATE INDEX kill_prior_tuple_hash ON kill_prior_tuple USING hash (key); +step flush: SELECT FROM pg_stat_force_next_flush(); +step disable_seq: SET enable_seqscan = false; +step disable_bitmap: SET enable_bitmapscan = false; +step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); +step access: EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF, BUFFERS OFF) SELECT * FROM kill_prior_tuple WHERE key = 1; +QUERY PLAN +--------------------------------------------------------------------------------------- +Index Scan using kill_prior_tuple_hash on kill_prior_tuple (actual rows=408.00 loops=1) + Index Cond: (key = 1) + Index Searches: 1 +(3 rows) + +step flush: SELECT FROM pg_stat_force_next_flush(); +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t +(1 row) + +step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); +step access: EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF, BUFFERS OFF) SELECT * FROM kill_prior_tuple WHERE key = 1; +QUERY PLAN +--------------------------------------------------------------------------------------- +Index Scan using kill_prior_tuple_hash on kill_prior_tuple (actual rows=408.00 loops=1) + Index Cond: (key = 1) + Index Searches: 1 +(3 rows) + +step flush: SELECT FROM pg_stat_force_next_flush(); +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t +(1 row) + +step delete: DELETE FROM kill_prior_tuple; +step flush: SELECT FROM pg_stat_force_next_flush(); +step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); +step access: EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF, BUFFERS OFF) SELECT * FROM kill_prior_tuple WHERE key = 1; +QUERY PLAN +------------------------------------------------------------------------------------- +Index Scan using kill_prior_tuple_hash on kill_prior_tuple (actual rows=0.00 loops=1) + Index Cond: (key = 1) + Index Searches: 1 +(3 rows) + +step flush: SELECT FROM pg_stat_force_next_flush(); +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t +(1 row) + +step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); +step access: EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF, BUFFERS OFF) SELECT * FROM kill_prior_tuple WHERE key = 1; +QUERY PLAN +------------------------------------------------------------------------------------- +Index Scan using kill_prior_tuple_hash on kill_prior_tuple (actual rows=0.00 loops=1) + Index Cond: (key = 1) + Index Searches: 1 +(3 rows) + +step flush: SELECT FROM pg_stat_force_next_flush(); +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +f (1 row) step drop_table: DROP TABLE IF EXISTS kill_prior_tuple; @@ -326,10 +401,10 @@ Bitmap Heap Scan on kill_prior_tuple (actual rows=0.00 loops=1) (6 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step measure: UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); @@ -345,10 +420,10 @@ Bitmap Heap Scan on kill_prior_tuple (actual rows=0.00 loops=1) (6 rows) step flush: SELECT FROM pg_stat_force_next_flush(); -step result: SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; -new_heap_accesses ------------------ - 1 +step result: SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; +has_new_heap_accesses +--------------------- +t (1 row) step drop_table: DROP TABLE IF EXISTS kill_prior_tuple; diff --git a/src/test/modules/index/specs/killtuples.spec b/src/test/modules/index/specs/killtuples.spec index 77fe8c689a7..3b98ff9f76d 100644 --- a/src/test/modules/index/specs/killtuples.spec +++ b/src/test/modules/index/specs/killtuples.spec @@ -36,12 +36,14 @@ step fill_10 { INSERT INTO kill_prior_tuple(key, cat) SELECT g.i, 'a' FROM gener step fill_500 { INSERT INTO kill_prior_tuple(key, cat) SELECT g.i, 'a' FROM generate_series(1, 500) g(i); } +step fill_same { INSERT INTO kill_prior_tuple(key, cat) SELECT 1, 'a' FROM generate_series(1, 408) g(i); } + # column-less select to make output easier to read step flush { SELECT FROM pg_stat_force_next_flush(); } step measure { UPDATE counter SET heap_accesses = (SELECT heap_blks_read + heap_blks_hit FROM pg_statio_all_tables WHERE relname = 'kill_prior_tuple'); } -step result { SELECT heap_blks_read + heap_blks_hit - counter.heap_accesses AS new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; } +step result { SELECT ((heap_blks_read + heap_blks_hit - counter.heap_accesses) > 0) AS has_new_heap_accesses FROM counter, pg_statio_all_tables WHERE relname = 'kill_prior_tuple'; } step access { EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF, BUFFERS OFF) SELECT * FROM kill_prior_tuple WHERE key = 1; } @@ -116,6 +118,17 @@ permutation measure access flush result drop_table +# Test the hash special case of use of the overflow page due to lots of duplicates +permutation + create_table fill_same create_hash flush + disable_seq disable_bitmap + measure access flush result + measure access flush result + delete flush + measure access flush result + measure access flush result + drop_table + # # Similar to first permutation, except that gin does not have killtuples support permutation create_table fill_500 create_ext_btree_gin create_gin flush