Re: Index AM API cleanup

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