Hi hackers, While working on [1], I noticed that a heap rescan can change whether a buffer access strategy is used while retaining the existing ReadStream. If the strategy is disabled, the stream keeps a dangling pointer after it is freed. If it is enabled, the stream continues using its NULL strategy and the queue capacity selected at creation.
The issue was introduced by b7b0f3f27241. Nazir pointed it out and Yuhang proposed fixing it with a new read_stream_set_strategy() API in [2]. Since the issue is independent of the streaming SampleScan changes, I wonder if it wouldn't make sense to fix it separately, as in the attached? 0001 takes a different approach (than [2]) and recreates the ReadStream when initscan() changes whether a strategy is active. Ordinary rescans continue to reset and reuse the existing stream. It uses the existing ReadStream APIs, which could also make the backpatches easier. 0002 contains a test. I'm not sure it is worth adding to the tree given the amount of setup, but it demonstrates the issue. [1]: https://postgr.es/m/auCYYbDyrQEwIsQq%40bdtpg [2]: https://postgr.es/m/CAN55FZ3Lik2m9J%2BEQjzEHSMSBNgAXU5Myt8LFg%3DD2OQ0i_jUMQ%40mail.gmail.com Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
>From f3b3c20c4291a0767eb50fb22fae0493f602a371 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Mon, 2 Nov 2026 14:18:02 +0000 Subject: [PATCH v1 1/2] Fix ReadStream handling when heap rescan strategy changes b7b0f3f27241 made heap scans keep their ReadStream across rescans. However, initscan() may add or remove rs_strategy when the relation size or caller flags change. ReadStream stores the strategy pointer in its ReadBuffersOperation objects and sizes its buffer queue when it is created. A rescan that enables a bulk read strategy therefore leaves the existing stream using its NULL strategy and old queue capacity. A rescan that disables the strategy frees it while the stream still refers to it, leaving a dangling pointer. Fix this by recreating the stream when the scan switches between using and not using a strategy. When instrumentation is enabled, reattach the accumulated statistics and preserve the maximum capacity across streams. Reported-by: Nazir Bilal Yavuz <[email protected]> Author: Bertrand Drouvot <[email protected]> Reviewed-by: Discussion: https://postgr.es/m/ --- src/backend/access/heap/heapam.c | 119 ++++++++++++++++---------- src/backend/storage/aio/read_stream.c | 3 +- 2 files changed, 76 insertions(+), 46 deletions(-) 91.7% src/backend/access/heap/ 8.2% src/backend/storage/aio/ diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 6a09a1a1517..bd3cb3b6b6c 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -352,6 +352,57 @@ bitmapheap_stream_read_next(ReadStream *pgsr, void *private_data, Assert(false); } +/* + * Start the read stream used by a heap scan. + * + * The caller must call initscan() first, because it sets up the buffer access + * strategy passed to the read stream API. The scan must not already own a read + * stream. + */ +static void +heap_scan_read_stream_begin(HeapScanDesc scan) +{ + Assert(scan->rs_read_stream == NULL); + + if (scan->rs_base.rs_flags & SO_TYPE_SEQSCAN || + scan->rs_base.rs_flags & SO_TYPE_TIDRANGESCAN) + { + ReadStreamBlockNumberCB cb; + + if (scan->rs_base.rs_parallel) + cb = heap_scan_stream_read_next_parallel; + else + cb = heap_scan_stream_read_next_serial; + + /* --- + * It is safe to use batchmode as the only locks taken by `cb` + * are never taken while waiting for IO: + * - SyncScanLock is used in the non-parallel case + * - in the parallel case, only spinlocks and atomics are used + * --- + */ + scan->rs_read_stream = read_stream_begin_relation(READ_STREAM_SEQUENTIAL | + READ_STREAM_USE_BATCHING, + scan->rs_strategy, + scan->rs_base.rs_rd, + MAIN_FORKNUM, + cb, + scan, + 0); + } + else if (scan->rs_base.rs_flags & SO_TYPE_BITMAPSCAN) + { + scan->rs_read_stream = read_stream_begin_relation(READ_STREAM_DEFAULT | + READ_STREAM_USE_BATCHING, + scan->rs_strategy, + scan->rs_base.rs_rd, + MAIN_FORKNUM, + bitmapheap_stream_read_next, + scan, + sizeof(TBMIterateResult)); + } +} + /* ---------------- * initscan - scan code common to heap_beginscan and heap_rescan * ---------------- @@ -403,6 +454,18 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock) else allow_strat = allow_sync = false; + /* + * A read stream keeps the strategy passed at creation and sizes its + * buffer queue accordingly. End it before replacing or freeing that + * strategy. heap_rescan() will create a new stream after the scan is + * reinitialized. + */ + if (scan->rs_read_stream && allow_strat != (scan->rs_strategy != NULL)) + { + read_stream_end(scan->rs_read_stream); + scan->rs_read_stream = NULL; + } + if (allow_strat) { /* During a rescan, keep the previous strategy object. */ @@ -1270,52 +1333,9 @@ heap_beginscan(Relation relation, Snapshot snapshot, else scan->rs_base.rs_key = NULL; - initscan(scan, key, false); - scan->rs_read_stream = NULL; - - /* - * Set up a read stream for sequential scans and TID range scans. This - * should be done after initscan() because initscan() allocates the - * BufferAccessStrategy object passed to the read stream API. - */ - if (scan->rs_base.rs_flags & SO_TYPE_SEQSCAN || - scan->rs_base.rs_flags & SO_TYPE_TIDRANGESCAN) - { - ReadStreamBlockNumberCB cb; - - if (scan->rs_base.rs_parallel) - cb = heap_scan_stream_read_next_parallel; - else - cb = heap_scan_stream_read_next_serial; - - /* --- - * It is safe to use batchmode as the only locks taken by `cb` - * are never taken while waiting for IO: - * - SyncScanLock is used in the non-parallel case - * - in the parallel case, only spinlocks and atomics are used - * --- - */ - scan->rs_read_stream = read_stream_begin_relation(READ_STREAM_SEQUENTIAL | - READ_STREAM_USE_BATCHING, - scan->rs_strategy, - scan->rs_base.rs_rd, - MAIN_FORKNUM, - cb, - scan, - 0); - } - else if (scan->rs_base.rs_flags & SO_TYPE_BITMAPSCAN) - { - scan->rs_read_stream = read_stream_begin_relation(READ_STREAM_DEFAULT | - READ_STREAM_USE_BATCHING, - scan->rs_strategy, - scan->rs_base.rs_rd, - MAIN_FORKNUM, - bitmapheap_stream_read_next, - scan, - sizeof(TBMIterateResult)); - } + initscan(scan, key, false); + heap_scan_read_stream_begin(scan); /* enable read stream instrumentation */ if ((flags & SO_SCAN_INSTRUMENT) && (scan->rs_read_stream != NULL)) @@ -1387,6 +1407,15 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params, * reinitialize scan descriptor */ initscan(scan, key, true); + + if (scan->rs_read_stream == NULL) + { + heap_scan_read_stream_begin(scan); + + if (scan->rs_read_stream && scan->rs_base.rs_instrument) + read_stream_enable_stats(scan->rs_read_stream, + &scan->rs_base.rs_instrument->io); + } } void diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index fddf6ef441d..b6269321aff 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -255,7 +255,8 @@ void read_stream_enable_stats(ReadStream *stream, IOStats *stats) { stream->stats = stats; - if (stream->stats) + if (stream->stats && + stream->stats->distance_capacity < stream->max_pinned_buffers) stream->stats->distance_capacity = stream->max_pinned_buffers; } -- 2.34.1
>From a7b9bca6236a68f77ed2ee18eb08a9081345e768 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Mon, 2 Nov 2026 14:55:40 +0000 Subject: [PATCH v1 2/2] Test heap rescan strategy changes --- src/test/modules/test_misc/meson.build | 1 + .../test_misc/t/016_heap_rescan_strategy.pl | 81 +++++++++++++++++++ 2 files changed, 82 insertions(+) 97.9% src/test/modules/test_misc/t/ diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build index 5d81f5b13be..b5e98fd0233 100644 --- a/src/test/modules/test_misc/meson.build +++ b/src/test/modules/test_misc/meson.build @@ -24,6 +24,7 @@ tests += { 't/013_temp_obj_multisession.pl', 't/014_log_statement_max_length.pl', 't/015_temp_schema_exit_deferrable.pl', + 't/016_heap_rescan_strategy.pl', ], # The injection points are cluster-wide, so disable installcheck 'runningcheck': false, diff --git a/src/test/modules/test_misc/t/016_heap_rescan_strategy.pl b/src/test/modules/test_misc/t/016_heap_rescan_strategy.pl new file mode 100644 index 00000000000..01007a3c737 --- /dev/null +++ b/src/test/modules/test_misc/t/016_heap_rescan_strategy.pl @@ -0,0 +1,81 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# Set a low threshold for selecting a bulk-read strategy. +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +$node->append_conf( + 'postgresql.conf', qq{ +shared_buffers = '1MB' +max_parallel_workers_per_gather = 0 +}); +$node->start; + +$node->safe_psql( + 'postgres', q{ +CREATE TABLE growing AS + SELECT g, repeat('x', 1000) AS payload + FROM generate_series(1, 16) AS g; +CREATE TABLE growth_done (done boolean PRIMARY KEY); + +CREATE FUNCTION grow_table(should_grow boolean) +RETURNS integer +LANGUAGE plpgsql +VOLATILE +AS $$ +DECLARE + inserted boolean; +BEGIN + IF should_grow THEN + INSERT INTO growth_done VALUES (true) + ON CONFLICT DO NOTHING + RETURNING true INTO inserted; + + IF inserted THEN + INSERT INTO growing + SELECT g, repeat('y', 1000) + FROM generate_series(17, 4000) AS g; + END IF; + END IF; + + RETURN 1; +END +$$; +}); + +# Grow the relation during the second scan. The third rescan must recreate +# its read stream with the newly selected bulk-read strategy. +my $bulkread = $node->safe_psql( + 'postgres', q{ +SET enable_material = off; +SET enable_memoize = off; +SELECT count(*) AS scan_count +FROM (VALUES (1, false), (2, true), (3, false)) AS v(ord, should_grow) +CROSS JOIN LATERAL ( + SELECT * + FROM growing + WHERE g >= grow_table(v.should_grow) + OFFSET 0 +) AS s +\gset + +SELECT pg_stat_force_next_flush() +\gset + +SELECT COALESCE(sum(reads), 0) + COALESCE(sum(hits), 0) +FROM pg_stat_get_backend_io(pg_backend_pid()) +WHERE object = 'relation' + AND context = 'bulkread'; +}); + +cmp_ok($bulkread, '>', 0, + 'a rescanned sequential scan uses its newly selected bulk-read strategy'); + +$node->stop; +done_testing(); -- 2.34.1
