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

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;
 }