Hi, On Tue, 3 Nov 2026 at 09:41, Bertrand Drouvot <[email protected]> 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
