Re: Confine vacuum skip logic to lazy_scan_skip

On 1/11/24 5:50 PM, Melanie Plageman wrote:
> On Fri, Jan 5, 2024 at 5:51 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>>
>> On Fri, 5 Jan 2024 at 02:25, Jim Nasby <jim.nasby@gmail.com> wrote:
>>>
>>> On 1/4/24 2:23 PM, Andres Freund wrote:
>>>
>>> On 2024-01-02 12:36:18 -0500, Melanie Plageman wrote:
>>>
>>> Subject: [PATCH v2 1/6] lazy_scan_skip remove unnecessary local var rel_pages
>>> Subject: [PATCH v2 2/6] lazy_scan_skip remove unneeded local var
>>>   nskippable_blocks
>>>
>>> I think these may lead to worse code - the compiler has to reload
>>> vacrel->rel_pages/next_unskippable_block for every loop iteration, because it
>>> can't guarantee that they're not changed within one of the external functions
>>> called in the loop body.
>>>
>>> Admittedly I'm not up to speed on recent vacuum changes, but I have to wonder if the concept of skipping should go
awayin the context of vector IO? Instead of thinking about "we can skip this range of blocks", why not maintain a list
of"here's the next X number of blocks that we need to vacuum"?
 
>>
>> Sorry if I misunderstood. AFAIU, with the help of the vectored IO;
>> "the next X number of blocks that need to be vacuumed" will be
>> prefetched by calculating the unskippable blocks ( using the
>> lazy_scan_skip() function ) and the X will be determined by Postgres
>> itself. Do you have something different in your mind?
> 
> I think you are both right. As we gain more control of readahead from
> within Postgres, we will likely want to revisit this heuristic as it
> may not serve us anymore. But the streaming read interface/vectored
> I/O is also not a drop-in replacement for it. To change anything and
> ensure there is no regression, we will probably have to do
> cross-platform benchmarking, though.
> 
> That being said, I would absolutely love to get rid of the skippable
> ranges because I find them very error-prone and confusing. Hopefully
> now that the skipping logic is isolated to a single function, it will
> be easier not to trip over it when working on lazy_scan_heap().

Yeah, arguably it's just a matter of semantics, but IMO it's a lot 
clearer to simply think in terms of "here's the next blocks we know we 
want to vacuum" instead of "we vacuum everything, but sometimes we skip 
some blocks".
-- 
Jim Nasby, Data Architect, Austin TX