Fix use of wrong variable in _hash_kill_items()

In 82467f627b 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 <akuzmenkov@tigerdata.com>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Reported-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://postgr.es/m/vjtmvwvbxt7w5uyacxpzibpj65ewcb7uqaqbhd4arvnjbp5jqz%405ksdh6fsyqve
Discussion: https://postgr.es/m/b9de8d05-3b02-4a27-9b0b-03972fa4bfd3@iki.fi
This commit is contained in:
Andres Freund 2026-03-17 14:54:41 -04:00
parent 01b02c0eca
commit f5eb854ab6
3 changed files with 163 additions and 75 deletions

View file

@ -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:

View file

@ -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;

View file

@ -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