> On 3 Apr 2025, at 22:54, Melanie Plageman <melanieplageman@gmail.com> wrote:
> On Thu, Apr 3, 2025 at 4:22 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>> + 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.
LGTM.
>> + 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.
Makes sense, thanks for clarifying and I agree with removing the assertion.
This patch is already marked Ready for Committer and I concur with that.
--
Daniel Gustafsson
