On Tue, Mar 18, 2025 at 11:12 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> I'm looking at 0001 with the intent of committing it soon. Today I've
> just been studying the test with the injection points.
Now, I've reviewed 0001. I manually validated it does read combining etc.
Few comments:
I know I was the one that advocated calling read_stream_next_buffer()
in the while loop, but I've realized that I think we have to change it
so that we can call vacuum_delay_point() before calling
read_stream_next_buffer(). Otherwise we hold the buffer pin during
vacuum_delay_point(). We'll want to remove it from the top of
btvacuumpage() and add it in this loop:
/* Iterate over pages, then loop back to recheck relation length */
while(BufferIsValid(buf = read_stream_next_buffer(stream, NULL)))
{
BlockNumber current_block = btvacuumpage(&vstate, buf);
if (info->report_progress)
pgstat_progress_update_param(PROGRESS_SCAN_BLOCKS_DONE,
current_block);
}
and before the call to ReadBufferExtended() in the backtrack code path.
buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL,
info->strategy);
Also, I think this comment could use a slight update.
/*
* btvacuumpage --- VACUUM one page
*
* This processes a single page for btvacuumscan(). In some cases we must
* backtrack to re-examine and VACUUM pages that were the scanblkno during
*/
scanblkno is a local variable but the comment was written when it was
a parameter. I think it would be more clear to refer to the passed in
"buf" variable instead.
- Melanie
