On Fri, Nov 29, 2024 at 6:20 AM Nazir Bilal Yavuz <byavu...@gmail.com> wrote: > > v4 is attached.
I've started looking at this version. What I don't like about this structure is that we are forced to increment the position in the array of BlockInfoRecords in both the callback and the main loop in autoprewarm_database_main(). There isn't a way around it because we have to return control to the user when we encounter a new relation (we can't close the relation and destroy the read stream in the callback). And in the main loop in autoprewarm_database_main(), we may fail to open the next relation and then need to keep advancing the position in the array of BlockInfoRecords. It isn't just that we have to advance the position in both places -- we also have to have a special case for the first block. All in all, given that in the current read stream API, a single stream must only concern itself with a single relation, fork combo, I think there is no elegant way to deal with this in autoprewarm. One alternative is to loop through the array of BlockInfoRecords and get the start and end positions of the blocks in the arary for a single relation/fork combo. Then we could make the read stream and pass those two positions and the array as callback_private_data. That would mean we loop through the whole array twice, but I wonder if the improvement in clarity is worth it? Some review feedback on your v4: I don't think we need the rs_have_free_buffer per_buffer_data. We can just check have_free_buffers() in both the callback and main loop in autoprewarm_database_main(). I also think you want a comment about why first_block is needed. And I think you need to guard the read_stream_end() after the loop -- what if we never made a read stream because we errored out for all the block's relations or something? - Melanie
