Re: Fix ReadStream handling when heap rescan strategy changes

Hi,

On Tue, Nov 03, 2026 at 01:23:26PM +0300, Nazir Bilal Yavuz wrote:
> Hi,
>
> On Tue, 3 Nov 2026 at 09:41, Bertrand Drouvot
> <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> >
> > 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.

Thanks for looking at it!

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

OTOH, [2] calls read_stream_set_strategy() on every rescan when the bulk strategy
remains enabled. FWIW, I ran a few benchmarks, and regardless of which approach
was faster, the absolute difference remained below 0.15 microseconds per rescan.
Each measurement timed 3 million rescans, and the results were repeatable across
15 samples.

So I don't think performance strongly favors either approach.

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

+1

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

Yeah, I think the test is worthwhile. It initializes and starts a new cluster,
which is not that cheap (and our test code has been growing quite a bit lately)
but that seems justified here.

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

Yeah, that's probably just a matter of taste. I find the existing links more
directly to "the requested state differs from the current state" but I'm happy
to change it if you feel strongly about it (I don't).

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com