Horton
Menu

Re: Index AM API cleanup

Mark Dilger <mark.dilger@enterprisedb.com>

View thread
From: Mark Dilger <mark.dilger@enterprisedb.com>
To: Peter Eisentraut <peter@eisentraut.org>
Cc: Andrew Dunstan <andrew@dunslane.net>, Kirill Reshke <reshkekirill@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Alex Wang <alex.wang@enterprisedb.com>
Date: 2024-11-27T15:51:15Z
Lists: 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 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

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Relax ordering-related hardcoded btree requirements in planning

  2. Support non-btree indexes in get_actual_variable_range()

  3. Convert PathKey to use CompareType

  4. Generalize index support in network support function

  5. Update a code comment

  6. Allow non-btree unique indexes for matviews

  7. Allow non-btree unique indexes for partition keys

  8. Add some opfamily support functions to lsyscache.c

  9. Simplify and generalize PrepareSortSupportFromIndexRel()

  10. Generalize hash and ordering support in amapi

  11. Drop opcintype from index AM strategy translation API

  12. Allow non-btree speculative insertion indexes

  13. Support non-btree indexes for foreign keys

  14. Integrate GistTranslateCompareType() into IndexAmTranslateCompareType()

  15. Convert strategies to and from compare types

  16. Move CompareType to separate header file

  17. Add get_opfamily_name() function

  18. Rename GistTranslateStratnum() to GistTranslateCompareType()

  19. Change gist stratnum function to use CompareType

  20. Rename RowCompareType to CompareType

  21. Improve slightly misleading internal error message

  22. Track scan reversals in MergeJoin

  23. Track sort direction in SortGroupClause

  24. Add stratnum GiST support function

  25. Add amgettreeheight index AM API routine