Re: Confine vacuum skip logic to lazy_scan_skip

On 08/03/2024 02:46, Melanie Plageman wrote:
> On Wed, Mar 06, 2024 at 10:00:23PM -0500, Melanie Plageman wrote:
>> On Wed, Mar 06, 2024 at 09:55:21PM +0200, Heikki Linnakangas wrote:
> I will say that now all of the variable names are *very* long. I didn't
> want to remove the "state" from LVRelState->next_block_state. (In fact, I
> kind of miss the "get". But I had to draw the line somewhere.) I think
> without "state" in the name, next_block sounds too much like a function.
> 
> Any ideas for shortening the names of next_block_state and its members
> or are you fine with them?

Hmm, we can remove the inner struct and add the fields directly into 
LVRelState. LVRelState already contains many groups of variables, like 
"Error reporting state", with no inner structs. I did it that way in the 
attached patch. I also used local variables more.

>> I was wondering if we should remove the "get" and just go with
>> heap_vac_scan_next_block(). I didn't do that originally because I didn't
>> want to imply that the next block was literally the sequentially next
>> block, but I think maybe I was overthinking it.
>>
>> Another idea is to call it heap_scan_vac_next_block() and then the order
>> of the words is more like the table AM functions that get the next block
>> (e.g. heapam_scan_bitmap_next_block()). Though maybe we don't want it to
>> be too similar to those since this isn't a table AM callback.
> 
> I've done a version of this.

+1

> However, by adding a vmbuffer to next_block_state, the callback may be
> able to avoid extra VM fetches from one invocation to the next.

That's a good idea, holding separate VM buffer pins for the 
next-unskippable block and the block we're processing. I adopted that 
approach.

My compiler caught one small bug when I was playing with various 
refactorings of this: heap_vac_scan_next_block() must set *blkno to 
rel_pages, not InvalidBlockNumber, after the last block. The caller uses 
the 'blkno' variable also after the loop, and assumes that it's set to 
rel_pages.

I'm pretty happy with the attached patches now. The first one fixes the 
existing bug I mentioned in the other email (based on the on-going 
discussion that might not how we want to fix it though). Second commit 
is a squash of most of the patches. Third patch is the removal of the 
delay point, that seems worthwhile to keep separate.

-- 
Heikki Linnakangas
Neon (https://neon.tech)