Re: Index AM API cleanup - Mailing list pgsql-hackers

> On Nov 27, 2024, at 4:57 AM, Peter Eisentraut <peter@eisentraut.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@gmail.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
suggestto 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
thatand keep it straight.  I also don't really see a reason for having both.  I tried removing one of them, and the
onlything that failed was the new code in AlterOpFamilyAdd() that tries to use strategy_get_rctype() to detect valid
operatornumbers.  But that's new code that is not essential to the purpose of this patch series.  So I strongly suggest
thatwe just have ROWCOMPARE_INVALID (to mirror InvalidStrategy) with value 0.  If there is a reason to have a _NONE
thatis 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
Iagree people will have a hard time understanding the difference.  For the record, the difference is between "this
indexdoesn'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
mappinglogic there.  The current patch series does not provide mapping functions for gist.  We should add those.  They
shouldmake 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
amtranslatestrategyand 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
whatit's doing.  I'm curious why this patch set provides an implementation for spgist but not gist.  Is there anything
interestingabout 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
reportback. 

The design of the patch allows indexes to simply not participate in the mapping.  There's no requirement that an index
providethese callbacks.  So one approach is to just not touch gist and spgist and leave that for another day.  However,
ifyou 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