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

> 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
mappingfrom 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,
asin Mark's proposal. 
>
> In this patch set, I'm attaching the existing standalone gist translate function as the index AM API function for the
gistindex AM.  And then all existing callers are changed to call through the index AM API functions provided by Mark's
patchset. 
>
> 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
somefunction 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
havethe opfamily and type handy than the opclass.  (And internally, the gist support function is attached to the
opfamilyanyway, 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)
independentof gist.  Also, the somewhat hackish get_equal_strategy_number() in the logical replication code disappears
completelyand 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
todo the equivalent of what GiST is doing, they'll hit a wall.  Making them submit a patch and wait until the next
majorrelease of Postgres ships isn't a solution, either; index AM authors may work separately from the core cadence,
andin 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.
Ifyou like this, we should go at least one more round of making sure this has thorough regression testing, but just to
getyour 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