On Thu, Apr 3, 2025 at 4:22 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> >> I had a quick look at this. Looks good overall
>
> Same here, this seemed like a good piece to bite into with my limited AIO
> knowledge to learn more, and reading it over it seems like a good change.
Thanks for taking a look!
> A few small comments:
>
> + * `pos` is the read stream callback's index into block_info. Because the
> I'm not a fan of markdown in code comments (also in a few more places).
Removed them. I got the idea of doing this to distinguish variable
names in comments from English words. But I can see how it is kind of
distracting -- since it is not common in the codebase.
> + /* Time to try and open our new found relation */
> s/new found/newfound/
Fixed
> + while (p->pos < apw_state->prewarm_stop_idx)
> + {
> + BlockInfoRecord blk = p->block_info[p->pos];
> +
> + CHECK_FOR_INTERRUPTS();
> Isn't checking inside this loop increasing the frequency of checks compared to
> the current version?
It's unclear. The current version does seem to execute the main while
loop (including the CFI) once per block -- even for blocks that it
doesn't end up reading for whatever reason. Things get murkier with
the read stream code. But I put it in the callback to keep the general
idea of doing a CFI once per block. In attached v14, I moved the CFI
to the top of the callback, outside of the loop, to make that
intention more clear.
> + Assert(read_stream_next_buffer(stream, NULL) == InvalidBuffer);
> Is there a non programmer-error case where this can happen? The Assert right
> after a loop around the same function seems to imply there is a race or toctou
> case which if so could use a comment.
Yep. Good call. At some point one read stream user had this assert
because its invocation of read_stream_buffer() was interleaved with
other stuff, so it wasn't obvious that the stream would be exhausted
when it was time to end it. And the assert helped defend that
invariant against future innovation :) I think I've copy-pasta'd this
assert around for no good reason to other read stream users. I've
removed it in v14 and I should probably do a follow-on commit to
master to remove it from the other places it obviously doesn't belong
and is a confusing distraction for future readers.
- Melanie
