Re: Using read_stream in index vacuum

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