Message-ID: <6a220195-52f8-303c-d154-3f29ac2f078d@iki.fi>
Date: Sun, 9 Jul 2023 21:05:53 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: BRIN indexes vs. SK_SEARCHARRAY (and preprocessing scan keys)
Content-Language: en-US
To: Tomas Vondra <tomas.vondra@enterprisedb.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <f6aab55c-35eb-4118-a7ff-571c62e6cc39@enterprisedb.com>
 <0e1f3350-c9cf-ab62-43a5-5dae314de89c@enterprisedb.com>
 <57020b2e-d9c9-9bc7-4892-b36d9bb07563@enterprisedb.com>
 <327064f1-276e-cb45-88c2-1ff498037cf5@enterprisedb.com>
 <a312b665-79fb-4ee1-e9eb-e8ed9df5479f@iki.fi>
 <4fa33caf-b5d1-6887-65a9-82ce9d92765c@enterprisedb.com>
 <1701e986-fa32-f3fa-0ac8-39fae8e5db35@enterprisedb.com>
 <856edf2c-8e95-0a8f-5a37-c8564a770a82@enterprisedb.com>
 <2484032b-3e6d-ae6a-61e5-732e2136ad18@iki.fi>
 <677c95ad-513f-6702-3c59-0afa2790a149@enterprisedb.com>
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <677c95ad-513f-6702-3c59-0afa2790a149@enterprisedb.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/6a220195-52f8-303c-d154-3f29ac2f078d%40iki.fi>
Precedence: bulk

On 09/07/2023 19:16, Tomas Vondra wrote:
> On 7/8/23 23:57, Heikki Linnakangas wrote:
>> The new preprocess support function feels a bit too inflexible to me.
>> True, you can store whatever you want in the ScanKey that it returns,
>> but since that's the case, why not just make it void * ? It seems that
>> the constraint here was that you need to pass a ScanKey to the
>> consistent function, because the consistent function's signature is what
>> it is. But we can change the signature, if we introduce a new support
>> amproc number for it.
> 
> Now sure I follow - what should be made (void *)? Oh, you mean not
> passing the preprocessed array as a scan key at all, and instead passing
> it as a new (void*) parameter to the (new) consistent function?

Right.

>> It would be unpleasant to force all BRIN opclasses to immediately
>> implement the searcharray-logic. If we don't want to do that, we need to
>> implement generic SK_SEARCHARRAY handling in BRIN AM itself. That would
>> be pretty easy, right? Just call the regular consistent function for
>> every element in the array.
> 
> True, although the question is how many out-of-core opclasses are there.
> My impression is the number is pretty close to 0, in which case we're
> making ourselves to jump through all kinds of hoops, making the code
> more complex, with almost no benefit in the end.

Perhaps. How many of the opclasses can do something smart with 
SEARCHARRAY? If the answer is "all" or "almost all", then it seems 
reasonable to just require them all to handle it. If the answer is 
"some", then it would still be nice to provide a naive default 
implementation in the AM itself. Otherwise all the opclasses need to 
include a bunch of boilerplate to support SEARCHARRAY.

>> If an opclass wants to provide a faster/better implementation, it can
>> provide a new consistent support procedure that supports that. Let's
>> assign a new amproc number for new-style consistent function, which must
>> support SK_SEARCHARRAY, and pass it some scratch space where it can
>> cache whatever per-scankey data. Because it gets a new amproc number, we
>> can define the arguments as we wish. We can pass a pointer to the
>> per-scankey scratch space as a new argument, for example.
>>
>> We did this backwards-compatibility dance with the 3/4-argument variants
>> of the current consistent functions. But I think assigning a whole new
>> procedure number is better than looking at the number of arguments.
> 
> I actually somewhat hate the 3/4-argument dance, and I'm opposed to
> doing that sort of thing again. First, I'm not quite convinced it's
> worth the effort to jump through this hoop (I recall this being one of
> the headaches when fixing the BRIN NULL handling). Second, it can only
> be done once - imagine we now need to add a new optional parameter.
> Presumably, we'd need to keep the existing 3/4 variants, and add new 4/5
> variants. At which point 4 is ambiguous.

My point is that we should assign a new amproc number to distinguish the 
new variant, instead of looking at the number of arguments. That way 
it's not ambiguous, and you can define whatever arguments you want for 
the new variant.

Yet another idea is to introduce a new amproc for a consistent function 
that *only* handles the SEARCHARRAY case, and keep the old consistent 
function as it is for the scalars. So every opclass would need to 
implement the current consistent function, just like today. But if an 
opclass wants to support SEARCHARRAY, it could optionally also provide 
an "consistent_array" function.

> Yes, my previous message was mostly about backwards compatibility, and
> this may seem a bit like an argument against it. But that message was
> more a question "If we do this, is it actually backwards compatible the
> way we want/need?")
> 
> Anyway, I think the BrinDesc scratch space is a neat idea, I'll try
> doing it that way and report back in a couple days.

Cool. In 0005-Support-SK_SEARCHARRAY-in-BRIN-bloom-20230702.patch, you 
used the preprocess function to pre-calculate the scankey's hash, even 
for scalars. You could use the scratch space in BrinDesc for that, 
before doing anything with SEARCHARRAYs.

-- 
Heikki Linnakangas
Neon (https://neon.tech)