Horton
Menu

Thread

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

  1. Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2024-08-21T19:25:02Z

    Hackers,

The index access method API mostly encapsulates the functionality of in-core index types, with some lingering oversights and layering violations.  There has been an ongoing discussion for several release cycles concerning how the API might be improved to allow interesting additional functionality.  That discussion has frequently included patch proposals to support peculiar needs of esoteric bespoke access methods, which have little interest for the rest of the community.

For your consideration, here is a patch series that takes a different approach.  It addresses many of the limitations and layering violations, along with introducing test infrastructure to validate the changes.  Nothing in this series is intended to introduce new functionality to the API.  Any such, "wouldn't it be great if..." type suggestions for the API are out of scope for this work.  On the other hand, this patch set does not purport to fix all such problems; it merely moves the project in that direction.

For validation purposes, the first patch creates shallow copies of hash and btree named "xash" and "xtree" and introduces some infrastructure to run the src/test/regress and src/test/isolation tests against them without needing to duplicate those tests.  Numerous failures like "unexpected non-btree AM" can be observed in the test results.

Also for validation purposes, the second patch creates a deep copy of btree named "treeb" which uses modified copies of the btree implementation rather than using the btree implementation by reference.  This allows for more experimentation, but might be more code than the community wants.  Since this is broken into its own patch, it can be excluded from what eventually gets committed.  Even if we knew a priori that this "treeb" test would surely never be committed, it still serves to help anybody reviewing the patch series to experiment with those other changes without having to construct such a test index AM individually.

The next twenty patches are a mix of fixes of various layering violations, such as not allowing non-core index AMs from use in replica identity full, or for speculative insertion, or for foreign key constraints, or as part of merge join; with updates to the "treeb" code as needed.  The changes to "treeb" are broken out so that they can also easily be excluded from whatever gets committed.

The final commit changes the ordering of the strategy numbers in treeb.  The name "treeb" is a rotation of "btree", and indeed the strategy numbers 1,2,3,4,5 are rotated to 5,1,2,3,4.  The fact that treeb indexes work properly after this change is meant to demonstrate that the core changes have been sufficient to address the prior dependency on btree strategy number ordering.  Again, this doesn't need to be committed; it might only serve to help reviewers in determining if the functional changes are correct.
 
Not to harp on this too heavily, but please note that running the core regression and isolation tests against xash, xtree, and treeb are known not to pass.  That's the point.  But by the end of the patch series, the failures are limited to EXPLAIN output changes; the query results themselves are intended to be consistent with the expected test output.  To avoid breaking `make check-world`, these test modules are not added to the test schedule.  They are also, at least for now, only useable from make, not from meson.

Internal development versions 1..16 not included.  Andrew, Peter, and Alex have all provided reviews internally and are cc'd here.  Patch by me.  Here is v17 for the community:

  2. Re: Index AM API cleanup

    Kirill Reshke <reshkekirill@gmail.com> — 2024-08-21T19:34:40Z

    On Thu, 22 Aug 2024 at 00:25, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
> Hackers,
>
> The index access method API mostly encapsulates the functionality of in-core index types, with some lingering oversights and layering violations.  There has been an ongoing discussion for several release cycles concerning how the API might be improved to allow interesting additional functionality.  That discussion has frequently included patch proposals to support peculiar needs of esoteric bespoke access methods, which have little interest for the rest of the community.
>
> For your consideration, here is a patch series that takes a different approach.  It addresses many of the limitations and layering violations, along with introducing test infrastructure to validate the changes.  Nothing in this series is intended to introduce new functionality to the API.  Any such, "wouldn't it be great if..." type suggestions for the API are out of scope for this work.  On the other hand, this patch set does not purport to fix all such problems; it merely moves the project in that direction.
>
> For validation purposes, the first patch creates shallow copies of hash and btree named "xash" and "xtree" and introduces some infrastructure to run the src/test/regress and src/test/isolation tests against them without needing to duplicate those tests.  Numerous failures like "unexpected non-btree AM" can be observed in the test results.
>
> Also for validation purposes, the second patch creates a deep copy of btree named "treeb" which uses modified copies of the btree implementation rather than using the btree implementation by reference.  This allows for more experimentation, but might be more code than the community wants.  Since this is broken into its own patch, it can be excluded from what eventually gets committed.  Even if we knew a priori that this "treeb" test would surely never be committed, it still serves to help anybody reviewing the patch series to experiment with those other changes without having to construct such a test index AM individually.
>
> The next twenty patches are a mix of fixes of various layering violations, such as not allowing non-core index AMs from use in replica identity full, or for speculative insertion, or for foreign key constraints, or as part of merge join; with updates to the "treeb" code as needed.  The changes to "treeb" are broken out so that they can also easily be excluded from whatever gets committed.
>
> The final commit changes the ordering of the strategy numbers in treeb.  The name "treeb" is a rotation of "btree", and indeed the strategy numbers 1,2,3,4,5 are rotated to 5,1,2,3,4.  The fact that treeb indexes work properly after this change is meant to demonstrate that the core changes have been sufficient to address the prior dependency on btree strategy number ordering.  Again, this doesn't need to be committed; it might only serve to help reviewers in determining if the functional changes are correct.
>
> Not to harp on this too heavily, but please note that running the core regression and isolation tests against xash, xtree, and treeb are known not to pass.  That's the point.  But by the end of the patch series, the failures are limited to EXPLAIN output changes; the query results themselves are intended to be consistent with the expected test output.  To avoid breaking `make check-world`, these test modules are not added to the test schedule.  They are also, at least for now, only useable from make, not from meson.
>
> Internal development versions 1..16 not included.  Andrew, Peter, and Alex have all provided reviews internally and are cc'd here.  Patch by me.  Here is v17 for the community:
>
>
>
> —
> Mark Dilger
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company


Hi! Why is the patch attached as .tar.bz2? Usually raw patches are sent here...

-- 
Best regards,
Kirill Reshke

  3. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2024-08-21T20:09:52Z

    
> On Aug 21, 2024, at 12:34 PM, Kirill Reshke <reshkekirill@gmail.com> wrote:
> 
> Hi! Why is the patch attached as .tar.bz2? Usually raw patches are sent here...

I worried the patch set, being greater than 1 MB, might bounce or be held up in moderation.

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

  4. Re: Index AM API cleanup

    Andrew Dunstan <andrew@dunslane.net> — 2024-08-21T20:16:50Z

    On 2024-08-21 We 4:09 PM, Mark Dilger wrote:
>
>> On Aug 21, 2024, at 12:34 PM, Kirill Reshke<reshkekirill@gmail.com>  wrote:
>>
>> Hi! Why is the patch attached as .tar.bz2? Usually raw patches are sent here...
> I worried the patch set, being greater than 1 MB, might bounce or be held up in moderation.


Yes, it would have required moderation AIUI. It is not at all 
unprecedented to send a compressed tar of patches, and is explicitly 
provided for by the cfbot: see 
<https://wiki.postgresql.org/wiki/Cfbot#Which_attachments_are_considered_to_be_patches.3F>


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

  5. Re: Index AM API cleanup

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-21T21:25:42Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> On Aug 21, 2024, at 12:34 PM, Kirill Reshke <reshkekirill@gmail.com> wrote:
>> Hi! Why is the patch attached as .tar.bz2? Usually raw patches are sent here...

> I worried the patch set, being greater than 1 MB, might bounce or be held up in moderation.

I'm +1 for doing it like this with such a large group of patches.
Separate attachments are nice up to say half a dozen attachments,
but beyond that they're kind of a pain to deal with.

			regards, tom lane

  6. Re: Index AM API cleanup

    Alexandra Wang <alexandra.wang.oss@gmail.com> — 2024-08-22T08:36:31Z

    Hi Mark,

On Wed, Aug 21, 2024 at 2:25 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
>
>
> For validation purposes, the first patch creates shallow copies of hash and btree named "xash" and "xtree" and introduces some infrastructure to run the src/test/regress and src/test/isolation tests against them without needing to duplicate those tests.  Numerous failures like "unexpected non-btree AM" can be observed in the test results.
>
> Also for validation purposes, the second patch creates a deep copy of btree named "treeb" which uses modified copies of the btree implementation rather than using the btree implementation by reference.  This allows for more experimentation, but might be more code than the community wants.  Since this is broken into its own patch, it can be excluded from what eventually gets committed.  Even if we knew a priori that this "treeb" test would surely never be committed, it still serves to help anybody reviewing the patch series to experiment with those other changes without having to construct such a test index AM individually.

Thank you for providing an infrastructure that allows modules to test
by reference and re-use existing regression and isolation tests. I
believe this approach ensures great coverage for the API cleanup.
I was very excited to compare the “make && make check” results in the
test modules - ‘xash,’ ‘xtree,’ and ‘treeb’ - before and after the
series of AM API fixes. Here are my results:

Before the fixes:
- xash: # 20 of 223 tests failed.
- xtree: # 95 of 223 tests failed.
- treeb: # 47 of 223 tests failed.

After the fixes:
- xash: # 21 of 223 tests failed.
- xtree: # 58 of 223 tests failed.
- treeb: # 58 of 223 tests failed.

I expected the series of fixes to eliminate all failed tests, but that
wasn’t the case. It's nice to see the failures for ‘xtree’ have significantly
decreased as the ‘unexpected non-btree AM’ errors have been resolved.
I noticed some of the remaining test failures are due to trivial index
name changes, like hash -> xash and btree -> treeb.

If we keep xtree and xash for testing, is there a way to ensure all
tests pass instead of excluding them from "make check-world"? On that
note, I ran ‘make check-world’ on the final patch, and everything
looks good.

I had to make some changes to the first two patches in order to run
"make check" and compile the treeb code on my machine. I’ve attached
my changes.

"make installcheck" for treeb is causing issues on my end. I can
investigate further if it’s not a problem for others.

Best,
Alex

  7. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2024-08-22T18:28:39Z

    
> On Aug 22, 2024, at 1:36 AM, Alexandra Wang <alexandra.wang.oss@gmail.com> wrote:
> 
> I had to make some changes to the first two patches in order to run
> "make check" and compile the treeb code on my machine. I’ve attached
> my changes.

Thank you for the review, and the patches!


> "make installcheck" for treeb is causing issues on my end. I can
> investigate further if it’s not a problem for others.

The test module index AMs are not intended for use in any installed database, so 'make installcheck' is unnecessary.  A mere 'make check' should suffice.  However, if you want to run it, you can install the modules, edit postgresql.conf to add 'treeb' to shared_preload_libraries, restart the server, and run 'make installcheck'.   This is necessary for 'treeb' because it requests shared memory, and that needs to be done at startup.

The v18 patch set includes the changes your patches suggest, though I modified the approach a bit.  Specifically, rather than standardizing on '1.0.0' for the module versions, as your patches do, I went with '1.0', as is standard in other modules in neighboring directories.  The '1.0.0' numbering was something I had been using in versions 1..16 of this patch, and I only partially converted to '1.0' before posting v17, so sorry about that.  The v18 patch also has some whitespace fixes.

To address your comments about the noise in the test failures, v18 modifies the clone_tests.pl script to do a little more work translating the expected output to expect the module's AM name ("xash", "xtree", "treeb", or whatnot) beyond what that script did in v17.

  8. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2024-08-26T12:21:26Z

    On 21.08.24 21:25, Mark Dilger wrote:
> The next twenty patches are a mix of fixes of various layering
> violations, such as not allowing non-core index AMs from use in replica
> identity full, or for speculative insertion, or for foreign key
> constraints, or as part of merge join; with updates to the "treeb" code
> as needed.  The changes to "treeb" are broken out so that they can also
> easily be excluded from whatever gets committed.

I made a first pass through this patch set.  I think the issues it aims 
to address are mostly legitimate.  In a few cases, we might need some 
more discussion and perhaps will end up slicing the APIs a bit 
differently.  The various patches that generalize the strategy numbers 
appear to overlap with things being discussed at [0], so we should see 
that the solution covers all the use cases.

[0]: 
https://www.postgresql.org/message-id/flat/CA+renyUApHgSZF9-nd-a0+OPGharLQLO=mDHcY4_qQ0+noCUVg@mail.gmail.com

To make a dent, I picked out something that should be mostly harmless: 
Stop calling directly into _bt_getrootheight() (patch 0004).  I think 
this patch is ok, but I might call the API function amgettreeheight 
instead of amgetrootheight.  The former seems more general.

Also, a note for us all in this thread, changes to the index AM API need 
updates to the corresponding documentation in doc/src/sgml/indexam.sgml.

I notice that _bt_getrootheight() is called only to fill in the 
IndexOptInfo tree_height field, which is only used by btcostestimate(), 
so in some sense this is btree-internal data.  But making it so that 
btcostestimate() calls _bt_getrootheight() directly to avoid all that 
intermediate business seems too complicated, and there was probably a 
reason that the cost estimation functions don't open the index.

Interestingly, the cost estimation functions for gist and spgist also 
look at the tree_height field but nothing ever fills it on.  So with 
your API restructuring, someone could provide the missing API functions 
for those index types.  Might be interesting.

That said, there might be value in generalizing this a bit.  If you look 
at the cost estimation functions in pgvector (hnswcostestimate() and 
ivfflatcostestimate()), they both have this pattern that 
btcostestimate() tries to avoid: They open the index, look up some 
number, close the index, then make a cost estimate computation with the 
number looked up.  So another idea would be to generalize the 
tree_height field to some "index size data" or even "internal data for 
cost estimation".  This wouldn't need to change the API much, since 
these are all just integer values, but we'd label the functions and 
fields a bit differently.

  9. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2024-08-26T15:10:34Z

    
> On Aug 26, 2024, at 5:21 AM, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> On 21.08.24 21:25, Mark Dilger wrote:
>> The next twenty patches are a mix of fixes of various layering
>> violations, such as not allowing non-core index AMs from use in replica
>> identity full, or for speculative insertion, or for foreign key
>> constraints, or as part of merge join; with updates to the "treeb" code
>> as needed.  The changes to "treeb" are broken out so that they can also
>> easily be excluded from whatever gets committed.
> 
> I made a first pass through this patch set.

Peter, thanks for the review!

> I think the issues it aims to address are mostly legitimate.  In a few cases, we might need some more discussion and perhaps will end up slicing the APIs a bit differently.  The various patches that generalize the strategy numbers appear to overlap with things being discussed at [0], so we should see that the solution covers all the use cases.
> 
> [0]: https://www.postgresql.org/message-id/flat/CA+renyUApHgSZF9-nd-a0+OPGharLQLO=mDHcY4_qQ0+noCUVg@mail.gmail.com

Paul, it seems what you are doing in v39-0001-Add-stratnum-GiST-support-function.patch is similar to what I am doing in v17-0012-Convert-strategies-to-and-from-row-compare-types.patch.  In particular, your function

+
+/*
+ * Returns the btree number for supported operators, otherwise invalid.
+ */
+Datum
+gist_stratnum_btree(PG_FUNCTION_ARGS)
+{
+ StrategyNumber strat = PG_GETARG_UINT16(0);
+
+ switch (strat)
+ {
+ case RTEqualStrategyNumber:
+ PG_RETURN_UINT16(BTEqualStrategyNumber);
+ case RTLessStrategyNumber:
+ PG_RETURN_UINT16(BTLessStrategyNumber);
+ case RTLessEqualStrategyNumber:
+ PG_RETURN_UINT16(BTLessEqualStrategyNumber);
+ case RTGreaterStrategyNumber:
+ PG_RETURN_UINT16(BTGreaterStrategyNumber);
+ case RTGreaterEqualStrategyNumber:
+ PG_RETURN_UINT16(BTGreaterEqualStrategyNumber);
+ default:
+ PG_RETURN_UINT16(InvalidStrategy);
+ }
+}

looks similar to the implementation of an amtranslate_rctype_function.  Do you have any interest in taking a look?



> To make a dent, I picked out something that should be mostly harmless: Stop calling directly into _bt_getrootheight() (patch 0004).  I think this patch is ok, but I might call the API function amgettreeheight instead of amgetrootheight.  The former seems more general.

Peter, your proposed rename seems fine for the current implementation, but your suggestion below might indicate a different naming.

> I notice that _bt_getrootheight() is called only to fill in the IndexOptInfo tree_height field, which is only used by btcostestimate(), so in some sense this is btree-internal data.  But making it so that btcostestimate() calls _bt_getrootheight() directly to avoid all that intermediate business seems too complicated, and there was probably a reason that the cost estimation functions don't open the index.
> 
> Interestingly, the cost estimation functions for gist and spgist also look at the tree_height field but nothing ever fills it on.  So with your API restructuring, someone could provide the missing API functions for those index types.  Might be interesting.
> 
> That said, there might be value in generalizing this a bit.  If you look at the cost estimation functions in pgvector (hnswcostestimate() and ivfflatcostestimate()), they both have this pattern that btcostestimate() tries to avoid: They open the index, look up some number, close the index, then make a cost estimate computation with the number looked up.  So another idea would be to generalize the tree_height field to some "index size data" or even "internal data for cost estimation".  This wouldn't need to change the API much, since these are all just integer values, but we'd label the functions and fields a bit differently.

Would they be just integers?  They could also be void*, with amgetrootheight_function returning data allocated in the current memory context.  For btree, that would just be a four byte integer, but other indexes could return whatever they like.  If you like that idea, I can code that up for v18, naming the field something like amgetcostestimateinfo_function.

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

  10. Re: Index AM API cleanup

    Andrew Dunstan <andrew@dunslane.net> — 2024-08-26T19:54:33Z

    On 2024-08-22 Th 2:28 PM, Mark Dilger wrote:
>
>> On Aug 22, 2024, at 1:36 AM, Alexandra Wang<alexandra.wang.oss@gmail.com>  wrote:
>>
>> I had to make some changes to the first two patches in order to run
>> "make check" and compile the treeb code on my machine. I’ve attached
>> my changes.
> Thank you for the review, and the patches!
>
>
>> "make installcheck" for treeb is causing issues on my end. I can
>> investigate further if it’s not a problem for others.
> The test module index AMs are not intended for use in any installed database, so 'make installcheck' is unnecessary.  A mere 'make check' should suffice.  However, if you want to run it, you can install the modules, edit postgresql.conf to add 'treeb' to shared_preload_libraries, restart the server, and run 'make installcheck'.   This is necessary for 'treeb' because it requests shared memory, and that needs to be done at startup.
>
> The v18 patch set includes the changes your patches suggest, though I modified the approach a bit.  Specifically, rather than standardizing on '1.0.0' for the module versions, as your patches do, I went with '1.0', as is standard in other modules in neighboring directories.  The '1.0.0' numbering was something I had been using in versions 1..16 of this patch, and I only partially converted to '1.0' before posting v17, so sorry about that.  The v18 patch also has some whitespace fixes.
>
> To address your comments about the noise in the test failures, v18 modifies the clone_tests.pl script to do a little more work translating the expected output to expect the module's AM name ("xash", "xtree", "treeb", or whatnot) beyond what that script did in v17.



Small addition to your clone script, taking into account the existence 
of alternative result files:


diff --git a/src/tools/clone_tests.pl b/src/tools/clone_tests.pl
index c1c50ad90b..d041f93f87 100755
--- a/src/tools/clone_tests.pl
+++ b/src/tools/clone_tests.pl
@@ -183,6 +183,12 @@ foreach my $regress (@regress)
     push (@dst_regress, "$dst_sql/$regress.sql");
     push (@src_expected, "$src_expected/$regress.out");
     push (@dst_expected, "$dst_expected/$regress.out");
+   foreach my $alt (1,2)
+   {
+       next unless -f "$src_expected/${regress}_$alt.out";
+       push (@src_expected, "$src_expected/${regress}_$alt.out");
+       push (@dst_expected, "$dst_expected/${regress}_$alt.out");
+   }
  }
  my @isolation = grep /\w+/, split(/\s+/, $isolation);
  foreach my $isolation (@isolation)


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

  11. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2024-09-03T16:52:35Z

    On 26.08.24 17:10, Mark Dilger wrote:
>> To make a dent, I picked out something that should be mostly harmless: Stop calling directly into _bt_getrootheight() (patch 0004).  I think this patch is ok, but I might call the API function amgettreeheight instead of amgetrootheight.  The former seems more general.
> Peter, your proposed rename seems fine for the current implementation, but your suggestion below might indicate a different naming.
> 
>> I notice that _bt_getrootheight() is called only to fill in the IndexOptInfo tree_height field, which is only used by btcostestimate(), so in some sense this is btree-internal data.  But making it so that btcostestimate() calls _bt_getrootheight() directly to avoid all that intermediate business seems too complicated, and there was probably a reason that the cost estimation functions don't open the index.
>>
>> Interestingly, the cost estimation functions for gist and spgist also look at the tree_height field but nothing ever fills it on.  So with your API restructuring, someone could provide the missing API functions for those index types.  Might be interesting.
>>
>> That said, there might be value in generalizing this a bit.  If you look at the cost estimation functions in pgvector (hnswcostestimate() and ivfflatcostestimate()), they both have this pattern that btcostestimate() tries to avoid: They open the index, look up some number, close the index, then make a cost estimate computation with the number looked up.  So another idea would be to generalize the tree_height field to some "index size data" or even "internal data for cost estimation".  This wouldn't need to change the API much, since these are all just integer values, but we'd label the functions and fields a bit differently.
> Would they be just integers?  They could also be void*, with amgetrootheight_function returning data allocated in the current memory context.  For btree, that would just be a four byte integer, but other indexes could return whatever they like.  If you like that idea, I can code that up for v18, naming the field something like amgetcostestimateinfo_function.

Here is a cleaned-up version of the v17-0004 patch.  I have applied the 
renaming discussed above.  I have also made a wrapper function 
btgettreeheight() that calls _bt_getrootheight().  That way, if we ever 
want to change the API, we don't have to change _bt_getrootheight(), or 
disentangle it then.  I've also added documentation and put in a source 
code comment that the API could be expanded for additional uses.  Also, 
I have removed the addition to the IndexOptInfo struct; that didn't seem 
necessary.

  12. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2024-09-03T17:26:11Z

    
> On Sep 3, 2024, at 9:52 AM, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> Here is a cleaned-up version of the v17-0004 patch.  I have applied the renaming discussed above.  I have also made a wrapper function btgettreeheight() that calls _bt_getrootheight().  That way, if we ever want to change the API, we don't have to change _bt_getrootheight(), or disentangle it then.  I've also added documentation and put in a source code comment that the API could be expanded for additional uses.

Ok.

>  Also, I have removed the addition to the IndexOptInfo struct; that didn't seem necessary.

Good catch.  I agree with the change. 

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

  13. Re: Index AM API cleanup

    Alexandra Wang <alexandra.wang.oss@gmail.com> — 2024-09-04T14:15:06Z

    On Thu, Aug 22, 2024 at 11:28 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> > On Aug 22, 2024, at 1:36 AM, Alexandra Wang <alexandra.wang.oss@gmail.com> wrote:
> > "make installcheck" for treeb is causing issues on my end. I can
> > investigate further if it’s not a problem for others.
>
> The test module index AMs are not intended for use in any installed database, so 'make installcheck' is unnecessary.  A mere 'make check' should suffice.  However, if you want to run it, you can install the modules, edit postgresql.conf to add 'treeb' to shared_preload_libraries, restart the server, and run 'make installcheck'.   This is necessary for 'treeb' because it requests shared memory, and that needs to be done at startup.

Thanks, Mark. This works, and I can run treeb now.

> The v18 patch set includes the changes your patches suggest, though I modified the approach a bit.  Specifically, rather than standardizing on '1.0.0' for the module versions, as your patches do, I went with '1.0', as is standard in other modules in neighboring directories.  The '1.0.0' numbering was something I had been using in versions 1..16 of this patch, and I only partially converted to '1.0' before posting v17, so sorry about that.  The v18 patch also has some whitespace fixes.
>
> To address your comments about the noise in the test failures, v18 modifies the clone_tests.pl script to do a little more work translating the expected output to expect the module's AM name ("xash", "xtree", "treeb", or whatnot) beyond what that script did in v17.

Thanks! I see fewer failures now.

There are a few occurrences of the following error when I run "make
check" in the xtree module:

+ERROR:  bogus RowCompare index qualification

I needed to specify `amroutine->amcancrosscompare = true;` in xtree.c
to eliminate them, as below:

diff --git a/src/test/modules/xtree/access/xtree.c
b/src/test/modules/xtree/access/xtree.c
index bd472edb04..960966801d 100644
--- a/src/test/modules/xtree/access/xtree.c
+++ b/src/test/modules/xtree/access/xtree.c
@@ -33,6 +33,7 @@ xtree_indexam_handler(PG_FUNCTION_ARGS)
        amroutine->amoptsprocnum = BTOPTIONS_PROC;
        amroutine->amcanorder = true;
        amroutine->amcanhash = false;
+       amroutine->amcancrosscompare = true;
        amroutine->amcanorderbyop = false;
        amroutine->amcanbackward = true;
        amroutine->amcanunique = true;

After adding that, the regression.diffs for the xtree and treeb
modules are identical in content, with only plan diffs remaining. I
think this change should be either part of
v18-0008-Generalize-hash-and-ordering-support-in-amapi.patch or a
separate commit, similar to
v18-0009-Adjust-treeb-to-use-amcanhash-and-amcancrosscomp.patch.

Best,
Alex

  14. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2024-09-10T08:10:31Z

    On 03.09.24 19:26, Mark Dilger wrote:
>> On Sep 3, 2024, at 9:52 AM, Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> Here is a cleaned-up version of the v17-0004 patch.  I have applied the renaming discussed above.  I have also made a wrapper function btgettreeheight() that calls _bt_getrootheight().  That way, if we ever want to change the API, we don't have to change _bt_getrootheight(), or disentangle it then.  I've also added documentation and put in a source code comment that the API could be expanded for additional uses.
> 
> Ok.
> 
>>   Also, I have removed the addition to the IndexOptInfo struct; that didn't seem necessary.
> 
> Good catch.  I agree with the change.

I have committed this patch.  (It needed another small change to silence 
a compiler warning about an uninitialized variable.)

  15. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2024-09-24T08:50:32Z

    Next, I have reviewed patches

v17-0010-Track-sort-direction-in-SortGroupClause.patch
v17-0011-Track-scan-reversals-in-MergeJoin.patch

Both of these seem ok and sensible to me.

They take the concept of the "reverse" flag that already exists in the 
affected code and just apply it more consistently throughout the various 
code layers, instead of relying on strategy numbers as intermediate 
storage.  This is both helpful for your ultimate goal in this patch 
series, and it also makes the affected code areas simpler and more 
consistent and robust.

  16. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2024-09-24T09:09:41Z

    
> On Sep 24, 2024, at 10:50 AM, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> Next, I have reviewed patches
> 
> v17-0010-Track-sort-direction-in-SortGroupClause.patch
> v17-0011-Track-scan-reversals-in-MergeJoin.patch
> 
> Both of these seem ok and sensible to me.
> 
> They take the concept of the "reverse" flag that already exists in the affected code and just apply it more consistently throughout the various code layers, instead of relying on strategy numbers as intermediate storage.  This is both helpful for your ultimate goal in this patch series, and it also makes the affected code areas simpler and more consistent and robust.
> 

Thanks for the review!

Yes, I found the existing use of a btree strategy number rather than a boolean "reverse" flag made using the code from other index AMs needlessly harder.  I am glad you see it the same way.

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

  17. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2024-10-14T14:31:00Z

    On 24.09.24 11:09, Mark Dilger wrote:
> 
> 
>> On Sep 24, 2024, at 10:50 AM, Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> Next, I have reviewed patches
>>
>> v17-0010-Track-sort-direction-in-SortGroupClause.patch
>> v17-0011-Track-scan-reversals-in-MergeJoin.patch
>>
>> Both of these seem ok and sensible to me.
>>
>> They take the concept of the "reverse" flag that already exists in the affected code and just apply it more consistently throughout the various code layers, instead of relying on strategy numbers as intermediate storage.  This is both helpful for your ultimate goal in this patch series, and it also makes the affected code areas simpler and more consistent and robust.
>>
> 
> Thanks for the review!
> 
> Yes, I found the existing use of a btree strategy number rather than a boolean "reverse" flag made using the code from other index AMs needlessly harder.  I am glad you see it the same way.

committed

  18. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2024-10-30T10:54:22Z

    I want to call out one particular aspect that is central to this patch
series that needs more broader discussion and agreement.

The problem is that btree indexes (and to a lesser extent hash
indexes) are hard-coded in various places.  This includes various
places in the optimizer (see patch v17-0018) that might be harder to
understand, but also more easy-to-understand places such as:

- Only hash and btree are supported for replica identity.  (see patch 
v17-0015)

- Only btree is supported for speculative insertion.  (see patch v17-0016)

- Only btree is supported for foreign keys.  (see patch v17-0017)

- Only btree can participate in merge join.  (see patch v17-0020)

The problem in cases such as these, and some of them actually contain
code comments about this, is that we somehow need to know what the
equality operator (and in some cases ordering operator) for a type is.
And the way this is currently done is that the btree and hash strategy
numbers are hardcoded, and other index AMs are just not supported
because there is no way to find that out from those.

We faced a similar issue for the temporal primary keys feature.  One
thing this feature had to overcome is that

- Only btree is supported for primary keys.

For that feature, we wanted to add support for gist indexes, and we
had to again find a way to get the equals operator (and also the
overlaps operator).

The solution we ended up with there is to add a support function to
gist that translates the strategy numbers used by the operator class
to "well-known" strategy numbers that we can understand (commit
7406ab623fe).  It later turned out that this problem actually might be
a subset of the general problem being discussed here, so we can also
change that solution if we find a more general solution here.

So the question is, how do we communicate these fundamental semantics
of operators?

The solution we discussed for temporal primary keys is that we just
mandate that certain strategy numbers have fixed meanings.  This is
how btree and hash work anyway, but this was never required for gist.
And there are many gist opclass implementations already out there that
don't use compatible strategy numbers, so that solution would not have
worked (at least without some upgrading pain).  Hence the idea of
mapping the actual strategy numbers to a fixed set of well-known ones.

For the general case across all index AMs, we could similarly decree
that all index AMs use certain strategy numbers for fixed purposes.
But again, this already wouldn't work for gist and perhaps others
already out there.  So here again we need some kind of mapping to
numbers with a fixed purpose.  Or perhaps new index AMs can start out
with the "correct" numbers and only some existing ones would need the
mapping.

And then what do you map them to?

One idea would be to map them to the existing btree numbers.  This
could work, but I always found it kind of confusing that you'd then
have multiple sets of strategy numbers in play, one native to the
index AM or opclass, and one sort of global one.

Another idea is to map them to another thing altogether, a new enum.
This is what this patch series does, essentially.  But it turns out
(so argues the patch series), that this enum already exists, namely

typedef enum RowCompareType
{
     /* Values of this enum are chosen to match btree strategy numbers */
     ROWCOMPARE_LT = 1,          /* BTLessStrategyNumber */
     ROWCOMPARE_LE = 2,          /* BTLessEqualStrategyNumber */
     ROWCOMPARE_EQ = 3,          /* BTEqualStrategyNumber */
     ROWCOMPARE_GE = 4,          /* BTGreaterEqualStrategyNumber */
     ROWCOMPARE_GT = 5,          /* BTGreaterStrategyNumber */
     ROWCOMPARE_NE = 6,          /* no such btree strategy */
} RowCompareType;

which in spite of the name, yes, sounds like exactly that kind of
thing.  And it's also used in many places for this kind of thing
already.

The patch series adds index AM callbacks

     amtranslatestrategy
     amtranslaterctype

to convert between strategy number and RowCompareType, and also adds

     ROWCOMPARE_NONE
     ROWCOMPARE_INVALID

to handle cases where there is no mapping.  I suppose for the temporal
PK use, we would need to add something like ROWCOMPARE_OVERLAPS.

So this is the idea.  To take a step back, I can see the following
options:

1. Require all index AMs to use btree-compatible strategy numbers.
    (Previously rejected, too much upgrading mess.)

2. Provide mapping between index AM strategy numbers and btree strategy
    numbers.  (This doesn't have a space for non-btree operations like
    overlaps.)

3. Provide mapping between index AM strategy numbers and the existing
    RT*StrategyNumber numbers.  (We can expand the set as we want.)
    (This is what the existing mapping function for gist does.)

4. Provide mapping between index AM strategy numbers and some
    completely new set of numbers/symbols.

5. Provide mapping between index AM strategy numbers and
    RowCompareType (with some small extensions).  This is what this
    patch does.

Other ideas?  Thoughts?

  19. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2024-10-31T10:01:57Z

    
> On Oct 30, 2024, at 12:54 PM, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> So this is the idea.  To take a step back, I can see the following
> options:
> 
> 1. Require all index AMs to use btree-compatible strategy numbers.
>   (Previously rejected, too much upgrading mess.)
> 
> 2. Provide mapping between index AM strategy numbers and btree strategy
>   numbers.  (This doesn't have a space for non-btree operations like
>   overlaps.)

I agree that neither of these options are good, for the reasons you mention.

> 3. Provide mapping between index AM strategy numbers and the existing
>   RT*StrategyNumber numbers.  (We can expand the set as we want.)
>   (This is what the existing mapping function for gist does.)

The point of such a mapping is that core code outside any index AM can know what an AM is claiming it can do when it advertises that it implements one of these strategy numbers.  We don't need any new entries in that mapping until some core feature depends on the semantics of the new entry.  Right now, all six of the btree comparators (including not-equal) have semantics that are used outside AMs by functions like match_clause_to_ordering_op().  If any index AM author comes along and creates an index AM which purports to provide these six semantics but actually does something semantically inconsistent with what the backend thinks these mean, that index AM is totally at fault when, for example, ORDER BY returns the wrong results.

On the other hand, if we add RTOverlapStrategyNumber to the common framework of strategy numbers, without anything outside an index AM depending on that, how is an index AM author to know exactly how an "overlaps" operator is supposed to behave?  No doubt, brin, gist, spgist, and friends all have their own understanding of what RTOverlapStrategyNumber means, but how is a new index AM supposed to know if it has analogized that concept correctly to its own operator?  And if several major versions later, you come along to create some feature, let's say a logical replication feature depending on "overlaps" semantics, how are you to know whether all the index AMs in the wild which advertise they provide an "overlaps" operator will work correctly with your new feature?  When logical replication breaks, who is at fault?  Perversely, knowing that RTOverlapStrategyNumber is already in the list, you would likely implement the new logical replication feature on some new strategy number, perhaps naming it RTLogicalOverlapStrategyNumber, to avoid such conflicts.

The RT*StrategyNumber list is much too long, containing many such problematic entries.  We should not, in my opinion, add these to the list prior to some new feature which depends on them, such as a planner or executor optimization.

> 4. Provide mapping between index AM strategy numbers and some
>   completely new set of numbers/symbols.

This is fine, if the new set is sufficiently restricted.  However, as mentioned below, the set of sufficiently restricted values is identical to what we currently define as a RowCompareType.  It creates needless code churn to throw that away and replace it with a new name.

> 5. Provide mapping between index AM strategy numbers and
>   RowCompareType (with some small extensions).  This is what this
>   patch does.

As the patch author, obviously this is the one I chose.  The "small extensions" are just to handle "no such value" type cases.


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

  20. Re: Index AM API cleanup

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-16T17:10:00Z

    On Thu, 31 Oct 2024 at 15:02, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
>
>
> > On Oct 30, 2024, at 12:54 PM, Peter Eisentraut <peter@eisentraut.org> wrote:
> >
> > So this is the idea.  To take a step back, I can see the following
> > options:
> >
> > 1. Require all index AMs to use btree-compatible strategy numbers.
> >   (Previously rejected, too much upgrading mess.)
> >
> > 2. Provide mapping between index AM strategy numbers and btree strategy
> >   numbers.  (This doesn't have a space for non-btree operations like
> >   overlaps.)
>
> I agree that neither of these options are good, for the reasons you mention.
>
> > 3. Provide mapping between index AM strategy numbers and the existing
> >   RT*StrategyNumber numbers.  (We can expand the set as we want.)
> >   (This is what the existing mapping function for gist does.)
>
> The point of such a mapping is that core code outside any index AM can know what an AM is claiming it can do when it advertises that it implements one of these strategy numbers.  We don't need any new entries in that mapping until some core feature depends on the semantics of the new entry.  Right now, all six of the btree comparators (including not-equal) have semantics that are used outside AMs by functions like match_clause_to_ordering_op().  If any index AM author comes along and creates an index AM which purports to provide these six semantics but actually does something semantically inconsistent with what the backend thinks these mean, that index AM is totally at fault when, for example, ORDER BY returns the wrong results.
>
> On the other hand, if we add RTOverlapStrategyNumber to the common framework of strategy numbers, without anything outside an index AM depending on that, how is an index AM author to know exactly how an "overlaps" operator is supposed to behave?  No doubt, brin, gist, spgist, and friends all have their own understanding of what RTOverlapStrategyNumber means, but how is a new index AM supposed to know if it has analogized that concept correctly to its own operator?  And if several major versions later, you come along to create some feature, let's say a logical replication feature depending on "overlaps" semantics, how are you to know whether all the index AMs in the wild which advertise they provide an "overlaps" operator will work correctly with your new feature?  When logical replication breaks, who is at fault?  Perversely, knowing that RTOverlapStrategyNumber is already in the list, you would likely implement the new logical replication feature on some new strategy number, perhaps naming it RTLogicalOverlapStrategyNumber, to avoid such conflicts.
>
> The RT*StrategyNumber list is much too long, containing many such problematic entries.  We should not, in my opinion, add these to the list prior to some new feature which depends on them, such as a planner or executor optimization.
>
> > 4. Provide mapping between index AM strategy numbers and some
> >   completely new set of numbers/symbols.
>
> This is fine, if the new set is sufficiently restricted.  However, as mentioned below, the set of sufficiently restricted values is identical to what we currently define as a RowCompareType.  It creates needless code churn to throw that away and replace it with a new name.
>
> > 5. Provide mapping between index AM strategy numbers and
> >   RowCompareType (with some small extensions).  This is what this
> >   patch does.
>
> As the patch author, obviously this is the one I chose.  The "small extensions" are just to handle "no such value" type cases.
>
>
> —
> Mark Dilger
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


Hi! Can we please have a rebased version of this patch series?

-- 
Best regards,
Kirill Reshke

  21. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2024-11-19T23:26:09Z

    
> 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.
 
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  22. Re: Index AM API cleanup

    Andrew Dunstan <andrew@dunslane.net> — 2024-11-26T14:27:41Z

    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 :-)


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  23. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2024-11-27T12:57:57Z

    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.

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.

  24. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2024-11-27T15:51:15Z

    
> 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

  25. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-04T14:49:20Z

    On 27.11.24 13:57, Peter Eisentraut wrote:
> 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'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.

Here is a patch set in that direction.  It renames RowCompareType to 
CompareType and updates the surrounding commentary a bit.  And then I'm 
changing the gist strategy mapping to use the CompareType values instead 
of the RT* strategy numbers.  Seeing this now, I like this a lot better 
than what we have now, because it makes it clearer in the API and the 
code what is a real strategy number and what's a different kind of 
thing.  (This isn't entirely the above-mentioned integration of the gist 
support into your patch set yet, but it's a meaningful part of it.)

  26. Re: Index AM API cleanup

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-01-05T23:20:07Z

    On 8/26/24 08:10, Mark Dilger wrote:
 > Paul, it seems what you are doing in v39-0001-Add-stratnum-GiST-support-function.patch is similar 
to what I am doing in v17-0012-Convert-strategies-to-and-from-row-compare-types.patch.

Thank you inviting me to share some thoughts here! The goals of this patch make a lot of sense to 
me. I'm very interested in making the non-btree AMs more useful, both to users and to Postgres core 
itself. Here is a breakdown of what I think so far:

- Strategy Numbers
- Can you create unique GiST indexes or not?
- Updating opclasses
- Logical replication
- Empty ranges

Most of this is pretty general and not a line-by-line review of the patches. I'd like to do that 
too, but this email is already long and late.


# Strategy Numbers

I think this is the latest re strategy numbers:

On 12/4/24 06:49, Peter Eisentraut wrote:
 > On 27.11.24 13:57, Peter Eisentraut wrote:
 >> 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'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.
 >
 > Here is a patch set in that direction.  It renames RowCompareType to CompareType and updates the
 > surrounding commentary a bit.  And then I'm changing the gist strategy mapping to use the
 > CompareType values instead of the RT* strategy numbers.  Seeing this now, I like this a lot better
 > than what we have now, because it makes it clearer in the API and the code what is a real strategy
 > number and what's a different kind of thing.  (This isn't entirely the above-mentioned integration
 > of the gist support into your patch set yet, but it's a meaningful part of it.)

I see that this supports COMPARE_OVERLAPS and COMPARE_CONTAINED_BY, which provides most of what 
foreign keys require. Great!

But I also need the intersect operator (*). There is no stratnum for that. Even if I added a 
stratnum, it is not really about an *index*. With my current approach, that made me uncomfortable 
using pg_amop, since I'd need to invent a new amoppurpose. Intersect isn't used for search or 
ordering, just for foreign keys (and later for temporal update/delete). Similarly, This new enum is 
COMPARE_*, and intersect isn't a comparison.

Is there anything we can do about that? Is pg_amop really only for indexes, or can I add a new 
amoppurpose and use it? And does this enum need to be only for indexes, or can it be for other 
things too? Maybe instead of COMPARE_* it can be OPERATION_*.

Today I just hardcode the built-in intersect operator for ranges and multiranges, but I would rather 
have a solution that lets people define foreign keys on arbitrary types. Temporal primary keys 
support arbitrary types (well other than the empty-range issue, which is something I think we can 
resolve), as do temporal UPDATE and DELETE, so getting foreign keys to support them would complete 
the picture. To me this goal is more faithful to Postgres's heritage as an "object-relational" 
database, a philosophy that has made it so extensible.

As I mentioned, UPDATE/DELETE FOR PORTION OF also needs intersect. But it only needs a proc, not an 
operator. For that I added another GiST support proc, intersect, that is just the same proc that 
implements the operator. But if foreign keys had a way to get an operator for any type, then FOR 
PORTION OF could use that too, because an operator implies a proc (but not vice versa).
Then I'd need one less support function.

Or here is another approach: I was thinking that instead of a `stratnum` support proc, used to get 
strategy numbers (whether by well-known stratnum or by COMPARE_*), we could have a support proc that 
*returns operator oids* (again, whether by well-known strategy number or by COMPARE_*). Instead of 
`stratnum` we could call it something like `get_operator`. I can't think of any other use for 
strategy numbers than looking up operators. The existing callsites of GistTranslateStratnum all 
immediately call get_opfamily_member (and Peter's patches don't change that). What if we had a 
higher-level wrapper of get_opfamily_member that took the same parameters, but used this new support 
function (if defined) when called for non-btree AMs?

Then (1) foreign keys could use it get the intersect operator (2) I could drop the FOR PORTION OF 
support proc. (The stratnum support proc only exists in v18devel, so it's still easy to change.)

One problem is: how do extension authors know the oids of their operators? I guess they could look 
them up in pg_operator (via pg_depend?) But we should acknowledge that it's harder for them to get 
oids than for core. Or maybe this isn't such a big deal after all: pg_operator is unique on 
(oprname, oprleft, oprright, oprnamespace).

Are there other problems? It does seem like a risk to have a second "source of truth", especially 
one that is implemented as code rather than data. What if the code changes? But still I like this 
approach. Non-btree indexes can't really use pg_amop anyway, because amopstrategy doesn't mean 
anything. I'm happy to make a commit replacing the stratnum support proc with get_operator as I've 
described it.

But then what is its input? If it's a stratnum, there is no problem: I'll add an 
RTIntersectStrategyNumber. If it's a COMPARE_*, then we still want a non-index-specific name for 
this enum.

Either approach to getting an intersect operator seems fine to me: using pg_amop with a third 
amoppurpose, or replacing stratnum with get_operator. But both seem to expand an index-specific 
abstraction to include something new. What do you think?


# Can you create unique GiST indexes or not?

Postgres lets you ask about the capabilities of different AMs. For instance:

		postgres=# select amname, pg_indexam_has_property(oid, 'can_unique') from pg_am where amtype = 'i';
		 amname | pg_indexam_has_property
		--------+-------------------------
		 btree  | t
		 hash   | f
		 gist   | f
		 gin    | f
		 spgist | f
		 brin   | f
		(6 rows)

So if GiST can't support unique indexes, why can you do this?:

		postgres=# create extension btree_gist;
		CREATE EXTENSION
		postgres=# create table t (id int, valid_at daterange, unique (id, valid_at without overlaps));
		CREATE TABLE
		postgres=# \d t
											 Table "public.t"
			Column  |   Type    | Collation | Nullable | Default
		----------+-----------+-----------+----------+---------
		 id       | integer   |           |          |
		 valid_at | daterange |           |          |
		Indexes:
				"t_id_valid_at_key" UNIQUE (id, valid_at WITHOUT OVERLAPS)

And also:

		postgres=# select indisunique from pg_index where indexrelid = 't_id_valid_at_key'::regclass;
		 indisunique
		-------------
		 t

But:

		postgres=# create unique index idx_t on t using gist (id, valid_at);
		ERROR:  access method "gist" does not support unique indexes

It seems like we have an inconsistency, as Matthew van de Meent brought up here (cc'd): 
https://www.postgresql.org/message-id/CAEze2WiD%2BU1BuJDLGL%3DFXxa8hDxNALVE6Jij0cNXjp10Q%3DnZHw%40mail.gmail.com

The reason is that GiST *might* support unique indexes, but it depends on the opclass. (And note 
GiST has supported effectively-unique indexes via exclusion constraints for a long time.)

 From another perspective, the error message is correct: you can't create a unique GiST index using 
CREATE UNIQUE INDEX. You can only create a UNIQUE/PRIMARY KEY *constraint*, which creates a unique 
GiST index to implement it. But that is not a very satisfying answer.

But since we are improving stratnums, we could allow the above command, even with what is already 
committed today: just see if we can find an equals operator, and create the index. I'm interested in 
working on that myself, but the other temporal patches are a higher priority.

In the meantime perhaps we can improve the error message to sound less contradictory. How about 
"access method 'gist' does not support unique indexes without a WITHOUT OVERLAPS constraint"?

And anyway, creating simply-unique GiST indexes is not very valuable, when you could just create a 
btree index instead. The main reason is legalistic, so we aren't guilty of contradicting our error 
message. It doesn't satisfy Matthew's use-case. He wants to avoid long exclusive locks by doing 
CREATE INDEX CONCURRENTLY then ADD CONSTRAINT USING INDEX. But then you'd need to CREATE UNIQUE 
INDEX with overlaps for the last element, not equals. There is no syntax for that. Perhaps we could 
borrow the syntax for constraints: CREATE UNIQUE INDEX idx ON t USING gist (id, valid_at WITHOUT 
OVERLAPS). It's not part of the standard though, and it might have parse conflicts with COLLATE or 
an opclass name.

But that *still* doesn't get there, because (1) the index has to enforce (temporal) uniqueness 
before you add the constraint (2) it needs to allow CONCURRENTLY. Exclusion constraints don't 
enforce their checks while updating the index, but with a separate search (IIRC), and the details 
would probably have to move from pg_constraint.conexclop to pg_index.indexclop (which doesn't exist 
today). To pre-create *any* exclusion constraint index, we'd need a more general syntax than WITHOUT 
OVERLAPS. Something like (id WITH =, valid_at WITH &&).

And then that has to work with CONCURRENTLY. You *can* create a GiST index CONCURRENTLY today, but 
not one that enforces anything. There was work already to allow CONCURRENTLY to REINDEX exclusion 
constraint indexes, but I assume it's tricky, because it has stalled twice:
see 
https://www.postgresql.org/message-id/flat/CAB7nPqS%2BWYN021oQHd9GPe_5dSVcVXMvEBW_E2AV9OOEwggMHw%40mail.gmail.com#e1a372074cfdf37bf9e5b4e29ddf7b2d 
from 2012 and 
https://www.postgresql.org/message-id/flat/60052986-956b-4478-45ed-8bd119e9b9cf%402ndquadrant.com#74948a1044c56c5e817a5050f554ddee 
from 2019. So there are some substantial gaps to fill, and I think the first step for now is just 
updating the error message.

What else can we do here? We have pg_indexam_has_property, pg_index_has_property, and 
pg_index_column_has_property. Do we want a pg_opclass_has_property?

Or here is a more modest suggestion: we could permit pg_index_has_property to answer can_unique 
questions. Today it returns NULL for that property. Perhaps it should (by default) answer whatever 
the AM can do. Then we could update gistproperty (which today only answers column-level inquiries), 
and return indisunique. To be honest that feels like an oversight that should have been included in 
the temporal primary key patch. I'll sign up to do that if people think it makes sense.

Another thought: should pg_indexam_has_property(783, 'can_unique') return NULL, on the theory that 
NULL means "unknown", and as of pg 18 GiST uniqueness is no longer "false" but now "it depends"? 
Maybe that is too cute. If we did that, probably it should be paired with a way to get a definitive 
answer for the scenario you care about (without creating an index first). pg_opclass_has_property 
could do that. Or maybe we add a second pg_indexam_has_property that also takes an array of opclass 
oids?


# Logical replication

I had to teach logical replication how to handle temporal index constraints. It wants a unique 
identifier for REPLICA IDENTITY. Even with WITHOUT OVERLAPS, the combination of index keys do 
identify a unique record. But again, logical replication needs to know which operators implement 
equals for non-btree indexes. I think the discussion in this thread has already covered this 
sufficiently. Peter's COMPARE_* change doesn't introduce any problems from what I can see.


# Updating opclasses

Today you can alter an opfamily, but you can't alter an opclass. So to add stratnum support 
functions, the btree_gist upgrade script does this:

		ALTER OPERATOR FAMILY gist_int4_ops USING gist ADD
			FUNCTION 12 (int4, int4) gist_stratnum_btree (int2) ;

Past upgrade scripts have done similar things.

But that doesn't achieve quite the same thing as putting the function in CREATE OPERATOR CLASS. Do 
we want a way to attach a new support function to an *opclass*? There are some problems to solve 
with staleness, the relcache for example I think, but nothing seems insurmountable. Maybe it risks 
incoherent indexes, if you change how they are computed partway through, but if so it was probably 
true already with ALTER OPERATOR FAMILY.


# Empty ranges

This is probably not relevant to this discussion, but just in case: in the v17 cycle we had to 
revert temporal PKs because empty ranges allowed duplicate records (because 'empty' && 'empty' is 
false, so there was no conflict). The solution was to reject empty ranges in a temporal PK or UNIQUE 
constraint, where they really don't make sense. Is there something generalized the index AM API 
could offer for this kind of thing?

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  27. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-15T14:31:12Z

    On 04.12.24 15:49, Peter Eisentraut wrote:
> On 27.11.24 13:57, Peter Eisentraut wrote:
>> 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'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.
> 
> Here is a patch set in that direction.  It renames RowCompareType to 
> CompareType and updates the surrounding commentary a bit.  And then I'm 
> changing the gist strategy mapping to use the CompareType values instead 
> of the RT* strategy numbers.  Seeing this now, I like this a lot better 
> than what we have now, because it makes it clearer in the API and the 
> code what is a real strategy number and what's a different kind of 
> thing.  (This isn't entirely the above-mentioned integration of the gist 
> support into your patch set yet, but it's a meaningful part of it.)

I have committed these, and I'll continue working my way through this 
patch set now.

  28. Re: Index AM API cleanup

    Nathan Bossart <nathandbossart@gmail.com> — 2025-01-15T21:15:51Z

    On Wed, Jan 15, 2025 at 03:31:12PM +0100, Peter Eisentraut wrote:
> On 04.12.24 15:49, Peter Eisentraut wrote:
>> Here is a patch set in that direction.  It renames RowCompareType to
>> CompareType and updates the surrounding commentary a bit.  And then I'm
>> changing the gist strategy mapping to use the CompareType values instead
>> of the RT* strategy numbers.  Seeing this now, I like this a lot better
>> than what we have now, because it makes it clearer in the API and the
>> code what is a real strategy number and what's a different kind of
>> thing.  (This isn't entirely the above-mentioned integration of the gist
>> support into your patch set yet, but it's a meaningful part of it.)
> 
> I have committed these, and I'll continue working my way through this patch
> set now.

cfbot's cpluspluscheck step seems to dislike the new CompareType enum
declaraction [0].  IIUC that isn't valid in C++.

[0] https://api.cirrus-ci.com/v1/task/4946960681009152/logs/headers_cpluspluscheck.log

-- 
nathan

  29. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-15T22:14:24Z

    On 15.01.25 22:15, Nathan Bossart wrote:
> On Wed, Jan 15, 2025 at 03:31:12PM +0100, Peter Eisentraut wrote:
>> On 04.12.24 15:49, Peter Eisentraut wrote:
>>> Here is a patch set in that direction.  It renames RowCompareType to
>>> CompareType and updates the surrounding commentary a bit.  And then I'm
>>> changing the gist strategy mapping to use the CompareType values instead
>>> of the RT* strategy numbers.  Seeing this now, I like this a lot better
>>> than what we have now, because it makes it clearer in the API and the
>>> code what is a real strategy number and what's a different kind of
>>> thing.  (This isn't entirely the above-mentioned integration of the gist
>>> support into your patch set yet, but it's a meaningful part of it.)
>>
>> I have committed these, and I'll continue working my way through this patch
>> set now.
> 
> cfbot's cpluspluscheck step seems to dislike the new CompareType enum
> declaraction [0].  IIUC that isn't valid in C++.

Fixed, thanks.

  30. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-25T07:18:53Z

    I've been working on integrating Mark's "Index AM API cleanup" patch set 
with the existing gist strategy number mapping from Paul's application 
time patch set.  Here is what I've come up with.

The previously committed patch (v19.1) already changed the gist strategy 
number mapping to use the (Row)CompareType, as in Mark's proposal.

In this patch set, I'm attaching the existing standalone gist translate 
function as the index AM API function for the gist index AM.  And then 
all existing callers are changed to call through the index AM API 
functions provided by Mark's patch set.

Patches 0001, 0002, 0003 are some preparatory renaming and refactoring 
patches.

Patches 0004 and 0005 are patch v19-0008 from Mark's (via Andrew) v19 
patch set, split into two patches, and with some function renaming from 
my side.

Patch 0006 then pulls it all together.  The key change is that we also 
need to pass the opclass to the index AM API functions, so that access 
methods like gist can use it.  Actually, I changed that to pass opfamily 
and opcintype instead.  I think this matches better with the rest of the 
"Index AM API cleanup" patch set, because it's more common to have the 
opfamily and type handy than the opclass.  (And internally, the gist 
support function is attached to the opfamily anyway, so it's actually 
simpler that way.)

I think this fits together quite well now.  Several places where gist 
was hardcoded are now fully (or mostly) independent of gist.  Also, the 
somewhat hackish get_equal_strategy_number() in the logical replication 
code disappears completely and is replaced by a proper index AM API 
function.  (This also takes care of patch v19-0011 from Mark's patch set.)

Also, since we have already built out test coverage for the 
GistTranslate* stuff, the new index-AM-level translate functionality 
gets to use this test coverage for free.

Thoughts?


[0]: 
https://www.postgresql.org/message-id/ad628976-8b33-468d-8e52-3fbfcff89103%40dunslane.net

  31. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2025-01-30T03:33:44Z

    
> On Jan 24, 2025, at 11:18 PM, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> I've been working on integrating Mark's "Index AM API cleanup" patch set with the existing gist strategy number mapping from Paul's application time patch set.  Here is what I've come up with.

Thank you.

> The previously committed patch (v19.1) already changed the gist strategy number mapping to use the (Row)CompareType, as in Mark's proposal.
> 
> In this patch set, I'm attaching the existing standalone gist translate function as the index AM API function for the gist index AM.  And then all existing callers are changed to call through the index AM API functions provided by Mark's patch set.
> 
> Patches 0001, 0002, 0003 are some preparatory renaming and refactoring patches.

These all look sensible.  I like that they reduce code duplication. No objection.

> Patches 0004 and 0005 are patch v19-0008 from Mark's (via Andrew) v19 patch set, split into two patches, and with some function renaming from my side.

0004 needs the copyright notice updated to 2025.

> Patch 0006 then pulls it all together.  The key change is that we also need to pass the opclass to the index AM API functions, so that access methods like gist can use it.  Actually, I changed that to pass opfamily and opcintype instead.  I think this matches better with the rest of the "Index AM API cleanup" patch set, because it's more common to have the opfamily and type handy than the opclass.  (And internally, the gist support function is attached to the opfamily anyway, so it's actually simpler that way.)
> 
> I think this fits together quite well now.  Several places where gist was hardcoded are now fully (or mostly) independent of gist.  Also, the somewhat hackish get_equal_strategy_number() in the logical replication code disappears completely and is replaced by a proper index AM API function.  (This also takes care of patch v19-0011 from Mark's patch set.)

I find the hardcoded GIST_AM_OID in GetOperatorFromCompareType() antithetical to the purpose of this patch-set.  Surely, we can avoid hardcoding this.  The patch as you have it works, I admit, but as soon as an Index AM author tries to do the equivalent of what GiST is doing, they'll hit a wall.  Making them submit a patch and wait until the next major release of Postgres ships isn't a solution, either; index AM authors may work separately from the core cadence, and in any event, often want their new indexes to work with postgres going back several versions.

How about we fix this now?  I threw this together in a hurry, and might have screwed it up, so please check carefully.  If you like this, we should go at least one more round of making sure this has thorough regression testing, but just to get your feedback, this can be applied atop your patch-set:

From 9710af54a8d468a1c7a06464d27d2f56dd9ee490 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Wed, 29 Jan 2025 19:08:27 -0800
Subject: [PATCH v19.3] Avoid hardcoding GIST_AM_OID

---
 src/backend/catalog/pg_constraint.c | 5 ++++-
 src/backend/commands/indexcmds.c    | 5 ++---
 src/backend/commands/tablecmds.c    | 4 +++-
 src/backend/utils/adt/ri_triggers.c | 3 ++-
 src/include/catalog/pg_constraint.h | 3 ++-
 src/include/commands/defrem.h       | 2 +-
 6 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index ac80652baf..492de1e3c1 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -1622,7 +1622,8 @@ DeconstructFkConstraintRow(HeapTuple tuple, int *numfks,
  * to just what was updated/deleted.
  */
 void
-FindFKPeriodOpers(Oid opclass,
+FindFKPeriodOpers(Oid amid,
+				  Oid opclass,
 				  Oid *containedbyoperoid,
 				  Oid *aggedcontainedbyoperoid,
 				  Oid *intersectoperoid)
@@ -1654,6 +1655,7 @@ FindFKPeriodOpers(Oid opclass,
 							   InvalidOid,
 							   COMPARE_CONTAINED_BY,
 							   containedbyoperoid,
+							   amid,
 							   &strat);
 
 	/*
@@ -1665,6 +1667,7 @@ FindFKPeriodOpers(Oid opclass,
 							   ANYMULTIRANGEOID,
 							   COMPARE_CONTAINED_BY,
 							   aggedcontainedbyoperoid,
+							   amid,
 							   &strat);
 
 	switch (opcintype)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 637ff52b50..e617125a5b 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2168,7 +2168,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
 				cmptype = COMPARE_OVERLAP;
 			else
 				cmptype = COMPARE_EQ;
-			GetOperatorFromCompareType(opclassOids[attn], InvalidOid, cmptype, &opid, &strat);
+			GetOperatorFromCompareType(opclassOids[attn], InvalidOid, cmptype, &opid, accessMethodId, &strat);
 			indexInfo->ii_ExclusionOps[attn] = opid;
 			indexInfo->ii_ExclusionProcs[attn] = get_opcode(opid);
 			indexInfo->ii_ExclusionStrats[attn] = strat;
@@ -2420,9 +2420,8 @@ GetDefaultOpClass(Oid type_id, Oid am_id)
  */
 void
 GetOperatorFromCompareType(Oid opclass, Oid rhstype, CompareType cmptype,
-						   Oid *opid, StrategyNumber *strat)
+						   Oid *opid, Oid amid, StrategyNumber *strat)
 {
-	Oid			amid = GIST_AM_OID;	/* For now we only need GiST support. */
 	Oid			opfamily;
 	Oid			opcintype;
 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 18f64db6e3..6c76c40fdc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10236,8 +10236,10 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		Oid			periodoperoid;
 		Oid			aggedperiodoperoid;
 		Oid			intersectoperoid;
+		Oid			amoid;
 
-		FindFKPeriodOpers(opclasses[numpks - 1], &periodoperoid, &aggedperiodoperoid,
+		amoid = get_rel_relam(indexOid);
+		FindFKPeriodOpers(amoid, opclasses[numpks - 1], &periodoperoid, &aggedperiodoperoid,
 						  &intersectoperoid);
 	}
 
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 3d9985b17c..68b1c2d4b3 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2337,8 +2337,9 @@ ri_LoadConstraintInfo(Oid constraintOid)
 	if (riinfo->hasperiod)
 	{
 		Oid			opclass = get_index_column_opclass(conForm->conindid, riinfo->nkeys);
+		Oid			amid = get_rel_relam(conForm->conindid);
 
-		FindFKPeriodOpers(opclass,
+		FindFKPeriodOpers(amid, opclass,
 						  &riinfo->period_contained_by_oper,
 						  &riinfo->agged_period_contained_by_oper,
 						  &riinfo->period_intersect_oper);
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 6da164e7e4..7ba6f077cd 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -288,7 +288,8 @@ extern void DeconstructFkConstraintRow(HeapTuple tuple, int *numfks,
 									   AttrNumber *conkey, AttrNumber *confkey,
 									   Oid *pf_eq_oprs, Oid *pp_eq_oprs, Oid *ff_eq_oprs,
 									   int *num_fk_del_set_cols, AttrNumber *fk_del_set_cols);
-extern void FindFKPeriodOpers(Oid opclass,
+extern void FindFKPeriodOpers(Oid accessMethodId,
+							  Oid opclass,
 							  Oid *containedbyoperoid,
 							  Oid *aggedcontainedbyoperoid,
 							  Oid *intersectoperoid);
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 6d9348bac8..6785770737 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -51,7 +51,7 @@ extern Oid	GetDefaultOpClass(Oid type_id, Oid am_id);
 extern Oid	ResolveOpClass(const List *opclass, Oid attrType,
 						   const char *accessMethodName, Oid accessMethodId);
 extern void GetOperatorFromCompareType(Oid opclass, Oid rhstype, CompareType cmptype,
-									   Oid *opid, StrategyNumber *strat);
+									   Oid *opid, Oid amid, StrategyNumber *strat);
 
 /* commands/functioncmds.c */
 extern ObjectAddress CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt);
-- 
2.39.3 (Apple Git-145)

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

  32. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-02-03T11:08:52Z

    On 30.01.25 04:33, Mark Dilger wrote:
>> The previously committed patch (v19.1) already changed the gist strategy number mapping to use the (Row)CompareType, as in Mark's proposal.
>>
>> In this patch set, I'm attaching the existing standalone gist translate function as the index AM API function for the gist index AM.  And then all existing callers are changed to call through the index AM API functions provided by Mark's patch set.
>>
>> Patches 0001, 0002, 0003 are some preparatory renaming and refactoring patches.
> 
> These all look sensible.  I like that they reduce code duplication. No objection.
> 
>> Patches 0004 and 0005 are patch v19-0008 from Mark's (via Andrew) v19 patch set, split into two patches, and with some function renaming from my side.
> 
> 0004 needs the copyright notice updated to 2025.
> 
>> Patch 0006 then pulls it all together.  The key change is that we also need to pass the opclass to the index AM API functions, so that access methods like gist can use it.  Actually, I changed that to pass opfamily and opcintype instead.  I think this matches better with the rest of the "Index AM API cleanup" patch set, because it's more common to have the opfamily and type handy than the opclass.  (And internally, the gist support function is attached to the opfamily anyway, so it's actually simpler that way.)
>>
>> I think this fits together quite well now.  Several places where gist was hardcoded are now fully (or mostly) independent of gist.  Also, the somewhat hackish get_equal_strategy_number() in the logical replication code disappears completely and is replaced by a proper index AM API function.  (This also takes care of patch v19-0011 from Mark's patch set.)
> 
> I find the hardcoded GIST_AM_OID in GetOperatorFromCompareType() antithetical to the purpose of this patch-set.

Yeah, actually this turned out to be unnecessary, because you can easily 
obtain the AM OID from the passed-in opclass ID.  So I have fixed it 
that way.  I have committed this whole patch set now with the mentioned 
adjustments.  I'll post a rebased version of the main patch set soon.

  33. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-02-04T13:31:51Z

    On 03.02.25 12:08, Peter Eisentraut wrote:
> Yeah, actually this turned out to be unnecessary, because you can easily 
> obtain the AM OID from the passed-in opclass ID.  So I have fixed it 
> that way.  I have committed this whole patch set now with the mentioned 
> adjustments.  I'll post a rebased version of the main patch set soon.

I picked off two more patches from the full patch series that can be 
dealt with easily.  These are

- Support non-btree indexes for foreign keys
- Allow non-btree speculative insertion indexes

(v19-0012 and v19-0013)

The version of these patches that were posted in the v19 bundle replaced 
the hardcoded comparison with BTREE_AM_OID by a check against 
(indirectly) amcanunique.  But I think that is not necessary, because at 
that point we already know that we are dealing with a unique index.  The 
reason for the check against BTREE_AM_OID was that we didn't have a way 
to get the right strategy number for other index methods.  But now we 
have, so we can use the strategy translation functions and we don't need 
to check for btree-ness anymore.

See attached patches.

  34. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-02-07T15:53:46Z

    On 04.02.25 14:31, Peter Eisentraut wrote:
> On 03.02.25 12:08, Peter Eisentraut wrote:
>> Yeah, actually this turned out to be unnecessary, because you can 
>> easily obtain the AM OID from the passed-in opclass ID.  So I have 
>> fixed it that way.  I have committed this whole patch set now with the 
>> mentioned adjustments.  I'll post a rebased version of the main patch 
>> set soon.
> 
> I picked off two more patches from the full patch series that can be 
> dealt with easily.  These are
> 
> - Support non-btree indexes for foreign keys
> - Allow non-btree speculative insertion indexes
> 
> (v19-0012 and v19-0013)
> 
> The version of these patches that were posted in the v19 bundle replaced 
> the hardcoded comparison with BTREE_AM_OID by a check against 
> (indirectly) amcanunique.  But I think that is not necessary, because at 
> that point we already know that we are dealing with a unique index.  The 
> reason for the check against BTREE_AM_OID was that we didn't have a way 
> to get the right strategy number for other index methods.  But now we 
> have, so we can use the strategy translation functions and we don't need 
> to check for btree-ness anymore.
> 
> See attached patches.

I have committed these.

Here is another small issue.  In the v19.2 patches, I had changed the 
argument of the gist strategy translation function from opclass to 
opfamily+opcintype.  This worked well with the existing uses, but when 
rebasing the rest of the main patch set, it seems that the correct value 
of "opcintype" is not always clear or readily available.  And if you 
think about it, we don't need that type, because the strategy mapping 
function is surely the same for all types supported by an opfamily.  So 
we could simplify that by registering the gist stratnum support function 
in a way that you can look it up without having to worry about the type. 
  Apparently, you can't register it with just zero in 
amproclefttype/amprocrighttype, that confuses some DDL commands.  But I 
figure we could put the pseudotype "any" in there, which is moderately 
intuitive.  And then we can drop the opcintype from all the APIs, which 
simplifies things.

See attached patches.  Thoughts?

  35. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-02-25T12:21:00Z

    Here is a rebased version of the main patch set.

As before, the patches marked [TEST] are not intended for committing, 
they are just to support testing this patch series.

Also, after some offline discussions, we are disregarding patch 0004 
"WIP: Add cluster support to the index AM API" for now.  It needs a 
different approach.

The remaining patches

0008 Generalize hash and ordering support in amapi
0010 WIP: Some stuff in AlterOpFamilyAdd
0011 FIXME: Fix direct cast from strategy to cmptype
0012 Use CompareType more and StrategyNumber less
0013 Generalize predicate tests beyond btree strategies
0014 Allow non-btree indexes to participate in mergejoin
0015 Replace various references to btree strategies

are in play at various stages of readiness.

I think 0008 is pretty much ready with some comment and documentation 
tweaking.

0013 and 0014 are also pretty straightforward.

The others (0010, 0011, 0012, 0015) still need a bit of work in some 
details, but overall the concepts are pretty solid now.

  36. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2025-03-01T13:42:40Z

    On Tue, Feb 25, 2025 at 4:21 AM Peter Eisentraut <peter@eisentraut.org>
wrote:

> Here is a rebased version of the main patch set.
>

And here is another, per gripe from the CFbot about the patch needing
another rebase:


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

  37. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-12T10:40:44Z

    While studying the patch "[PATCH v21 08/12] Allow non-btree indexes to 
participate in mergejoin", I figured we could do the sortsupport.c piece 
much simpler.  The underlying issue there is similar to commits 
0d2aa4d4937 and c594f1ad2ba: We're just using the btree strategy numbers 
to pass information about the sort direction.  We can do this simpler by 
just passing a bool.  See attached patch.

  38. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-12T14:00:10Z

    And another small patch set extracted from the bigger one, to keep 
things moving along:

0001: Add get_opfamily_method() in lsyscache.c, from your patch set.

0002: Add get_opfamily_member_for_cmptype().  This was called 
get_opmethod_member() in your patch set, but I think that name wasn't 
quite right.  I also removed the opmethod argument, which was rarely 
used and is somewhat redundant.

0003 and 0004 are enabling non-btree unique indexes for partition keys 
and materialized views.  These were v21-0011 and v21-0012, except that 
I'm combining the switch from strategies to compare types (which was in 
v21-0006 or so) with the removal of the btree requirements.

  39. Re: Index AM API cleanup

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-12T14:25:17Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> 0002: Add get_opfamily_member_for_cmptype().  This was called 
> get_opmethod_member() in your patch set, but I think that name wasn't 
> quite right.  I also removed the opmethod argument, which was rarely 
> used and is somewhat redundant.

Hm, that will throw an error if IndexAmTranslateCompareType fails.
Shouldn't it be made to return InvalidOid instead?

			regards, tom lane

  40. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2025-03-12T15:52:36Z

    On Wed, Mar 12, 2025 at 7:00 AM Peter Eisentraut <peter@eisentraut.org>
wrote:

> And another small patch set extracted from the bigger one, to keep
> things moving along:
>
> 0001: Add get_opfamily_method() in lsyscache.c, from your patch set.
>

Right, this came from v21-0006-*, with a slight code comment change and one
variable renaming.  It is ready for commit.

0002: Add get_opfamily_member_for_cmptype().  This was called
> get_opmethod_member() in your patch set, but I think that name wasn't
> quite right.  I also removed the opmethod argument, which was rarely
> used and is somewhat redundant.
>

This is also taken from v21-0006-*.  The reason I had an opmethod argument
was that some of the callers of this function already know the opmethod,
and without the argument, this function has to look up the opmethod from
the syscache again.  Whether that makes a measurable performance difference
is an open question.

Your version is ready for commit.  If we want to reintroduce the opmethod
argument for performance reasons, we can always circle back to that in a
later commit.

0003 and 0004 are enabling non-btree unique indexes for partition keys
>

You refactored v21-0011-* into v21.2-0003-*, in which an error gets raised
about a missing operator in a slightly different part of the logic.  I am
concerned that the new positioning of the check-and-error might allow the
flow of execution to reach the Assert(idx_eqop) statement in situations
where the user has defined an incomplete opfamily or opclass.  Such a
condition should raise an error about the missing operator rather than
asserting.

In particular, looking at the control logic:

                       if (stmt->unique && !stmt->iswithoutoverlaps)
                        {
                               ....
                        }
                        else if (exclusion)
                               ....;

                        Assert(idx_eqop);

I cannot prove to myself that the assertion cannot trigger, because the
upstream logic before we reach this point *might* be filtering out all
cases where this could be a problem, but it is too complicated to prove.
Even if it is impossible now, this is a pretty brittle piece of code after
applying the patch.

Any chance you'd like to keep the patch closer to how I had it in
v21-0011-* ?


> and materialized views.  These were v21-0011 and v21-0012, except that
> I'm combining the switch from strategies to compare types (which was in
> v21-0006 or so) with the removal of the btree requirements.
>

v21.2-0004-* is ready for commit.

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

  41. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2025-03-12T16:08:00Z

    On Wed, Mar 12, 2025 at 7:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Peter Eisentraut <peter@eisentraut.org> writes:
> > 0002: Add get_opfamily_member_for_cmptype().  This was called
> > get_opmethod_member() in your patch set, but I think that name wasn't
> > quite right.  I also removed the opmethod argument, which was rarely
> > used and is somewhat redundant.
>
> Hm, that will throw an error if IndexAmTranslateCompareType fails.
> Shouldn't it be made to return InvalidOid instead?
>

There are two failure modes.  In one mode, the AM has a concept of
equality, but there is no operator for the given type.  In the other mode,
the AM simply has no concept of equality.

In DefineIndex, the call:

                        if (stmt->unique && !stmt->iswithoutoverlaps)
                        {
                            idx_eqop =
get_opfamily_member_for_cmptype(idx_opfamily,

 idx_opcintype,

 idx_opcintype,

 COMPARE_EQ);
                            if (!idx_eqop)
                                ereport(ERROR,
                                        errcode(ERRCODE_UNDEFINED_OBJECT),
                                        errmsg("could not identify an
equality operator for type %s", format_type_be(idx_opcintype)),
                                        errdetail("There is no suitable
operator in operator family \"%s\" for access method \"%s\".",

get_opfamily_name(idx_opfamily, false),
get_am_name(get_opfamily_method(idx_opfamily))));
                        }

probably should error about COMPARE_EQ not having a strategy in the AM,
rather than complaining later that an equality operator cannot be found for
the type.  So that one seems fine as it is now.

The other caller, refresh_by_match_merge, is similar:

                op = get_opfamily_member_for_cmptype(opfamily, opcintype,
opcintype, COMPARE_EQ);
                if (!OidIsValid(op))
                    elog(ERROR, "missing equality operator for (%u,%u) in
opfamily %u",
                         opcintype, opcintype, opfamily);

seems fine.  There are no other callers as yet.

You have made me a bit paranoid that, in future, we might add additional
callers who are not anticipating the error.  Should we solve that with a
more complete code comment, or do you think refactoring is in order?

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

  42. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-18T11:10:07Z

    On 12.03.25 17:08, Mark Dilger wrote:
> 
> 
> On Wed, Mar 12, 2025 at 7:25 AM Tom Lane <tgl@sss.pgh.pa.us 
> <mailto:tgl@sss.pgh.pa.us>> wrote:
> 
>     Peter Eisentraut <peter@eisentraut.org
>     <mailto:peter@eisentraut.org>> writes:
>      > 0002: Add get_opfamily_member_for_cmptype().  This was called
>      > get_opmethod_member() in your patch set, but I think that name
>     wasn't
>      > quite right.  I also removed the opmethod argument, which was rarely
>      > used and is somewhat redundant.
> 
>     Hm, that will throw an error if IndexAmTranslateCompareType fails.
>     Shouldn't it be made to return InvalidOid instead?
> 
> 
> There are two failure modes.  In one mode, the AM has a concept of 
> equality, but there is no operator for the given type.  In the other 
> mode, the AM simply has no concept of equality.

I have committed it in such a way that it returns InvalidOid in either 
case.  That seems the safest approach for now.

  43. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-18T11:11:05Z

    I have committed these four patches (squashed into three).  I made the 
error handling change in 0003 that you requested, and also the error 
handling change in 0002 discussed in an adjacent message.


On 12.03.25 16:52, Mark Dilger wrote:
> 
> 
> On Wed, Mar 12, 2025 at 7:00 AM Peter Eisentraut <peter@eisentraut.org 
> <mailto:peter@eisentraut.org>> wrote:
> 
>     And another small patch set extracted from the bigger one, to keep
>     things moving along:
> 
>     0001: Add get_opfamily_method() in lsyscache.c, from your patch set.
> 
> 
> Right, this came from v21-0006-*, with a slight code comment change and 
> one variable renaming.  It is ready for commit.
> 
>     0002: Add get_opfamily_member_for_cmptype().  This was called
>     get_opmethod_member() in your patch set, but I think that name wasn't
>     quite right.  I also removed the opmethod argument, which was rarely
>     used and is somewhat redundant.
> 
> 
> This is also taken from v21-0006-*.  The reason I had an opmethod 
> argument was that some of the callers of this function already know the 
> opmethod, and without the argument, this function has to look up the 
> opmethod from the syscache again.  Whether that makes a measurable 
> performance difference is an open question.
> Your version is ready for commit.  If we want to reintroduce the 
> opmethod argument for performance reasons, we can always circle back to 
> that in a later commit.
> 
>     0003 and 0004 are enabling non-btree unique indexes for partition keys
> 
> 
> You refactored v21-0011-* into v21.2-0003-*, in which an error gets 
> raised about a missing operator in a slightly different part of the 
> logic.  I am concerned that the new positioning of the check-and-error 
> might allow the flow of execution to reach the Assert(idx_eqop) 
> statement in situations where the user has defined an incomplete 
> opfamily or opclass.  Such a condition should raise an error about the 
> missing operator rather than asserting.
> 
> In particular, looking at the control logic:
> 
>                         if (stmt->unique && !stmt->iswithoutoverlaps)
>                          {
>                                 ....
>                          }
>                          else if (exclusion)
>                                 ....;
> 
>                          Assert(idx_eqop);
> 
> I cannot prove to myself that the assertion cannot trigger, because the 
> upstream logic before we reach this point *might* be filtering out all 
> cases where this could be a problem, but it is too complicated to 
> prove.  Even if it is impossible now, this is a pretty brittle piece of 
> code after applying the patch.
> 
> Any chance you'd like to keep the patch closer to how I had it in 
> v21-0011-* ?
> 
>     and materialized views.  These were v21-0011 and v21-0012, except that
>     I'm combining the switch from strategies to compare types (which was in
>     v21-0006 or so) with the removal of the btree requirements.
> 
> 
> v21.2-0004-* is ready for commit.
> 
> —
> Mark Dilger
> EnterpriseDB:http://www.enterprisedb.com <http://www.enterprisedb.com/>
> The Enterprise PostgreSQL Company

  44. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-20T11:59:29Z

    Here is a new version of the remaining main patch set.  I've made a lot 
of changes to reduce the size and scope.

- In version v21, you had included a bunch of expected files in the 
"treeb" module, which wasn't necessary, since the test driver overwrote 
those anyway.  I removed those, which makes the patch much smaller.

- The patch set made various attempts to be able to use a non-btree 
operator family as the referenced operator family for ordering 
operators, but this was not fully developed, since you had just 
hardcoded BTREE_AM_OID in most places.  I have cut all of this out, 
since this just blows up the patch surface but doesn't add any 
functionality at this time.  I believe the patch for ALTER OPERATOR 
FAMILY / ADD falls into this category as well, so I have moved this to 
the end of the patch series, as "WIP".

(I think this is ok because the purpose of this patch set is to allow 
new index types that behave like btree in many ways, whereas what the 
above feature would allow is to have a type that doesn't require a btree 
operator family to communicate its semantics.  These things are morally 
related but technically distinct.)

- For similar reasons, I removed the changes you made in typcache.c. 
With the new infrastructure we have in place, we might someday 
reconsider how the type cache works and make it more independent of 
hardcoded btree and hash behavior, but this is not necessary for this 
patch set.

- Move the changes in rangetypes.c to a separate "WIP" patch.  This 
patch is incomplete, as described in the patch, and also not needed for 
this patch series' goal.

- There were also a couple of other minor "excessive" generalizations 
that I skipped.  For example, we don't need to make the function 
btcostestimate() independent of btree strategies.

- Conversely, I moved the changes in network.c to a separate patch at 
the front of the patch series, and fixed them up so that they actually 
work.  The effect of this is easily visible in the "inet" test in the 
"xtree" test module.

- I separated out the changes dealing with row compare support into a 
separate patch.  I think this is not fully ready.  You'll see the FIXME 
in src/backend/executor/nodeIndexscan.c.  Also, this still had a test 
for BTREE_AM_OID in it, so it didn't really work anyway.  I think 
leaving this out for now is not a huge loss.  Index support for row 
compare is a specialized feature.

- I squashed together the remaining feature patches into one patch.  I 
observed that you had gradually adjusted the interfaces to use 
CompareType instead of strategy numbers, but then left it so that they 
would accept either one (for example, PathKey).  But then I went through 
and removed the strategy number support for the most part, since nothing 
was using it anymore.  (For example, PathKey was using strategy numbers 
for fake purposes anyway, so we don't need them there anymore, and it 
all trickles from there in a way.)  The resulting patch is much smaller 
and much much simpler now, since it is mostly just a straight conversion 
from strategy to compare type and not much else.

Here is the current lineup:

v22-0001-TEST-Add-loadable-modules-as-tests-of-the-amapi.patch
v22-0002-TEST-Fixup-for-test-driver.patch

This is the test setup, as before.  I added a small fix to reduce the 
diffs further.

v22-0003-TEST-Add-treeb-deep-copy-of-btree.patch.gz
v22-0004-TEST-treeb-fixups.patch

As before, with a compilation fix to keep up with some other changes.

v22-0005-Generalize-index-support-in-network-support-func.patch

As explained above, I think this patch stands on its own and is ready to 
go.  Check please.

v22-0006-Convert-from-StrategyNumber-to-CompareType.patch

This is all that remains now.  I think with a bit more polishing around 
the edges, some comment updates, etc., this is close to ready.

v22-0007-TEST-Rotate-treeb-strategy-numbers.patch

This still works. ;-)

v22-0008-WIP-Support-row-compare-index-scans-with-non-btr.patch
v22-0009-WIP-Generalize-index-support-in-range-type-suppo.patch
v22-0010-WIP-Allow-more-index-AMs-in-ALTER-OP-FAMILY.ADD.patch

These are postponed, as described above.

  45. Re: Index AM API cleanup

    Mark Dilger <mark.dilger@enterprisedb.com> — 2025-03-20T21:14:20Z

    On Thu, Mar 20, 2025 at 4:59 AM Peter Eisentraut <peter@eisentraut.org>
wrote:

> Here is a new version of the remaining main patch set.  I've made a lot
> of changes to reduce the size and scope.
>
> - In version v21, you had included a bunch of expected files in the
> "treeb" module, which wasn't necessary, since the test driver overwrote
> those anyway.  I removed those, which makes the patch much smaller.
>

Yes, I had noticed that also, but didn't repost because each post is quite
large.  Thanks for combining this cleanup with v22.


> - The patch set made various attempts to be able to use a non-btree
> operator family as the referenced operator family for ordering
> operators, but this was not fully developed, since you had just
> hardcoded BTREE_AM_OID in most places.  I have cut all of this out,
> since this just blows up the patch surface but doesn't add any
> functionality at this time.  I believe the patch for ALTER OPERATOR
> FAMILY / ADD falls into this category as well, so I have moved this to
> the end of the patch series, as "WIP".
>

Ok.

(I think this is ok because the purpose of this patch set is to allow
> new index types that behave like btree in many ways, whereas what the
> above feature would allow is to have a type that doesn't require a btree
> operator family to communicate its semantics.  These things are morally
> related but technically distinct.)
>

Ok.


> - For similar reasons, I removed the changes you made in typcache.c.
> With the new infrastructure we have in place, we might someday
> reconsider how the type cache works and make it more independent of
> hardcoded btree and hash behavior, but this is not necessary for this
> patch set.
>

Ok.


> - Move the changes in rangetypes.c to a separate "WIP" patch.  This
> patch is incomplete, as described in the patch, and also not needed for
> this patch series' goal.
>

Ok.

- There were also a couple of other minor "excessive" generalizations
> that I skipped.  For example, we don't need to make the function
> btcostestimate() independent of btree strategies.
>

Ok.


> - Conversely, I moved the changes in network.c to a separate patch at
> the front of the patch series, and fixed them up so that they actually
> work.  The effect of this is easily visible in the "inet" test in the
> "xtree" test module.
>

Your changes get exercised by the main regression suite and, of
course, src/test/recovery and src/bin/pg_upgrade, which each run the main
regression suite.  These all pass.  Likewise, the tests in
src/test/modules/treeb/ exercise the changes.


> - I separated out the changes dealing with row compare support into a
> separate patch.  I think this is not fully ready.  You'll see the FIXME
> in src/backend/executor/nodeIndexscan.c.  Also, this still had a test
> for BTREE_AM_OID in it, so it didn't really work anyway.  I think
> leaving this out for now is not a huge loss.  Index support for row
> compare is a specialized feature.
>

Ok.


> - I squashed together the remaining feature patches into one patch.  I
> observed that you had gradually adjusted the interfaces to use
> CompareType instead of strategy numbers, but then left it so that they
> would accept either one (for example, PathKey).  But then I went through
> and removed the strategy number support for the most part, since nothing
> was using it anymore.  (For example, PathKey was using strategy numbers
> for fake purposes anyway, so we don't need them there anymore, and it
> all trickles from there in a way.)  The resulting patch is much smaller
> and much much simpler now, since it is mostly just a straight conversion
> from strategy to compare type and not much else.
>

Ah, nice.  Yeah, I didn't like leaving the strategy number stuff lying
around, but didn't realize I had left so many improvements unrealized.
Thanks for your analysis.


> Here is the current lineup:
>
> v22-0001-TEST-Add-loadable-modules-as-tests-of-the-amapi.patch
> v22-0002-TEST-Fixup-for-test-driver.patch
>
> This is the test setup, as before.  I added a small fix to reduce the
> diffs further.
>
> v22-0003-TEST-Add-treeb-deep-copy-of-btree.patch.gz
> v22-0004-TEST-treeb-fixups.patch
>
> As before, with a compilation fix to keep up with some other changes.
>
> v22-0005-Generalize-index-support-in-network-support-func.patch
>
> As explained above, I think this patch stands on its own and is ready to
> go.  Check please.
>

Looks good.


> v22-0006-Convert-from-StrategyNumber-to-CompareType.patch


> This is all that remains now.  I think with a bit more polishing around
> the edges, some comment updates, etc., this is close to ready.
>

I went through this and only found one problem, a missing argument, easily
fixed as follows:

diff --git a/src/backend/executor/nodeIndexscan.c
b/src/backend/executor/nodeIndexscan.c
index c04fe461083..02a1feb2add 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -1365,6 +1365,7 @@ ExecIndexBuildScanKeys(PlanState *planstate, Relation
index,

                get_op_opfamily_properties(opno, opfamily, isorderby,
                                           &op_strategy,
+                                          NULL,        /* don't need
cmptype */
                                           &op_lefttype,
                                           &op_righttype);


v22-0007-TEST-Rotate-treeb-strategy-numbers.patch
>
> This still works. ;-)
>

Yes, this seems fine.


> v22-0008-WIP-Support-row-compare-index-scans-with-non-btr.patch
> v22-0009-WIP-Generalize-index-support-in-range-type-suppo.patch
> v22-0010-WIP-Allow-more-index-AMs-in-ALTER-OP-FAMILY.ADD.patch
>
> These are postponed, as described above.
>

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

  46. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-04-01T12:50:55Z

    On 20.03.25 12:59, Peter Eisentraut wrote:
> v22-0006-Convert-from-StrategyNumber-to-CompareType.patch
> 
> This is all that remains now.  I think with a bit more polishing around 
> the edges, some comment updates, etc., this is close to ready.

Here is an updated version of this patch.  I have left out all the extra 
tests and WIP patches etc. from the series for now so that the focus is 
clear.

This patch is mostly unchanged from the above, except some small amount 
of updating comments, as well as the following.

I've done a fair bit of performance testing to make sure there are no 
noticeable regressions from this patch.  I've found that the function 
get_mergejoin_opfamilies() is quite critical to the planning time of 
even simple queries (such as pgbench --select-only), so I played around 
with various caching schemes.  In the end, I just settled on hardcoding 
information about the built-in index AM types.  Which is of course ugly, 
but at least it's essentially the same as before.  If we find other 
affected hotspots, we could apply similar workarounds, but so far I 
haven't found any.

  47. Re: Index AM API cleanup

    Andres Freund <andres@anarazel.de> — 2025-04-04T12:17:20Z

    Hi,,

skink/valgrind just started to die during the main regression tests:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2025-04-04%2011%3A01%3A56

The set of commits seem to point to the changes made as part of this thread.

==4163809== VALGRINDERROR-BEGIN
==4163809== Invalid read of size 4
==4163809==    at 0x6983FC: get_actual_variable_range (selfuncs.c:6354)
==4163809==    by 0x69E45F: ineq_histogram_selectivity (selfuncs.c:1130)
==4163809==    by 0x69E8C7: scalarineqsel (selfuncs.c:689)
==4163809==    by 0x69EB35: scalarineqsel_wrapper (selfuncs.c:1460)
==4163809==    by 0x69EBFD: scalargesel (selfuncs.c:1501)
==4163809==    by 0x6FE1AB: FunctionCall4Coll (fmgr.c:1213)
==4163809==    by 0x6FE77C: OidFunctionCall4Coll (fmgr.c:1449)
==4163809==    by 0x49C070: restriction_selectivity (plancat.c:1985)
==4163809==    by 0x43C8BB: clause_selectivity_ext (clausesel.c:848)
==4163809==    by 0x43CB3B: clauselist_selectivity_ext (clausesel.c:136)
==4163809==    by 0x43CEE3: clauselist_selectivity (clausesel.c:106)
==4163809==    by 0x4441D5: set_baserel_size_estimates (costsize.c:5342)
==4163809==  Address 0x0 is not stack'd, malloc'd or (recently) free'd


A bunch of other instances also started to die recently:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2025-04-04%2011%3A38%3A08
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=canebrake&dt=2025-04-04%2011%3A30%3A09

Greetings,

Andres Freund

  48. Re: Index AM API cleanup

    Peter Eisentraut <peter@eisentraut.org> — 2025-04-04T12:46:59Z

    On 04.04.25 14:17, Andres Freund wrote:
> skink/valgrind just started to die during the main regression tests:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2025-04-04%2011%3A01%3A56

Thanks, fix is on the way.

> The set of commits seem to point to the changes made as part of this thread.
> 
> ==4163809== VALGRINDERROR-BEGIN
> ==4163809== Invalid read of size 4
> ==4163809==    at 0x6983FC: get_actual_variable_range (selfuncs.c:6354)
> ==4163809==    by 0x69E45F: ineq_histogram_selectivity (selfuncs.c:1130)
> ==4163809==    by 0x69E8C7: scalarineqsel (selfuncs.c:689)
> ==4163809==    by 0x69EB35: scalarineqsel_wrapper (selfuncs.c:1460)
> ==4163809==    by 0x69EBFD: scalargesel (selfuncs.c:1501)
> ==4163809==    by 0x6FE1AB: FunctionCall4Coll (fmgr.c:1213)
> ==4163809==    by 0x6FE77C: OidFunctionCall4Coll (fmgr.c:1449)
> ==4163809==    by 0x49C070: restriction_selectivity (plancat.c:1985)
> ==4163809==    by 0x43C8BB: clause_selectivity_ext (clausesel.c:848)
> ==4163809==    by 0x43CB3B: clauselist_selectivity_ext (clausesel.c:136)
> ==4163809==    by 0x43CEE3: clauselist_selectivity (clausesel.c:106)
> ==4163809==    by 0x4441D5: set_baserel_size_estimates (costsize.c:5342)
> ==4163809==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
> 
> 
> A bunch of other instances also started to die recently:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2025-04-04%2011%3A38%3A08
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=canebrake&dt=2025-04-04%2011%3A30%3A09
> 
> Greetings,
> 
> Andres Freund