Message-ID: <c115a5ed-e2d6-4811-66fc-1a42c53a3f1b@enterprisedb.com>
Date: Thu, 21 Dec 2023 13:48:09 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: index prefetching
Content-Language: en-US
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Georgios <gkokolatos@protonmail.com>
References: <cf85f46f-b02f-05b2-5248-5000b894ebab@enterprisedb.com>
 <20230609000600.syqy447e6metnvyj@awork3.anarazel.de>
 <a062ac09-c574-1d6a-22b6-f3cc0d28ed42@enterprisedb.com>
 <20230610203456.5gancfekm4pj4pbs@awork3.anarazel.de>
 <6030d836-e8b7-e7b9-2cbb-144309679d03@enterprisedb.com>
 <8c86c3a6-074e-6c88-3e7e-9452b6a37b9b@enterprisedb.com>
 <3cd40425-965a-5ce1-1af3-d51971c44b93@enterprisedb.com>
 <8ec36f51-b863-60e3-20e2-b9c981c5ce5e@enterprisedb.com>
 <06bb7d02-2c44-3062-731e-a735ba13da7e@enterprisedb.com>
 <367160ea-b1ed-4481-e804-bca509128878@enterprisedb.com>
 <cd63c8bc-3d3c-00ab-f7eb-fa55969c3234@enterprisedb.com>
 <CA+Tgmoaf0EHzcfWRydntzqhfc1Nv96C-60fEBbdf1pYZmT0SLA@mail.gmail.com>
 <280dc83c-a16f-4424-1319-95e7e3f798bd@enterprisedb.com>
 <CAFiTN-v1DYMwibRHPnyBtkkQ8-S53G=QXMmpz3sVYgU0w9tsaQ@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <CAFiTN-v1DYMwibRHPnyBtkkQ8-S53G=QXMmpz3sVYgU0w9tsaQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/c115a5ed-e2d6-4811-66fc-1a42c53a3f1b%40enterprisedb.com>
Precedence: bulk


On 12/21/23 07:49, Dilip Kumar wrote:
> On Wed, Dec 20, 2023 at 7:11 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
> I was going through to understand the idea, couple of observations
> 
> --
> + for (int i = 0; i < PREFETCH_LRU_SIZE; i++)
> + {
> + entry = &prefetch->prefetchCache[lru * PREFETCH_LRU_SIZE + i];
> +
> + /* Is this the oldest prefetch request in this LRU? */
> + if (entry->request < oldestRequest)
> + {
> + oldestRequest = entry->request;
> + oldestIndex = i;
> + }
> +
> + /*
> + * If the entry is unused (identified by request being set to 0),
> + * we're done. Notice the field is uint64, so empty entry is
> + * guaranteed to be the oldest one.
> + */
> + if (entry->request == 0)
> + continue;
> 
> If the 'entry->request == 0' then we should break instead of continue, right?
> 

Yes, I think that's true. The small LRU caches are accessed/filled
linearly, so once we find an empty entry, all following entries are
going to be empty too.

I thought this shouldn't make any difference, because the LRUs are very
small (only 8 entries, and I don't think we should make them larger).
And it's going to go away once the cache gets full. But now that I think
about it, maybe this could matter for small queries that only ever hit a
couple rows. Hmmm, I'll have to check.

Thanks for noticing this!

> ---
> /*
>  * Used to detect sequential patterns (and disable prefetching).
>  */
> #define PREFETCH_QUEUE_HISTORY 8
> #define PREFETCH_SEQ_PATTERN_BLOCKS 4
> 
> If for sequential patterns we search only 4 blocks then why we are
> maintaining history for 8 blocks
> 
> ---

Right, I think there's no reason to keep these two separate constants. I
believe this is a remnant from an earlier patch version which tried to
do something smarter, but I ended up abandoning that.

> 
> + *
> + * XXX Perhaps this should be tied to effective_io_concurrency somehow?
> + *
> + * XXX Could it be harmful that we read the queue backwards? Maybe memory
> + * prefetching works better for the forward direction?
> + */
> + for (int i = 1; i < PREFETCH_SEQ_PATTERN_BLOCKS; i++)
> 
> Correct, I think if we fetch this forward it will have an advantage
> with memory prefetching.
> 

OK, although we only really have a couple uint32 values, so it should be
the same cacheline I guess.


regards

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