From 75e3370e708840e4bd7d373c2d9773da6d1dcc6c Mon Sep 17 00:00:00 2001 From: Mikhail Nikalayeu Date: Thu, 27 Aug 2026 20:29:30 +0200 Subject: [PATCH v2] Don't re-derive in the scan what the apply worker knew about its index FindLogicalRepLocalIndex() picks the index to search the local relation by, either as the relation's replica identity or primary key, or as one usable for a REPLICA IDENTITY FULL remote relation. Which of the two it is decides whether the first index match is the tuple or every match has to be compared against the search slot. Only the OID is kept, though, so RelationFindReplTupleByIndex() works that out a second time on its own, by testing the index it is handed against GetRelationIdentityOrPK(). The second answer need not match the first. Apply holds only RowExclusiveLock, which conflicts with neither REINDEX CONCURRENTLY swapping the index for its rebuilt copy nor DROP INDEX CONCURRENTLY marking it invalid, so either can commit in between, and ExecOpenIndices() is where the invalidation lands. The scan then compares whole rows against a search slot that carries one only under REPLICA IDENTITY FULL, nothing matches, and the change is silently dropped as an update_missing conflict. Assertion builds trip over the identity check just above instead. Record which of the two it was in the relation map entry and pass it down, settling the question once. The new field goes into the padding after updatable, so that the back branches keep sizeof(LogicalRepRelMapEntry) and every member offset unchanged; there RelationFindReplTupleByIndex() likewise keeps its signature, and the new argument goes on a RelationFindReplTupleByIndexExt() that it wraps. HEAD keeps the single entry point. Oversight in 89e46da5e51. Author: Mikhail Nikalayeu Reviewed-by: Amit Kapila Reviewed-by: vignesh C Discussion: ................ Backpatch-through: 16, where it was introduced --- src/backend/executor/execReplication.c | 41 ++++++++++++++++++---- src/backend/replication/logical/relation.c | 19 +++++++--- src/backend/replication/logical/worker.c | 35 +++++++++++------- src/include/executor/executor.h | 5 +++ src/include/replication/logicalrelation.h | 7 ++++ 5 files changed, 84 insertions(+), 23 deletions(-) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index bb78d89265d..424c6dc2687 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -174,12 +174,20 @@ should_refetch_tuple(TM_Result res, TM_FailureData *tmfd) * * If a matching tuple is found, lock it with lockmode, fill the slot with its * contents, and return true. Return false otherwise. + * + * 'isIdxSafeToSkipDuplicates' says how the scan's matches are to be treated: + * if true, the index determines the tuple all by itself and the first match + * is taken; if false, every match is compared against 'searchslot', which must + * then carry a complete row -- the remote relation has REPLICA IDENTITY FULL. + * The caller decides; deriving it here would let concurrent DDL flip it after + * the index was chosen. */ bool -RelationFindReplTupleByIndex(Relation rel, Oid idxoid, - LockTupleMode lockmode, - TupleTableSlot *searchslot, - TupleTableSlot *outslot) +RelationFindReplTupleByIndexExt(Relation rel, Oid idxoid, + bool isIdxSafeToSkipDuplicates, + LockTupleMode lockmode, + TupleTableSlot *searchslot, + TupleTableSlot *outslot) { ScanKeyData skey[INDEX_MAX_KEYS]; int skey_attoff; @@ -189,13 +197,10 @@ RelationFindReplTupleByIndex(Relation rel, Oid idxoid, Relation idxrel; bool found; TypeCacheEntry **eq = NULL; - bool isIdxSafeToSkipDuplicates; /* Open the index. */ idxrel = index_open(idxoid, RowExclusiveLock); - isIdxSafeToSkipDuplicates = (GetRelationIdentityOrPK(rel) == idxoid); - InitDirtySnapshot(snap); /* Build scan key. */ @@ -341,6 +346,28 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, return true; } +/* + * Same as RelationFindReplTupleByIndexExt(), except that it derives + * 'isIdxSafeToSkipDuplicates' the way that function did before the argument + * was added. Kept with its original signature so that the ABI of this branch + * does not change; no in-core caller uses it, precisely because deriving the + * value here is what concurrent DDL can flip. + */ +bool +RelationFindReplTupleByIndex(Relation rel, Oid idxoid, + LockTupleMode lockmode, + TupleTableSlot *searchslot, + TupleTableSlot *outslot) +{ + bool isIdxSafeToSkipDuplicates; + + isIdxSafeToSkipDuplicates = (GetRelationIdentityOrPK(rel) == idxoid); + + return RelationFindReplTupleByIndexExt(rel, idxoid, + isIdxSafeToSkipDuplicates, + lockmode, searchslot, outslot); +} + /* * Search the relation 'rel' for tuple using the sequential scan. * diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c index 0915b4d8b15..ad21732a7f3 100644 --- a/src/backend/replication/logical/relation.c +++ b/src/backend/replication/logical/relation.c @@ -55,7 +55,7 @@ typedef struct LogicalRepPartMapEntry } LogicalRepPartMapEntry; static Oid FindLogicalRepLocalIndex(Relation localrel, LogicalRepRelation *remoterel, - AttrMap *attrMap); + AttrMap *attrMap, bool *isidentity); /* * Relcache invalidation callback for our relation map cache. @@ -484,7 +484,8 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode) * on the relation). */ entry->localindexoid = FindLogicalRepLocalIndex(entry->localrel, remoterel, - entry->attrmap); + entry->attrmap, + &entry->isidentity); entry->localrelvalid = true; } @@ -756,7 +757,8 @@ logicalrep_partition_open(LogicalRepRelMapEntry *root, * anything in the LogicalRepPartMapContext (hence CacheMemoryContext). */ entry->localindexoid = FindLogicalRepLocalIndex(partrel, remoterel, - entry->attrmap); + entry->attrmap, + &entry->isidentity); entry->localrelvalid = true; @@ -917,13 +919,19 @@ GetRelationIdentityOrPK(Relation rel) /* * Returns the index oid if we can use an index for subscriber. Otherwise, * returns InvalidOid. + * + * '*isidentity' is set to whether the returned index was chosen as the + * relation's replica identity or primary key, rather than as one usable + * for a REPLICA IDENTITY FULL remote relation. */ static Oid FindLogicalRepLocalIndex(Relation localrel, LogicalRepRelation *remoterel, - AttrMap *attrMap) + AttrMap *attrMap, bool *isidentity) { Oid idxoid; + *isidentity = false; + /* * We never need index oid for partitioned tables, always rely on leaf * partition's index. @@ -936,7 +944,10 @@ FindLogicalRepLocalIndex(Relation localrel, LogicalRepRelation *remoterel, */ idxoid = GetRelationIdentityOrPK(localrel); if (OidIsValid(idxoid)) + { + *isidentity = true; return idxoid; + } if (remoterel->replident == REPLICA_IDENTITY_FULL) { diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index 5b53b5fb64e..fa7a26e3edd 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -380,14 +380,17 @@ static void apply_handle_update_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo, TupleTableSlot *remoteslot, LogicalRepTupleData *newtup, - Oid localindexoid); + Oid localindexoid, + bool isidentity); static void apply_handle_delete_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo, TupleTableSlot *remoteslot, - Oid localindexoid); + Oid localindexoid, + bool isidentity); static bool FindReplTupleInLocalRel(ApplyExecutionData *edata, Relation localrel, LogicalRepRelation *remoterel, Oid localidxoid, + bool isidentity, TupleTableSlot *remoteslot, TupleTableSlot **localslot); static void apply_handle_tuple_routing(ApplyExecutionData *edata, @@ -2657,7 +2660,8 @@ apply_handle_update(StringInfo s) remoteslot, &newtup, CMD_UPDATE); else apply_handle_update_internal(edata, edata->targetRelInfo, - remoteslot, &newtup, rel->localindexoid); + remoteslot, &newtup, rel->localindexoid, + rel->isidentity); finish_edata(edata); @@ -2682,7 +2686,7 @@ apply_handle_update_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo, TupleTableSlot *remoteslot, LogicalRepTupleData *newtup, - Oid localindexoid) + Oid localindexoid, bool isidentity) { EState *estate = edata->estate; LogicalRepRelMapEntry *relmapentry = edata->targetRel; @@ -2694,11 +2698,12 @@ apply_handle_update_internal(ApplyExecutionData *edata, MemoryContext oldctx; EvalPlanQualInit(&epqstate, estate, NULL, NIL, -1, NIL); + ExecOpenIndices(relinfo, false); found = FindReplTupleInLocalRel(edata, localrel, &relmapentry->remoterel, - localindexoid, + localindexoid, isidentity, remoteslot, &localslot); /* @@ -2840,7 +2845,8 @@ apply_handle_delete(StringInfo s) ExecOpenIndices(relinfo, false); apply_handle_delete_internal(edata, relinfo, - remoteslot, rel->localindexoid); + remoteslot, rel->localindexoid, + rel->isidentity); ExecCloseIndices(relinfo); } @@ -2866,7 +2872,7 @@ static void apply_handle_delete_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo, TupleTableSlot *remoteslot, - Oid localindexoid) + Oid localindexoid, bool isidentity) { EState *estate = edata->estate; Relation localrel = relinfo->ri_RelationDesc; @@ -2884,6 +2890,7 @@ apply_handle_delete_internal(ApplyExecutionData *edata, RelationGetIndexList(localrel) == NIL); found = FindReplTupleInLocalRel(edata, localrel, remoterel, localindexoid, + isidentity, remoteslot, &localslot); /* If found delete it. */ @@ -2934,6 +2941,7 @@ static bool FindReplTupleInLocalRel(ApplyExecutionData *edata, Relation localrel, LogicalRepRelation *remoterel, Oid localidxoid, + bool isidentity, TupleTableSlot *remoteslot, TupleTableSlot **localslot) { @@ -2957,16 +2965,17 @@ FindReplTupleInLocalRel(ApplyExecutionData *edata, Relation localrel, Relation idxrel = index_open(localidxoid, AccessShareLock); /* Index must be PK, RI, or usable for REPLICA IDENTITY FULL tables */ - Assert(GetRelationIdentityOrPK(localrel) == localidxoid || + Assert(isidentity || (remoterel->replident == REPLICA_IDENTITY_FULL && IsIndexUsableForReplicaIdentityFull(idxrel, edata->targetRel->attrmap))); index_close(idxrel, AccessShareLock); #endif - found = RelationFindReplTupleByIndex(localrel, localidxoid, - LockTupleExclusive, - remoteslot, *localslot); + found = RelationFindReplTupleByIndexExt(localrel, localidxoid, + isidentity, + LockTupleExclusive, + remoteslot, *localslot); } else found = RelationFindReplTupleSeq(localrel, LockTupleExclusive, @@ -3067,7 +3076,8 @@ apply_handle_tuple_routing(ApplyExecutionData *edata, case CMD_DELETE: apply_handle_delete_internal(edata, partrelinfo, remoteslot_part, - part_entry->localindexoid); + part_entry->localindexoid, + part_entry->isidentity); break; case CMD_UPDATE: @@ -3090,6 +3100,7 @@ apply_handle_tuple_routing(ApplyExecutionData *edata, found = FindReplTupleInLocalRel(edata, partrel, &part_entry->remoterel, part_entry->localindexoid, + part_entry->isidentity, remoteslot_part, &localslot); if (!found) { diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index b6697c0fc75..61430d06b12 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -760,6 +760,11 @@ extern bool RelationFindReplTupleByIndex(Relation rel, Oid idxoid, LockTupleMode lockmode, TupleTableSlot *searchslot, TupleTableSlot *outslot); +extern bool RelationFindReplTupleByIndexExt(Relation rel, Oid idxoid, + bool isIdxSafeToSkipDuplicates, + LockTupleMode lockmode, + TupleTableSlot *searchslot, + TupleTableSlot *outslot); extern bool RelationFindReplTupleSeq(Relation rel, LockTupleMode lockmode, TupleTableSlot *searchslot, TupleTableSlot *outslot); diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h index 7a561a8e8d8..6af30b411a2 100644 --- a/src/include/replication/logicalrelation.h +++ b/src/include/replication/logicalrelation.h @@ -32,6 +32,13 @@ typedef struct LogicalRepRelMapEntry Relation localrel; /* relcache entry (NULL when closed) */ AttrMap *attrmap; /* map of local attributes to remote ones */ bool updatable; /* Can apply updates/deletes? */ + bool isidentity; /* whether localindexoid is the relation's + * replica identity or primary key, rather + * than an index usable for a REPLICA + * IDENTITY FULL remote relation. Placed + * here to fit in existing padding, so + * that the struct layout is unchanged + * from earlier minor releases. */ Oid localindexoid; /* which index to use, or InvalidOid if none */ /* Sync state. */ -- 2.43.0