From 94fe6cae4fcc1357991ef11faea83cf826eb71c9 Mon Sep 17 00:00:00 2001 From: Mikhail Nikalayeu Date: Thu, 27 Aug 2026 20:32:48 +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 | 38 +++++++++++++------- src/include/executor/executor.h | 5 +++ src/include/replication/logicalrelation.h | 7 ++++ 5 files changed, 86 insertions(+), 24 deletions(-) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 61ddbe548f3..fc2e9ec8026 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -130,12 +130,20 @@ build_replindex_scan_key(ScanKey skey, Relation rel, Relation idxrel, * * 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; @@ -145,13 +153,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. */ @@ -323,6 +328,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 c6730b58701..94e939113be 100644 --- a/src/backend/replication/logical/relation.c +++ b/src/backend/replication/logical/relation.c @@ -53,7 +53,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. @@ -451,7 +451,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; } @@ -724,7 +725,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; @@ -858,13 +860,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. @@ -877,7 +885,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 76367458768..347946a9f0b 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -406,14 +406,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, @@ -2647,7 +2650,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); @@ -2672,7 +2676,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; @@ -2683,11 +2687,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); ExecClearTuple(remoteslot); @@ -2802,7 +2807,8 @@ apply_handle_delete(StringInfo s) remoteslot, NULL, CMD_DELETE); else apply_handle_delete_internal(edata, edata->targetRelInfo, - remoteslot, rel->localindexoid); + remoteslot, rel->localindexoid, + rel->isidentity); finish_edata(edata); @@ -2826,7 +2832,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; @@ -2839,6 +2845,7 @@ apply_handle_delete_internal(ApplyExecutionData *edata, ExecOpenIndices(relinfo, false); found = FindReplTupleInLocalRel(edata, localrel, remoterel, localindexoid, + isidentity, remoteslot, &localslot); /* If found delete it. */ @@ -2880,6 +2887,7 @@ static bool FindReplTupleInLocalRel(ApplyExecutionData *edata, Relation localrel, LogicalRepRelation *remoterel, Oid localidxoid, + bool isidentity, TupleTableSlot *remoteslot, TupleTableSlot **localslot) { @@ -2903,15 +2911,16 @@ 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(idxrel) == localidxoid || + Assert(isidentity || IsIndexUsableForReplicaIdentityFull(BuildIndexInfo(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, @@ -3012,7 +3021,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: @@ -3033,6 +3043,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) { @@ -3130,7 +3141,8 @@ apply_handle_tuple_routing(ApplyExecutionData *edata, /* DELETE old tuple found in the old partition. */ apply_handle_delete_internal(edata, partrelinfo, localslot, - part_entry->localindexoid); + part_entry->localindexoid, + part_entry->isidentity); /* INSERT new tuple into the new partition. */ diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index deba7a64af0..26a9c7c73a2 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -655,6 +655,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 3f4d906d741..36b5103ca8f 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