Message-ID: <2fda99cb-52b1-432f-b570-d9acbecb42bb@enterprisedb.com>
Date: Mon, 22 Jan 2024 00:47:27 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: index prefetching
Content-Language: en-US
To: Konstantin Knizhnik <knizhnik@garret.ru>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <8ec36f51-b863-60e3-20e2-b9c981c5ce5e@enterprisedb.com>
 <280dc83c-a16f-4424-1319-95e7e3f798bd@enterprisedb.com>
 <CA+Tgmob5Nh7sQO8GENzEDSjYJQuKCgfhWtwDLjMtdueFMCh05A@mail.gmail.com>
 <98ba4b25-fae8-c1f4-1597-8093375a1986@enterprisedb.com>
 <20231221134314.wf2rs62d37u62j7t@alap3.anarazel.de>
 <afd17f3c-d496-5583-e93a-5a162d37235f@enterprisedb.com>
 <20231221154352.ijtg6wloa3nowivh@alap3.anarazel.de>
 <e00dac57-0181-4d9c-9eae-71346383e69b@enterprisedb.com>
 <CA+TgmoacmxNQBZsgMPSn9b7oE6QQLOqr_EP=YZTzdfHrqa5dqw@mail.gmail.com>
 <482ec3ff-52ad-415d-96fd-f3832a894023@enterprisedb.com>
 <8969e2cf-6b23-4682-ab56-b3c0aca877ab@garret.ru>
 <214d5bbe-21d7-4a63-b62b-91fe84f281f5@enterprisedb.com>
 <fd13ce28-912f-4851-a9c5-b03a0d794517@garret.ru>
 <731c8bb7-0662-47a5-a782-5cb631916f1d@enterprisedb.com>
 <22dbfb04-24e4-49b1-b3fa-5367c8d171d2@garret.ru>
 <a9ea5843-8d92-4bfe-b999-f1cee84a35dd@enterprisedb.com>
 <41b959cb-f483-413a-ba5e-f1ac4db0ef0b@garret.ru>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <41b959cb-f483-413a-ba5e-f1ac4db0ef0b@garret.ru>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/2fda99cb-52b1-432f-b570-d9acbecb42bb%40enterprisedb.com>
Precedence: bulk


On 1/21/24 20:56, Konstantin Knizhnik wrote:
> 
> On 19/01/2024 2:35 pm, Tomas Vondra wrote:
>>
>> 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"?
> 
> 
> I mean "prefetch" field of IndexPrefetchEntry:
> 
> +
> +typedef struct IndexPrefetchEntry
> +{
> +    ItemPointerData tid;
> +
> +    /* should we prefetch heap page for this TID? */
> +    bool        prefetch;
> +
> 
> You store the same flag twice:
> 
> +        /* prefetch only if not all visible */
> +        entry->prefetch = !all_visible;
> +
> +        /* store the all_visible flag in the private part of the entry */
> +        entry->data = palloc(sizeof(bool));
> +        *(bool *) entry->data = all_visible;
> 
> My question was: why do we need to allocate something in entry->data and
> store all_visible in it, while we already stored !all-visible in
> entry->prefetch.
> 

Ah, right. Well, you're right in this case we perhaps could set just one
of those flags, but the "purpose" of the two places is quite different.

The "prefetch" flag is fully controlled by the prefetcher, and it's up
to it to change it (e.g. I can easily imagine some new logic touching
setting it to "false" for some reason).

The "data" flag is fully controlled by the custom callbacks, so whatever
the callback stores, will be there.

I don't think it's worth simplifying this. In particular, I don't think
the callback can assume it can rely on the "prefetch" flag.


regards

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