Make the conditions in IsIndexUsableForReplicaIdentityFull() more explicit

IsIndexUsableForReplicaIdentityFull() described a number of conditions
that a suitable index has to fulfill.  But not all of these were
actually checked in the code.  Instead, it appeared to rely on
get_equal_strategy_number() to filter out any indexes that are not
btree or hash.  As we look to generalize index AM capabilities, this
would possibly break if we added additional support in
get_equal_strategy_number().  Instead, write out code to check for the
required capabilities explicitly.  This shouldn't change any behaviors
at the moment.

Reviewed-by: Paul Jungwirth <pj@illuminatedcomputing.com>
Reviewed-by: vignesh C <vignesh21@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/CA+renyUApHgSZF9-nd-a0+OPGharLQLO=mDHcY4_qQ0+noCUVg@mail.gmail.com

1 files changed, 25 insertions, 27 deletions

diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index c3799a6185e..dd8a3809096 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -17,9 +17,7 @@
 
 #include "postgres.h"
 
-#ifdef USE_ASSERT_CHECKING
 #include "access/amapi.h"
-#endif
 #include "access/genam.h"
 #include "access/table.h"
 #include "catalog/namespace.h"
@@ -798,9 +796,10 @@ FindUsableIndexForReplicaIdentityFull(Relation localrel, AttrMap *attrmap)
 /*
  * Returns true if the index is usable for replica identity full.
  *
- * The index must be btree or hash, non-partial, and the leftmost field must be
- * a column (not an expression) that references the remote relation column. These
- * limitations help to keep the index scan similar to PK/RI index scans.
+ * The index must have an equal strategy for each key column, be non-partial,
+ * and the leftmost field must be a column (not an expression) that references
+ * the remote relation column. These limitations help to keep the index scan
+ * similar to PK/RI index scans.
  *
  * attrmap is a map of local attributes to remote ones. We can consult this
  * map to check whether the local index attribute has a corresponding remote
@@ -813,19 +812,6 @@ 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:
- *
- * 1) Other index access methods don't have a fixed strategy for equality
- * 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
- * a datatype (e.g. point or box) that does not have a default operator class
- * for Btree or Hash.
- *
- * XXX: Note that BRIN and GIN indexes do not implement "amgettuple" which
- * will be used later to fetch the tuples. See RelationFindReplTupleByIndex().
- *
  * XXX: To support partial indexes, the required changes are likely to be larger.
  * If none of the tuples satisfy the expression for the index scan, we fall-back
  * to sequential execution, which might not be a good idea in some cases.
@@ -853,6 +839,21 @@ IsIndexUsableForReplicaIdentityFull(Relation idxrel, AttrMap *attrmap)
 			return false;
 	}
 
+	/*
+	 * For indexes other than PK and REPLICA IDENTITY, we need to match the
+	 * local and remote tuples.  The equality routine tuples_equal() cannot
+	 * accept a data type where the type cache cannot provide an equality
+	 * operator.
+	 */
+	for (int i = 0; i < idxrel->rd_att->natts; i++)
+	{
+		TypeCacheEntry *typentry;
+
+		typentry = lookup_type_cache(TupleDescAttr(idxrel->rd_att, i)->atttypid, TYPECACHE_EQ_OPR_FINFO);
+		if (!OidIsValid(typentry->eq_opr_finfo.fn_oid))
+			return false;
+	}
+
 	/* The leftmost index field must not be an expression */
 	keycol = idxrel->rd_index->indkey.values[0];
 	if (!AttributeNumberIsValid(keycol))
@@ -867,15 +868,12 @@ IsIndexUsableForReplicaIdentityFull(Relation idxrel, AttrMap *attrmap)
 		attrmap->attnums[AttrNumberGetAttrOffset(keycol)] < 0)
 		return false;
 
-#ifdef USE_ASSERT_CHECKING
-	{
-		IndexAmRoutine *amroutine;
-
-		/* The given index access method must implement amgettuple. */
-		amroutine = GetIndexAmRoutineByAmId(idxrel->rd_rel->relam, false);
-		Assert(amroutine->amgettuple != NULL);
-	}
-#endif
+	/*
+	 * The given index access method must implement "amgettuple", which will
+	 * be used later to fetch the tuples.  See RelationFindReplTupleByIndex().
+	 */
+	if (GetIndexAmRoutineByAmId(idxrel->rd_rel->relam, false)->amgettuple == NULL)
+		return false;
 
 	return true;
 }