Re: Index AM API cleanup

On 26.11.24 15:27, Andrew Dunstan wrote:
> On 2024-11-19 Tu 6:26 PM, Mark Dilger wrote:
>>> On Nov 16, 2024, at 9:10 AM, Kirill Reshke <reshkekirill(at)gmail(dot)com>
>>> wrote:
>>>
>>> Hi! Can we please have a rebased version of this patch series?
>> Sorry for the delay, and thanks for your interest.  I will try to get
>> around to rebasing in the next few days.
>
> beat you to it :-)

Thanks for that.

I'm content to move forward with the approach of mapping to
RowCompareType, as we had already discussed.

I think, however, that we should rename RowCompareType. Otherwise, it's
just going to be confusing forevermore. I suggest to rename it simply
to CompareType.

What I really don't like is that there is ROWCOMPARE_INVALID *and*
ROWCOMPARE_NONE. No one is going to understand that and keep it
straight. I also don't really see a reason for having both. I tried
removing one of them, and the only thing that failed was the new code in
AlterOpFamilyAdd() that tries to use strategy_get_rctype() to detect
valid operator numbers. But that's new code that is not essential to
the purpose of this patch series. So I strongly suggest that we just
have ROWCOMPARE_INVALID (to mirror InvalidStrategy) with value 0. If
there is a reason to have a _NONE that is separate from that, we need to
think of a different interface.

We should make sure that gist is properly integrated into this
framework, give that we already have strategy number mapping logic
there. The current patch series does not provide mapping functions for
gist. We should add those. They should make use of
GistTranslateStratnum() (and we might need to add an inverse function).
The problem is that gist and GistTranslateStratnum() also need the
opclass, because the strategy numbers are not fixed for gist. So the
amtranslatestrategy and amtranslaterctype callbacks probably need to
expanded to cover that.

It might be that spgist has a similar requirement, I'm not sure. The
code in spgutils.c raises some doubts about what it's doing. I'm
curious why this patch set provides an implementation for spgist but not
gist. Is there anything interesting about spgist for this purpose?

I'm going to try to code up the gist support on top of this patch set to
make sure that it will fit well. I'll report back.