On 22.11.24 01:30, Paul Jungwirth wrote:
- return get_equal_strategy_number_for_am(am);
+ /* For GiST indexes we need to ask the opclass what strategy
number to use. */
+ if (am == GIST_AM_OID)
+ return GistTranslateStratnum(opclass,
RTEqualStrategyNumber);
+ else
+ return get_equal_strategy_number_for_am(am);
This code should probably be pushed into
get_equal_strategy_number_for_am(). That function already
has a switch based on index AM OIDs. Also, there are other callers of
get_equal_strategy_number_for_am(), which also might want to become
aware of GiST support.
Done. The reason I didn't do this before is because we need the opclass,
not just the am. I put an
explanation about that into the function comment. If that's a problem I
can undo the change.
I think this is the right idea, but after digging around a bit more, I
think more could/should be done.
After these changes, the difference between
get_equal_strategy_number_for_am() and get_equal_strategy_number() is
kind of pointless. We should really just use
get_equal_strategy_number() for all purposes.
But then you have the problem that IsIndexUsableForReplicaIdentityFull()
doesn't have the opclass IDs available in the IndexInfo structure. You
appear to have worked around that by writing
+ if (get_equal_strategy_number_for_am(indexInfo->ii_Am, InvalidOid)
== InvalidStrategy)
which I suppose will have the same ultimate result as before that patch,
but it seems kind of incomplete.
I figure this could all be simpler if
IsIndexUsableForReplicaIdentityFull() used the index relcache entry
directly instead of going the detour through IndexInfo. Then we have
all the information available, and this should ultimately all work
properly for suitable GiST indexes as well.
I have attached three patches that show how that could be done. (This
would work in conjunction with your new tests. (Although now we could
also test GiST with replica identity full?))
The comment block for IsIndexUsableForReplicaIdentityFull() makes a
bunch of claims that are not all explicitly supported by the code. The
code doesn't actually check the AM, this is all only done indirectly via
other checks. The second point (about tuples_equal()) appears to be
slightly wrong, because while you need an equals operator from the type
cache, that shouldn't prevent you from also using a different index AM
than btree or hash for the replica identity index. And the stuff about
amgettuple, if that is important, why is it only checked for assert builds?
From fdf5a2335cb987953b070dec19e20a3f9caaaa5e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 26 Nov 2024 11:25:12 +0100
Subject: [PATCH 1/3] Simplify IsIndexUsableForReplicaIdentityFull()
Take Relation as argument instead of IndexInfo. Building the
IndexInfo is an unnecessary intermediate step here.
---
src/backend/replication/logical/relation.c | 16 +++++++---------
src/backend/replication/logical/worker.c | 2 +-
src/include/replication/logicalrelation.h | 2 +-
3 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/relation.c
b/src/backend/replication/logical/relation.c
index f5a0ef2bd9d..1b426f43fd7 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -750,11 +750,9 @@ FindUsableIndexForReplicaIdentityFull(Relation localrel,
AttrMap *attrmap)
{
bool isUsableIdx;
Relation idxRel;
- IndexInfo *idxInfo;
idxRel = index_open(idxoid, AccessShareLock);
- idxInfo = BuildIndexInfo(idxRel);
- isUsableIdx = IsIndexUsableForReplicaIdentityFull(idxInfo,
attrmap);
+ isUsableIdx = IsIndexUsableForReplicaIdentityFull(idxRel,
attrmap);
index_close(idxRel, AccessShareLock);
/* Return the first eligible index found */
@@ -801,22 +799,22 @@ FindUsableIndexForReplicaIdentityFull(Relation localrel,
AttrMap *attrmap)
* to sequential execution, which might not be a good idea in some cases.
*/
bool
-IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo, AttrMap *attrmap)
+IsIndexUsableForReplicaIdentityFull(Relation idxrel, AttrMap *attrmap)
{
AttrNumber keycol;
/* Ensure that the index access method has a valid equal strategy */
- if (get_equal_strategy_number_for_am(indexInfo->ii_Am) ==
InvalidStrategy)
+ if (get_equal_strategy_number_for_am(idxrel->rd_rel->relam) ==
InvalidStrategy)
return false;
/* The index must not be a partial index */
- if (indexInfo->ii_Predicate != NIL)
+ if (!heap_attisnull(idxrel->rd_indextuple, Anum_pg_index_indpred, NULL))
return false;
- Assert(indexInfo->ii_NumIndexAttrs >= 1);
+ Assert(idxrel->rd_index->indnatts >= 1);
/* The leftmost index field must not be an expression */
- keycol = indexInfo->ii_IndexAttrNumbers[0];
+ keycol = idxrel->rd_index->indkey.values[0];
if (!AttributeNumberIsValid(keycol))
return false;
@@ -834,7 +832,7 @@ IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo,
AttrMap *attrmap)
IndexAmRoutine *amroutine;
/* The given index access method must implement amgettuple. */
- amroutine = GetIndexAmRoutineByAmId(indexInfo->ii_Am, false);
+ amroutine = GetIndexAmRoutineByAmId(idxrel->rd_rel->relam,
false);
Assert(amroutine->amgettuple != NULL);
}
#endif
diff --git a/src/backend/replication/logical/worker.c
b/src/backend/replication/logical/worker.c
index 925dff9cc44..46d3ad566f6 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2931,7 +2931,7 @@ FindReplTupleInLocalRel(ApplyExecutionData *edata,
Relation localrel,
/* Index must be PK, RI, or usable for REPLICA IDENTITY FULL
tables */
Assert(GetRelationIdentityOrPK(localrel) == localidxoid ||
(remoterel->replident == REPLICA_IDENTITY_FULL &&
-
IsIndexUsableForReplicaIdentityFull(BuildIndexInfo(idxrel),
+ IsIndexUsableForReplicaIdentityFull(idxrel,
edata->targetRel->attrmap)));
index_close(idxrel, AccessShareLock);
#endif
diff --git a/src/include/replication/logicalrelation.h
b/src/include/replication/logicalrelation.h
index e687b40a566..33534672ec3 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -48,7 +48,7 @@ extern LogicalRepRelMapEntry
*logicalrep_partition_open(LogicalRepRelMapEntry *r
Relation partrel, AttrMap *map);
extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
LOCKMODE
lockmode);
-extern bool IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo, AttrMap
*attrmap);
+extern bool IsIndexUsableForReplicaIdentityFull(Relation idxrel, AttrMap
*attrmap);
extern Oid GetRelationIdentityOrPK(Relation rel);
#endif /* LOGICALRELATION_H */
--
2.47.0
From e24695c4a912eeefe0cfc2fbfa5369a0374e6223 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 26 Nov 2024 11:47:24 +0100
Subject: [PATCH 2/3] Replace get_equal_strategy_number_for_am() by
get_equal_strategy_number()
---
src/backend/executor/execReplication.c | 17 +++--------------
src/backend/replication/logical/relation.c | 15 +++++++++++----
src/include/executor/executor.h | 2 +-
3 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/src/backend/executor/execReplication.c
b/src/backend/executor/execReplication.c
index 54025c9f150..8313ccdb46b 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -39,7 +39,7 @@ static bool tuples_equal(TupleTableSlot *slot1,
TupleTableSlot *slot2,
/*
* Returns the fixed strategy number, if any, of the equality operator for the
- * given index access method, otherwise, InvalidStrategy.
+ * given operator class, otherwise, InvalidStrategy.
*
* Currently, only Btree and Hash indexes are supported. The other index access
* methods don't have a fixed strategy for equality operation - instead, the
@@ -47,8 +47,9 @@ static bool tuples_equal(TupleTableSlot *slot1,
TupleTableSlot *slot2,
* according to the operator class's definition.
*/
StrategyNumber
-get_equal_strategy_number_for_am(Oid am)
+get_equal_strategy_number(Oid opclass)
{
+ Oid am = get_opclass_method(opclass);
int ret;
switch (am)
@@ -68,18 +69,6 @@ get_equal_strategy_number_for_am(Oid am)
return ret;
}
-/*
- * Return the appropriate strategy number which corresponds to the equality
- * operator.
- */
-static StrategyNumber
-get_equal_strategy_number(Oid opclass)
-{
- Oid am = get_opclass_method(opclass);
-
- return get_equal_strategy_number_for_am(am);
-}
-
/*
* Setup a ScanKey for a search in the relation 'rel' for a tuple 'key' that
* is setup to match 'rel' (*NOT* idxrel!).
diff --git a/src/backend/replication/logical/relation.c
b/src/backend/replication/logical/relation.c
index 1b426f43fd7..7cda6a792a2 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -29,6 +29,7 @@
#include "replication/logicalrelation.h"
#include "replication/worker_internal.h"
#include "utils/inval.h"
+#include "utils/syscache.h"
static MemoryContext LogicalRepRelMapContext = NULL;
@@ -784,7 +785,7 @@ FindUsableIndexForReplicaIdentityFull(Relation localrel,
AttrMap *attrmap)
* The reasons why only Btree and Hash indexes can be considered as usable are:
*
* 1) Other index access methods don't have a fixed strategy for equality
- * operation. Refer get_equal_strategy_number_for_am().
+ * operation. Refer get_equal_strategy_number().
*
* 2) For indexes other than PK and REPLICA IDENTITY, we need to match the
* local and remote tuples. The equality routine tuples_equal() cannot accept
@@ -802,10 +803,9 @@ bool
IsIndexUsableForReplicaIdentityFull(Relation idxrel, AttrMap *attrmap)
{
AttrNumber keycol;
+ oidvector *indclass;
- /* Ensure that the index access method has a valid equal strategy */
- if (get_equal_strategy_number_for_am(idxrel->rd_rel->relam) ==
InvalidStrategy)
- return false;
+ indclass = (oidvector *)
DatumGetPointer(SysCacheGetAttrNotNull(INDEXRELID, idxrel->rd_indextuple,
Anum_pg_index_indclass));
/* The index must not be a partial index */
if (!heap_attisnull(idxrel->rd_indextuple, Anum_pg_index_indpred, NULL))
@@ -813,6 +813,13 @@ IsIndexUsableForReplicaIdentityFull(Relation idxrel,
AttrMap *attrmap)
Assert(idxrel->rd_index->indnatts >= 1);
+ /* Ensure that the index has a valid equal strategy for each column */
+ for (int i = 0; i < idxrel->rd_index->indnatts; i++)
+ {
+ if (get_equal_strategy_number(indclass->values[i]) ==
InvalidStrategy)
+ return false;
+ }
+
/* The leftmost index field must not be an expression */
keycol = idxrel->rd_index->indkey.values[0];
if (!AttributeNumberIsValid(keycol))
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 69c3ebff00a..e949cce7d98 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -658,7 +658,7 @@ extern void check_exclusion_constraint(Relation heap,
Relation index,
/*
* prototypes from functions in execReplication.c
*/
-extern StrategyNumber get_equal_strategy_number_for_am(Oid am);
+extern StrategyNumber get_equal_strategy_number(Oid opclass);
extern bool RelationFindReplTupleByIndex(Relation rel, Oid idxoid,
LockTupleMode lockmode,
TupleTableSlot *searchslot,
--
2.47.0
From a46139648ac7c4de927315158b66c15bd2fd5360 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 26 Nov 2024 11:57:08 +0100
Subject: [PATCH 3/3] Support for GIST in get_equal_strategy_number()
---
src/backend/executor/execReplication.c | 9 ++++-----
src/backend/replication/logical/relation.c | 2 +-
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/backend/executor/execReplication.c
b/src/backend/executor/execReplication.c
index 8313ccdb46b..0fa67adb990 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -15,6 +15,7 @@
#include "postgres.h"
#include "access/genam.h"
+#include "access/gist.h"
#include "access/relscan.h"
#include "access/tableam.h"
#include "access/transam.h"
@@ -40,11 +41,6 @@ static bool tuples_equal(TupleTableSlot *slot1,
TupleTableSlot *slot2,
/*
* Returns the fixed strategy number, if any, of the equality operator for the
* given operator class, otherwise, InvalidStrategy.
- *
- * Currently, only Btree and Hash indexes are supported. The other index access
- * methods don't have a fixed strategy for equality operation - instead, the
- * support routines of each operator class interpret the strategy numbers
- * according to the operator class's definition.
*/
StrategyNumber
get_equal_strategy_number(Oid opclass)
@@ -60,6 +56,9 @@ get_equal_strategy_number(Oid opclass)
case HASH_AM_OID:
ret = HTEqualStrategyNumber;
break;
+ case GIST_AM_OID:
+ ret = GistTranslateStratnum(opclass,
RTEqualStrategyNumber);
+ break;
default:
/* XXX: Only Btree and Hash indexes are supported */
ret = InvalidStrategy;
diff --git a/src/backend/replication/logical/relation.c
b/src/backend/replication/logical/relation.c
index 7cda6a792a2..f686975f39d 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -782,7 +782,7 @@ FindUsableIndexForReplicaIdentityFull(Relation localrel,
AttrMap *attrmap)
* compare the tuples for non-PK/RI index scans. See
* RelationFindReplTupleByIndex().
*
- * The reasons why only Btree and Hash indexes can be considered as usable are:
+ * The reasons why only Btree and Hash indexes can be considered as usable
are: XXX
*
* 1) Other index access methods don't have a fixed strategy for equality
* operation. Refer get_equal_strategy_number().
--
2.47.0
