From da4eb0de3eda7792b1f8fd0de7ff19e4dfeadba5 Mon Sep 17 00:00:00 2001 From: Masahiko Sawada Date: Tue, 18 Jul 2023 15:11:27 +0900 Subject: [PATCH v3] 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 actually, 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 checked by RemoteRelContainsLeftMostColumnOnIdx(). This commit removes IsIndexOnlyExpression() and RemoteRelContainsLeftMostColumnOnIdx() and all checks for usable indexes for REPLICA IDENTITY FULL tables are now performed by IsIndexUsableForReplicaIdentityFull(). Reviewed-by: Discussion: https://postgr.es/m/ --- src/backend/executor/execReplication.c | 9 --- src/backend/replication/logical/relation.c | 92 ++++++++-------------- src/backend/replication/logical/worker.c | 24 ++++-- src/include/replication/logicalrelation.h | 2 +- 4 files changed, 51 insertions(+), 76 deletions(-) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index e776524227..81f27042bc 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -222,16 +222,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; diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c index ed57e5d2b6..ad960f7bad 100644 --- a/src/backend/replication/logical/relation.c +++ b/src/backend/replication/logical/relation.c @@ -734,58 +734,9 @@ logicalrep_partition_open(LogicalRepRelMapEntry *root, return entry; } -/* - * Returns true if the given index consists only of expressions such as: - * CREATE INDEX idx ON table(foo(col)); - * - * Returns false even if there is one column reference: - * CREATE INDEX idx ON table(foo(col), col_2); - */ -static bool -IsIndexOnlyOnExpression(IndexInfo *indexInfo) -{ - for (int i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++) - { - AttrNumber attnum = indexInfo->ii_IndexAttrNumbers[i]; - - if (AttributeNumberIsValid(attnum)) - return false; - } - - return true; -} - -/* - * Returns true if the attrmap contains the leftmost column of the index. - * Otherwise returns false. - * - * 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 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 relation. * * 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 @@ -815,19 +766,16 @@ FindUsableIndexForReplicaIdentityFull(Relation localrel, AttrMap *attrmap) { Oid idxoid = lfirst_oid(lc); bool isUsableIdx; - bool containsLeftMostCol; Relation idxRel; IndexInfo *idxInfo; idxRel = index_open(idxoid, AccessShareLock); idxInfo = BuildIndexInfo(idxRel); - isUsableIdx = IsIndexUsableForReplicaIdentityFull(idxInfo); - containsLeftMostCol = - RemoteRelContainsLeftMostColumnOnIdx(idxInfo, attrmap); + isUsableIdx = IsIndexUsableForReplicaIdentityFull(idxInfo, attrmap); index_close(idxRel, AccessShareLock); /* Return the first eligible index found */ - if (isUsableIdx && containsLeftMostCol) + if (isUsableIdx) return idxoid; } @@ -835,11 +783,13 @@ FindUsableIndexForReplicaIdentityFull(Relation localrel, AttrMap *attrmap) } /* - * Returns true if the index is usable for replica identity full. For details, - * see FindUsableIndexForReplicaIdentityFull. + * 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. * - * Currently, only Btree and Hash indexes can be returned as usable. This - * is due to following reasons: + * 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(). @@ -851,16 +801,36 @@ FindUsableIndexForReplicaIdentityFull(Relation localrel, AttrMap *attrmap) * * XXX: Note that BRIN and GIN indexes do not implement "amgettuple" which * will be used later to fetch the tuples. See RelationFindReplTupleByIndex(). + * + * 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. */ bool -IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo) +IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo, 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) return false; if (indexInfo->ii_Predicate != NIL) return false; - if (IsIndexOnlyOnExpression(indexInfo)) + + Assert(indexInfo->ii_NumIndexAttrs >= 1); + + /* The leftmost index field must not be an expression */ + keycol = indexInfo->ii_IndexAttrNumbers[0]; + if (!AttributeNumberIsValid(keycol)) + return false; + + /* + * And must reference the remote relation column. This is because if it + * doesn't, the sequential scan is favorable over index scan in most + * cases.. + */ + if (attrmap->maplen <= AttrNumberGetAttrOffset(keycol) || + attrmap->attnums[AttrNumberGetAttrOffset(keycol)] < 0) return false; #ifdef USE_ASSERT_CHECKING diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index dd353fd1cb..0dd69739fb 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -140,6 +140,7 @@ #include #include +#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); diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h index 921b9974db..3f4d906d74 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); +extern bool IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo, AttrMap *attrmap); extern Oid GetRelationIdentityOrPK(Relation rel); #endif /* LOGICALRELATION_H */ -- 2.31.1