Re: Index AM API cleanup

> On Nov 27, 2024, at 4:57 AM, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> 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.

I kept the name RowCompareType to avoid code churn, but go ahead and rename it if you prefer.

> 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.

Yeah, those two can be combined if you like. I found the distinction between them useful during patch development, but I agree people will have a hard time understanding the difference. For the record, the difference is between "this index doesn't provide the requested functionality" vs. "you passed in an invalid parameter".

> 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?

That was experimental, as the code comment indicates:

+ /*
+ * Assume these have the semantics of =, !=, <, <=, >, >= as their
+ * names imply.
+ *
+ * TODO: Audit the behavior of these RTree strategies as used by spgist
+ * to determine if this assumption is appropriate.
+ */

In hindsight, this patch might have been less confusing if I had simply not provided the callbacks for spgist.

> 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.

The design of the patch allows indexes to simply not participate in the mapping. There's no requirement that an index provide these callbacks. So one approach is to just not touch gist and spgist and leave that for another day. However, if you want to verify that the interface is sufficient to meet gist's needs, then go ahead and try something out.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company