Re: Fix ReadStream handling when heap rescan strategy changes

Hi,

On Tue, 3 Nov 2026 at 09:41, Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> 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?

Fixing it separately makes sense to me.

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

The main difference between Yuhang's ([2]) and your approach is that
[2] updates the existing stream through read_stream_set_strategy()
when the strategy is enabled but your approach ends and re-creates the
stream. Both approaches end and re-create the stream if the strategy
is disabled. It seems [2]'s performance is better because of this
reason but I am not sure whether it makes a measurable difference.

Although I didn't find a correctness problem with [2] during my
earlier review, I can see the simplicity benefit here. Recreating the
stream keeps strategy-dependent initialization in one place. That
seems a reasonable approach for backpatching.

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

I haven't reviewed 0002 yet, but I think regression coverage for this
bug would be worthwhile.

Minor readability suggestion:

+ if (scan->rs_read_stream && allow_strat != (scan->rs_strategy != NULL))

We can replace both '!=' with '=='. I find it slightly easier to read,
just a suggestion.

--
Regards,
Nazir Bilal Yavuz
Microsoft