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 <[email protected]>
Reviewed-by: vignesh C <[email protected]>
Discussion: https://www.postgresql.org/message-id/flat/CA+renyUApHgSZF9-nd-a0+OPGharLQLO=mDHcY4_qQ0+noCUVg@mail.gmail.com

diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index c3799a6185e8159b72366aae1977b229c6476c70..dd8a3809096020dbfe0317ff6cf10f38723008c6 100644 (file)
--- 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;
 }