On Thu, Feb 13, 2025 at 4:55 PM Melanie Plageman <melanieplage...@gmail.com> wrote: > > Thanks for your review! I've made the changes in attached v18. > > I do want to know what you think we should do about what you brought > up about lazy_check_wraparound_failsafe() -- given my reply (below). > > On Thu, Feb 13, 2025 at 6:08 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > Sorry for the late chiming in. I've reviewed the v16 patch set, and > > the patches mostly look good. Here are some comments mostly about > > cosmetic things: > > > > 0001: > > > > - bool all_visible_according_to_vm, > > - was_eager_scanned = false; > > + uint8 blk_flags = 0; > > > > Can we probably declare blk_flags inside the main loop? > > I've done this in 0002 (can't in 0001 because of it being used inside > the while loop itself).
You're right, thanks. > > > 0002: > > > > In lazy_scan_heap(), we have a failsafe check at the beginning of the > > main loop, which is performed before reading the first block. Isn't it > > better to move this check after scanning a block (especially after > > incrementing scanned_pages)? Otherwise, we would end up calling > > lazy_check_wraparound_failsafe() at the very first loop, which > > previously didn't happen without the patch. Since we already called > > lazy_check_wraparound_failsafe() just before calling lazy_scan_heap(), > > the extra check would not make much sense. > > Yes, I agonized over this a bit. The problem with calling > lazy_check_wraparound_failsafe() (and vacuum_delay_point() especially) > after reading the first block is that read_stream_next_buffer() > returns the buffer pinned. Good point. > We don't want to hang onto that pin for a > long time. But I can't move them to the bottom of the loop after we > release the buffer because some of the code paths don't make it that > far. I don't see a good way other than how I did it or special-casing > block 0. What do you think? How about adding 'vacrel->scanned_pages > 0' to the if statement? Which seems not odd to me. Looking at the 0002 patch, it seems you reverted the change to the following comment: /* * Vacuum the Free Space Map to make newly-freed space visible on - * upper-level FSM pages. Note we have not yet processed blkno. + * upper-level FSM pages. Note we have not yet processed blkno+1. */ I feel that the previous change I saw in v17 is clearer: /* * Vacuum the Free Space Map to make newly-freed space visible on - * upper-level FSM pages. Note we have not yet processed blkno. + * upper-level FSM pages. Note that blkno is the previously + * processed block. */ The rest looks good to me. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
