postgres.git / summary / log / commit / refs

commit    d167c19295da46be1998dd474841f94170e32597
Author:   David Rowley <drowley@postgresql.org>
Date:     Thu Nov 27 23:40:50 2025 +0000

    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()
    does not initialize rs_startblock, 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 <dgrowleyml@gmail.com>
    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;


[parent: c75bf57a90e0]