Message-ID: <033fd9c8-2efc-4777-bdc6-59c13ec4a64d@enterprisedb.com>
Date: Thu, 25 Jan 2024 17:47:06 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: index prefetching
Content-Language: en-US
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Konstantin Knizhnik <knizhnik@garret.ru>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <8ec36f51-b863-60e3-20e2-b9c981c5ce5e@enterprisedb.com>
 <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>
 <2fda99cb-52b1-432f-b570-d9acbecb42bb@enterprisedb.com>
 <316ca3df-d1b0-49ce-ba05-56b342b4cefa@garret.ru>
 <777e981c-bf0c-4eb9-a9e0-42d677e94327@enterprisedb.com>
 <CAFiTN-vQDbf02UBP1O5C+NoBjp4-yfRg7QJKnWhReLu1rmdCJg@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: 
 <CAFiTN-vQDbf02UBP1O5C+NoBjp4-yfRg7QJKnWhReLu1rmdCJg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/033fd9c8-2efc-4777-bdc6-59c13ec4a64d%40enterprisedb.com>
Precedence: bulk


On 1/25/24 11:45, Dilip Kumar wrote:
> On Wed, Jan 24, 2024 at 11:43 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
> 
>> On 1/22/24 07:35, Konstantin Knizhnik wrote:
>>>
>>> On 22/01/2024 1:47 am, Tomas Vondra wrote:
>>>> h, 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.
>>>>
>>> Why not to add "all_visible" flag to IndexPrefetchEntry ? If will not
>>> cause any extra space overhead (because of alignment), but allows to
>>> avoid dynamic memory allocation (not sure if it is critical, but nice to
>>> avoid if possible).
>>>
>>
> While reading through the first patch I got some questions, I haven't
> read it complete yet but this is what I got so far.
> 
> 1.
> +static bool
> +IndexPrefetchBlockIsSequential(IndexPrefetch *prefetch, BlockNumber block)
> +{
> + int idx;
> ...
> + if (prefetch->blockItems[idx] != (block - i))
> + return false;
> +
> + /* Don't prefetch if the block happens to be the same. */
> + if (prefetch->blockItems[idx] == block)
> + return false;
> + }
> +
> + /* not sequential, not recently prefetched */
> + return true;
> +}
> 
> The above function name is BlockIsSequential but at the end, it
> returns true if it is not sequential, seem like a problem?

Actually, I think it's the comment that's wrong - the last return is
reached only for a sequential pattern (and when the block was not
accessed recently).

> Also other 2 checks right above the end of the function are returning
> false if the block is the same or the pattern is sequential I think
> those are wrong too.
> 

Hmmm. You're right this is partially wrong. There are two checks:

    /*
     * For a sequential pattern, blocks "k" step ago needs to have block
     * number by "k" smaller compared to the current block.
     */
    if (prefetch->blockItems[idx] != (block - i))
        return false;

    /* Don't prefetch if the block happens to be the same. */
    if (prefetch->blockItems[idx] == block)
        return false;

The first condition is correct - we want to return "false" when the
pattern is not sequential.

But the second condition is wrong - we want to skip prefetching when the
block was already prefetched recently, so this should return true (which
is a bit misleading, as it seems to imply the pattern is sequential,
when it's not).

However, this is harmless, because we then identify this block as
recently prefetched in the "full" cache check, so we won't prefetch it
anyway. So it's harmless, although a bit more expensive.

There's another inefficiency - we stop looking for the same block once
we find the first block breaking the non-sequential pattern. Imagine a
sequence of blocks 1, 2, 3, 1, 2, 3, ... in which case we never notice
the block was recently prefetched, because we always find the break of
the sequential pattern. But again, it's harmless, thanks to the full
cache of recently prefetched blocks.

>  2.
>  I have noticed that the prefetch history is maintained at the backend
> level, but what if multiple backends are trying to fetch the same heap
> blocks maybe scanning the same index, so should that be in some shared
> structure?  I haven't thought much deeper about this from the
> implementation POV, but should we think about it, or it doesn't
> matter?

Yes, the cache is at the backend level - it's a known limitation, but I
see it more as a conscious tradeoff.

Firstly, while the LRU cache is at backend level, PrefetchBuffer also
checks shared buffers for each prefetch request. So with sufficiently
large shared buffers we're likely to find it there (and for direct I/O
there won't be any other place to check).

Secondly, the only other place to check is page cache, but there's no
good (sufficiently cheap) way to check that. See the preadv2/nowait
experiment earlier in this thread.

I suppose we could implement a similar LRU cache for shared memory (and
I don't think it'd be very complicated), but I did not plan to do that
in this patch unless absolutely necessary.


regards

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