Re: index prefetching

On 12/21/23 14:27, Andres Freund wrote:
> Hi,
> 
> On 2023-12-09 19:08:20 +0100, Tomas Vondra wrote:
>> But there's a layering problem that I don't know how to solve - I don't
>> see how we could make indexam.c entirely oblivious to the prefetching,
>> and move it entirely to the executor. Because how else would you know
>> what to prefetch?
> 
>> With index_getnext_tid() I can imagine fetching XIDs ahead, stashing
>> them into a queue, and prefetching based on that. That's kinda what the
>> patch does, except that it does it from inside index_getnext_tid(). But
>> that does not work for index_getnext_slot(), because that already reads
>> the heap tuples.
> 
>> We could say prefetching only works for index_getnext_tid(), but that
>> seems a bit weird because that's what regular index scans do. (There's a
>> patch to evaluate filters on index, which switches index scans to
>> index_getnext_tid(), so that'd make prefetching work too, but I'd ignore
>> that here.
> 
> I think we should just switch plain index scans to index_getnext_tid(). It's
> one of the primary places triggering index scans, so a few additional lines
> don't seem problematic.
> 
> I continue to think that we should not have split plain and index only scans
> into separate files...
> 

I do agree with that opinion. Not just because of this prefetching
thread, but also because of the discussions about index-only filters in
a nearby thread.

> 
>> There are other index_getnext_slot() callers, and I don't
>> think we should accept does not work for those places seems wrong (e.g.
>> execIndexing/execReplication would benefit from prefetching, I think).
> 
> I don't think it'd be a problem to have to opt into supporting
> prefetching. There's plenty places where it doesn't really seem likely to be
> useful, e.g. doing prefetching during syscache lookups is very likely just a
> waste of time.
> 
> I don't think e.g. execReplication is likely to benefit from prefetching -
> you're just fetching a single row after all. You'd need a lot of dead rows to
> make it beneficial.  I think it's similar in execIndexing.c.
> 

Yeah, systable scans are unlikely to benefit from prefetching of this
type. I'm not sure about execIndexing/execReplication, it wasn't clear
to me but maybe you're right.

> 
> I suspect we should work on providing executor nodes with some estimates about
> the number of rows that are likely to be consumed. If an index scan is under a
> LIMIT 1, we shoulnd't prefetch. Similar for sequential scan with the
> infrastructure in
> https://postgr.es/m/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2gkvA%40mail.gmail.com
> 

Isn't this mostly addressed by the incremental ramp-up at the beginning?
Even with target set to 1000, we only start prefetching 1, 2, 3, ...
blocks ahead, it's not like we'll prefetch 1000 blocks right away.

I did initially plan to also consider the number of rows we're expected
to need, but I think it's actually harder than it might seem. With LIMIT
for example we often don't know how selective the qual is, it's not like
we can just stop prefetching after the reading the first N tids. With
other nodes it's good to remember those are just estimates - it'd be
silly to be bitten both by a wrong estimate and also prefetching doing
the wrong thing based on an estimate.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company