On 1/19/24 09:34, Konstantin Knizhnik wrote:
>
> On 18/01/2024 6:00 pm, Tomas Vondra wrote:
>> On 1/17/24 09:45, Konstantin Knizhnik wrote:
>>> I have integrated your prefetch patch in Neon and it actually works!
>>> Moreover, I combined it with prefetch of leaf pages for IOS and it also
>>> seems to work.
>>>
>> Cool! And do you think this is the right design/way to do this?
>
> I like the idea of prefetching TIDs in executor.
>
> But looking though your patch I have some questions:
>
>
> 1. Why it is necessary to allocate and store all_visible flag in data
> buffer. Why caller of IndexPrefetchNext can not look at prefetch field?
>
> + /* store the all_visible flag in the private part of the entry */
> + entry->data = palloc(sizeof(bool));
> + *(bool *) entry->data = all_visible;
>
What you mean by "prefetch field"? The reason why it's done like this is
to only do the VM check once - without keeping the value, we'd have to
do it in the "next" callback, to determine if we need to prefetch the
heap tuple, and then later in the index-only scan itself. That's a
significant overhead, especially in the case when everything is visible.
> 2. Names of the functions `IndexPrefetchNext` and
> `IndexOnlyPrefetchNext` are IMHO confusing because they look similar and
> one can assume that for one is used for normal index scan and last one -
> for index only scan. But actually `IndexOnlyPrefetchNext` is callback
> and `IndexPrefetchNext` is used in both nodeIndexscan.c and
> nodeIndexonlyscan.c
>
Yeah, that's a good point. The naming probably needs rethinking.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company