Message-ID: <2484032b-3e6d-ae6a-61e5-732e2136ad18@iki.fi>
Date: Sun, 9 Jul 2023 00:57:14 +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>
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <856edf2c-8e95-0a8f-5a37-c8564a770a82@enterprisedb.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/2484032b-3e6d-ae6a-61e5-732e2136ad18%40iki.fi>
Precedence: bulk

On 02/07/2023 19:09, Tomas Vondra wrote:
> Here's an updated version of the patch series.
> 
> I've polished and pushed the first three patches with cleanup, tests to
> improve test coverage and so on. I chose not to backpatch those - I
> planned to do that to make future backpatches simpler, but the changes
> ended up less disruptive than expected.
> 
> The remaining patches are just about adding SK_SEARCHARRAY to BRIN.
> 
> 0001 - adds the optional preprocess procedure, calls it from brinrescan
> 
> 0002 to 0005 - adds the support to the existing BRIN opclasses

Could you implement this completely in the consistent-function, by 
caching the sorted array in fn_extra, without adding the new preprocess 
procedure? On first call, when fn_extra == NULL, sort the array and 
stash it in fn_extra.

I don't think that works, because fn_extra isn't reset when the scan 
keys change on rescan. We could reset it, and document that you can use 
fn_extra for per-scankey caching. There's some precedence for not 
resetting it though, see commit d22a09dc70f. But we could provide an 
opaque per-scankey scratch space like that somewhere else. In BrinDesc, 
perhaps.

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.

> The main open question I have is what exactly does it mean that the
> procedure is optional. In particular, should it be supported to have a
> BRIN opclass without the "preprocess" procedure but using the other
> built-in support procedures?
>
> For example, imagine you have a custom BRIN opclass in an extension (for
> a custom data type or something). This does not need to implement any
> procedures, it can just call the existing built-in ones. Of course, this
> won't get the "preprocess" procedure automatically.
> 
> Should we support such opclasses or should we force the extension to be
> updated by adding a preprocess procedure? I'd say "optional" means we
> should support (otherwise it'd not really optional).
> 
> The reason why this matters is that "amsearcharray" is AM-level flag,
> but the support procedure is defined by the opclass. So the consistent
> function needs to handle SK_SEARCHARRAY keys both with and without
> preprocessing.
> 
> That's mostly what I did for all existing BRIN opclasses (it's a bit
> confusing that opclass may refer to both the "generic" minmax or the
> opclass defined for a particular data type). All the opclasses now
> handle three cases:
> 
> 1) scalar keys (just like before, with amsearcharray=fase)
> 
> 2) array keys with preprocessing (sorted array, array of hashes, ...)
> 
> 3) array keys without preprocessing (for compatibility with old
>     opclasses missing the optional preprocess procedure)
> 
> The current code is a bit ugly, because it duplicates a bunch of code,
> because the option (3) mostly does (1) in a loop. I'm confident this can
> be reduced by refactoring and reusing some of the "shared" code.
> 
> The question is if my interpretation of what "optional" procedure means
> is reasonable. Thoughts?
> 
> The other thing is how to test this "compatibility" code. I assume we
> want to have the procedure for all built-in opclasses, so that won't
> exercise it. I did test it by temporarily removing the procedure from a
> couple pg_amproc.dat entries. I guess creating a custom opclass in the
> regression test is the right solution.

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.

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.

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