Re: Index AM API cleanup

> On Jan 24, 2025, at 11:18 PM, Peter Eisentraut <peter(at)eisentraut(dot)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(dot)dilger(at)enterprisedb(dot)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