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 > <[email protected]> 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
