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)
