From 1611a32fdda6b8d137b133e04ee83a73d8e08b7e Mon Sep 17 00:00:00 2001 From: David Rowley Date: Fri, 28 Nov 2025 01:47:52 +1300 Subject: [PATCH v1] Fix possibly uninitialized HeapScanDesc.rs_startblock The solution used in 0ca3b1697 to determine the Parallel TID Range Scan's start location was to modify the signature of table_block_parallelscan_startblock_init() to allow the startblock to be passed in as a parameter. This allows the scan limits to be adjusted before that function is called so that the limits are picked up when the parallel scan starts. The commit made it so the call to table_block_parallelscan_startblock_init uses the HeapScanDesc's rs_startblock to pass the startblock to the parallel scan. That all works ok for Parallel TID Range scans as the HeapScanDesc rs_startblock gets set by heap_setscanlimits(), but for Parallel Seq Scans, initscan() has no code path where that's set and that results in passing an uninitialized value to table_block_parallelscan_startblock_init() as noted by the buildfarm member skink, running Valgrind. To fix this issue, make it so initscan() sets the rs_startblock for parallel scans unless we're doing a rescan. This makes it so table_block_parallelscan_startblock_init() will be called with the startblock set to InvalidBlockNumber, and that'll allow the syncscan code to find the correct start location (when enabled). For Parallel TID Range Scans, this InvalidBlockNumber value will be overwritten in the call to heap_setscanlimits(). initscan() is a bit light on documentation on what's meant to get initialized where for parallel scans. From what I can tell, it looks like it just didn't matter prior to 0ca3b1697 that rs_startblock was left uninitialized for parallel scans. To address the light documentation, I've also added some comments to mention that the syncscan location for parallel scans is figured out in table_block_parallelscan_startblock_init. I've also taken the liberty to adjust the if/else if/else code in initscan() to make it clearer which parts apply to parallel scans and which parts are for the serial scans. Author: David Rowley Discussion: https://postgr.es/m/CAApHDvqALm+k7FyfdQdCw1yF_8HojvR61YRrNhwRQPE=zSmnQA@mail.gmail.com --- src/backend/access/heap/heapam.c | 47 ++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 0a820bab87a..4d382a04338 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -415,28 +415,41 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock) scan->rs_base.rs_flags |= SO_ALLOW_SYNC; else scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC; - } - else if (keep_startblock) - { + /* - * When rescanning, we want to keep the previous startblock setting, - * so that rewinding a cursor doesn't generate surprising results. - * Reset the active syncscan setting, though. + * If not rescanning, initialize the startblock. Finding the actual + * start location is done in table_block_parallelscan_startblock_init, + * based on whether an alternative start location has been set with + * heap_setscanlimits, or using the syncscan location, when syncscan + * is enabled. */ - if (allow_sync && synchronize_seqscans) - scan->rs_base.rs_flags |= SO_ALLOW_SYNC; - else - scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC; - } - else if (allow_sync && synchronize_seqscans) - { - scan->rs_base.rs_flags |= SO_ALLOW_SYNC; - scan->rs_startblock = ss_get_location(scan->rs_base.rs_rd, scan->rs_nblocks); + if (!keep_startblock) + scan->rs_startblock = InvalidBlockNumber; } else { - scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC; - scan->rs_startblock = 0; + if (keep_startblock) + { + /* + * When rescanning, we want to keep the previous startblock + * setting, so that rewinding a cursor doesn't generate surprising + * results. Reset the active syncscan setting, though. + */ + if (allow_sync && synchronize_seqscans) + scan->rs_base.rs_flags |= SO_ALLOW_SYNC; + else + scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC; + } + else if (allow_sync && synchronize_seqscans) + { + scan->rs_base.rs_flags |= SO_ALLOW_SYNC; + scan->rs_startblock = ss_get_location(scan->rs_base.rs_rd, scan->rs_nblocks); + } + else + { + scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC; + scan->rs_startblock = 0; + } } scan->rs_numblocks = InvalidBlockNumber; -- 2.43.0