Remove unnecessary checks for indexes for REPLICA IDENTITY FULL tables.

Previously, when selecting an usable index for update/delete for the
REPLICA IDENTITY FULL table, in IsIndexOnlyExpression(), we used to
check if all index fields are not expressions. However, it was not
necessary, because it is enough to check if only the leftmost index
field is not an expression (and references the remote table column)
and this check has already been done by
RemoteRelContainsLeftMostColumnOnIdx().

This commit removes IsIndexOnlyExpression() and
RemoteRelContainsLeftMostColumnOnIdx() and all checks for usable
indexes for REPLICA IDENTITY FULL tables are now performed by
IsIndexUsableForReplicaIdentityFull().

Backpatch this to remain the code consistent.

Reported-by: Peter Smith
Reviewed-by: Amit Kapila, Önder Kalacı
Discussion: https://postgr.es/m/CAHut%2BPsGRE5WSsY0jcLHJEoA17MrbP9yy8FxdjC_ZOAACxbt%2BQ%40mail.gmail.com
Backpatch-through: 16

src/backend/executor/execReplication.c

@@ -175,16 +175,7 @@ retry:
 		if (!isIdxSafeToSkipDuplicates)
 		{
 			if (eq == NULL)
-			{
-#ifdef USE_ASSERT_CHECKING
-				/* apply assertions only once for the input idxoid */
-				IndexInfo  *indexInfo = BuildIndexInfo(idxrel);
-
-				Assert(IsIndexUsableForReplicaIdentityFull(indexInfo));
-#endif
-
 				eq = palloc0(sizeof(*eq) * outslot->tts_tupleDescriptor->natts);
-			}
 
 			if (!tuples_equal(outslot, searchslot, eq))
 				continue;

src/backend/replication/logical/relation.c

@@ -732,57 +732,51 @@ logicalrep_partition_open(LogicalRepRelMapEntry *root,
 }
 
 /*
- * Returns true if the given index consists only of expressions such as:
+ * Returns the oid of an index that can be used by the apply worker to scan
- * 	CREATE INDEX idx ON table(foo(col));
+ * the relation.
  *
- * Returns false even if there is one column reference:
+ * We expect to call this function when REPLICA IDENTITY FULL is defined for
- * 	 CREATE INDEX idx ON table(foo(col), col_2);
+ * the remote relation.
+ *
+ * If no suitable index is found, returns InvalidOid.
  */
-static bool
+static Oid
-IsIndexOnlyOnExpression(IndexInfo *indexInfo)
+FindUsableIndexForReplicaIdentityFull(Relation localrel, AttrMap *attrmap)
 {
-	for (int i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++)
+	List	   *idxlist = RelationGetIndexList(localrel);
+	ListCell   *lc;
+
+	foreach(lc, idxlist)
 	{
-		AttrNumber	attnum = indexInfo->ii_IndexAttrNumbers[i];
+		Oid			idxoid = lfirst_oid(lc);
+		bool		isUsableIdx;
+		Relation	idxRel;
+		IndexInfo  *idxInfo;
 
-		if (AttributeNumberIsValid(attnum))
+		idxRel = index_open(idxoid, AccessShareLock);
-			return false;
+		idxInfo = BuildIndexInfo(idxRel);
+		isUsableIdx = IsIndexUsableForReplicaIdentityFull(idxInfo, attrmap);
+		index_close(idxRel, AccessShareLock);
+
+		/* Return the first eligible index found */
+		if (isUsableIdx)
+			return idxoid;
 	}
 
-	return true;
+	return InvalidOid;
 }
 
 /*
- * Returns true if the attrmap contains the leftmost column of the index.
+ * Returns true if the index is usable for replica identity full.
- * Otherwise returns false.
+ *
+ * The index must be btree, 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
  * attribute.
- */
-static bool
-RemoteRelContainsLeftMostColumnOnIdx(IndexInfo *indexInfo, AttrMap *attrmap)
-{
-	AttrNumber	keycol;
-
-	Assert(indexInfo->ii_NumIndexAttrs >= 1);
-
-	keycol = indexInfo->ii_IndexAttrNumbers[0];
-	if (!AttributeNumberIsValid(keycol))
-		return false;
-
-	if (attrmap->maplen <= AttrNumberGetAttrOffset(keycol))
-		return false;
-
-	return attrmap->attnums[AttrNumberGetAttrOffset(keycol)] >= 0;
-}
-
-/*
- * Returns the oid of an index that can be used by the apply worker to scan
- * the relation. The index must be btree, 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.
  *
  * Note that the limitations of index scans for replica identity full only
  * adheres to a subset of the limitations of PK/RI. For example, we support
@@ -796,53 +790,37 @@ RemoteRelContainsLeftMostColumnOnIdx(IndexInfo *indexInfo, AttrMap *attrmap)
  * For 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.
- *
- * We expect to call this function when REPLICA IDENTITY FULL is defined for
- * the remote relation.
- *
- * If no suitable index is found, returns InvalidOid.
  */
-static Oid
+bool
-FindUsableIndexForReplicaIdentityFull(Relation localrel, AttrMap *attrmap)
+IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo, AttrMap *attrmap)
 {
-	List	   *idxlist = RelationGetIndexList(localrel);
+	AttrNumber	keycol;
-	ListCell   *lc;
 
-	foreach(lc, idxlist)
+	/* The index must be a Btree index */
-	{
+	if (indexInfo->ii_Am != BTREE_AM_OID)
-		Oid			idxoid = lfirst_oid(lc);
+		return false;
-		bool		isUsableIdx;
-		bool		containsLeftMostCol;
-		Relation	idxRel;
-		IndexInfo  *idxInfo;
 
-		idxRel = index_open(idxoid, AccessShareLock);
+	/* The index must not be a partial index */
-		idxInfo = BuildIndexInfo(idxRel);
+	if (indexInfo->ii_Predicate != NIL)
-		isUsableIdx = IsIndexUsableForReplicaIdentityFull(idxInfo);
+		return false;
-		containsLeftMostCol =
-			RemoteRelContainsLeftMostColumnOnIdx(idxInfo, attrmap);
-		index_close(idxRel, AccessShareLock);
 
-		/* Return the first eligible index found */
+	Assert(indexInfo->ii_NumIndexAttrs >= 1);
-		if (isUsableIdx && containsLeftMostCol)
-			return idxoid;
-	}
 
-	return InvalidOid;
+	/* The leftmost index field must not be an expression */
-}
+	keycol = indexInfo->ii_IndexAttrNumbers[0];
+	if (!AttributeNumberIsValid(keycol))
+		return false;
 
 	/*
- * Returns true if the index is usable for replica identity full. For details,
+	 * And the leftmost index field must reference the remote relation column.
- * see FindUsableIndexForReplicaIdentityFull.
+	 * This is because if it doesn't, the sequential scan is favorable over
+	 * index scan in most cases.
 	 */
-bool
+	if (attrmap->maplen <= AttrNumberGetAttrOffset(keycol) ||
-IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo)
+		attrmap->attnums[AttrNumberGetAttrOffset(keycol)] < 0)
-{
+		return false;
-	bool		is_btree = (indexInfo->ii_Am == BTREE_AM_OID);
-	bool		is_partial = (indexInfo->ii_Predicate != NIL);
-	bool		is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
 
-	return is_btree && !is_partial && !is_only_on_expression;
+	return true;
 }
 
 /*

src/backend/replication/logical/worker.c

@@ -140,6 +140,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include "access/genam.h"
 #include "access/table.h"
 #include "access/tableam.h"
 #include "access/twophase.h"
@@ -410,7 +411,7 @@ static void apply_handle_delete_internal(ApplyExecutionData *edata,
 										 ResultRelInfo *relinfo,
 										 TupleTableSlot *remoteslot,
 										 Oid localindexoid);
-static bool FindReplTupleInLocalRel(EState *estate, Relation localrel,
+static bool FindReplTupleInLocalRel(ApplyExecutionData *edata, Relation localrel,
 									LogicalRepRelation *remoterel,
 									Oid localidxoid,
 									TupleTableSlot *remoteslot,
@@ -2663,7 +2664,7 @@ apply_handle_update_internal(ApplyExecutionData *edata,
 	EvalPlanQualInit(&epqstate, estate, NULL, NIL, -1, NIL);
 	ExecOpenIndices(relinfo, false);
 
-	found = FindReplTupleInLocalRel(estate, localrel,
+	found = FindReplTupleInLocalRel(edata, localrel,
 									&relmapentry->remoterel,
 									localindexoid,
 									remoteslot, &localslot);
@@ -2816,7 +2817,7 @@ apply_handle_delete_internal(ApplyExecutionData *edata,
 	EvalPlanQualInit(&epqstate, estate, NULL, NIL, -1, NIL);
 	ExecOpenIndices(relinfo, false);
 
-	found = FindReplTupleInLocalRel(estate, localrel, remoterel, localindexoid,
+	found = FindReplTupleInLocalRel(edata, localrel, remoterel, localindexoid,
 									remoteslot, &localslot);
 
 	/* If found delete it. */
@@ -2855,12 +2856,13 @@ apply_handle_delete_internal(ApplyExecutionData *edata,
  * Local tuple, if found, is returned in '*localslot'.
  */
 static bool
-FindReplTupleInLocalRel(EState *estate, Relation localrel,
+FindReplTupleInLocalRel(ApplyExecutionData *edata, Relation localrel,
 						LogicalRepRelation *remoterel,
 						Oid localidxoid,
 						TupleTableSlot *remoteslot,
 						TupleTableSlot **localslot)
 {
+	EState	   *estate = edata->estate;
 	bool		found;
 
 	/*
@@ -2875,9 +2877,21 @@ FindReplTupleInLocalRel(EState *estate, Relation localrel,
 		   (remoterel->replident == REPLICA_IDENTITY_FULL));
 
 	if (OidIsValid(localidxoid))
+	{
+#ifdef USE_ASSERT_CHECKING
+		Relation	idxrel = index_open(localidxoid, AccessShareLock);
+
+		/* Index must be PK, RI, or usable for REPLICA IDENTITY FULL tables */
+		Assert(GetRelationIdentityOrPK(idxrel) == localidxoid ||
+			   IsIndexUsableForReplicaIdentityFull(BuildIndexInfo(idxrel),
+												   edata->targetRel->attrmap));
+		index_close(idxrel, AccessShareLock);
+#endif
+
 		found = RelationFindReplTupleByIndex(localrel, localidxoid,
 											 LockTupleExclusive,
 											 remoteslot, *localslot);
+	}
 	else
 		found = RelationFindReplTupleSeq(localrel, LockTupleExclusive,
 										 remoteslot, *localslot);
@@ -2995,7 +3009,7 @@ apply_handle_tuple_routing(ApplyExecutionData *edata,
 				bool		found;
 
 				/* Get the matching local tuple from the partition. */
-				found = FindReplTupleInLocalRel(estate, partrel,
+				found = FindReplTupleInLocalRel(edata, partrel,
 												&part_entry->remoterel,
 												part_entry->localindexoid,
 												remoteslot_part, &localslot);

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);
+extern bool IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo, AttrMap *attrmap);
 extern Oid	GetRelationIdentityOrPK(Relation rel);
 
 #endif							/* LOGICALRELATION_H */