From 70486b1d7544f94b042da828f048be927923b9e2 Mon Sep 17 00:00:00 2001 From: Mikhail Nikalayeu Date: Thu, 3 Sep 2026 11:25:04 +0200 Subject: [PATCH v5_REL16] Fix tuple search during apply after concurrent index DDL. When the apply worker searches the local relation by index, it can take the first match as the row only if the index is the relation's replica identity or primary key. Otherwise every match has to be compared with the search slot, which holds a complete row only under REPLICA IDENTITY FULL. Only the index OID was saved, so the scan worked this out a second time from the catalogs, and the two answers can differ. Apply holds only RowExclusiveLock, which doesn't conflict with DROP INDEX CONCURRENTLY or REINDEX CONCURRENTLY, so either can demote the chosen index in between. Nothing matches, so the change is silently dropped as an update_missing conflict, and assert-enabled builds fail. Fix this by recording the answer as idxisreplident in the relation map entry and passing the entry down to FindReplTupleInLocalRel(), so it is settled once. Oversight in 89e46da5e5. Author: Mikhail Nikalayeu Reviewed-by: Amit Kapila Reviewed-by: vignesh C Reviewed-by: Zhijie Hou Discussion: https://postgr.es/m/CADzfLwUJovFcnknCC9wjZKECX9xecgnGzC2r2TMV8h4QDD_jwQ@mail.gmail.com Backpatch-through: 16, where it was introduced --- src/backend/executor/execReplication.c | 37 +++++++--- src/backend/replication/logical/relation.c | 18 +++-- src/backend/replication/logical/worker.c | 83 ++++++++++++---------- src/include/executor/executor.h | 5 ++ src/include/replication/logicalrelation.h | 4 ++ 5 files changed, 98 insertions(+), 49 deletions(-) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 61ddbe548f3..cb63df021ec 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -130,12 +130,17 @@ 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. + * + * 'skipduplicates' specifies whether the first matching tuple can be used + * without comparing it against 'searchslot'. If false, all matching tuples are + * compared against 'searchslot', which must contain a complete row. */ bool -RelationFindReplTupleByIndex(Relation rel, Oid idxoid, - LockTupleMode lockmode, - TupleTableSlot *searchslot, - TupleTableSlot *outslot) +RelationFindReplTupleByIndexExt(Relation rel, Oid idxoid, + bool skipduplicates, + LockTupleMode lockmode, + TupleTableSlot *searchslot, + TupleTableSlot *outslot) { ScanKeyData skey[INDEX_MAX_KEYS]; int skey_attoff; @@ -145,13 +150,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. */ @@ -172,7 +174,7 @@ retry: * Avoid expensive equality check if the index is primary key or * replica identity index. */ - if (!isIdxSafeToSkipDuplicates) + if (!skipduplicates) { if (eq == NULL) eq = palloc0(sizeof(*eq) * outslot->tts_tupleDescriptor->natts); @@ -257,6 +259,25 @@ retry: return found; } +/* + * ABI-compatible wrapper to emulate old version of the above function. + * Do not call this version in new code. + */ +bool +RelationFindReplTupleByIndex(Relation rel, Oid idxoid, + LockTupleMode lockmode, + TupleTableSlot *searchslot, + TupleTableSlot *outslot) +{ + bool skipduplicates; + + skipduplicates = (GetRelationIdentityOrPK(rel) == idxoid); + + return RelationFindReplTupleByIndexExt(rel, idxoid, + skipduplicates, + lockmode, searchslot, outslot); +} + /* * Compare the tuples in the slots by checking if they have equal values. */ diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c index c6730b58701..87dbfa687c0 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 *idxisreplident); /* * 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->idxisreplident); 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->idxisreplident); entry->localrelvalid = true; @@ -858,13 +860,18 @@ GetRelationIdentityOrPK(Relation rel) /* * Returns the index oid if we can use an index for subscriber. Otherwise, * returns InvalidOid. + * + * '*idxisreplident' is true if the returned index is the relation's replica + * identity or primary key, and false otherwise. */ static Oid FindLogicalRepLocalIndex(Relation localrel, LogicalRepRelation *remoterel, - AttrMap *attrMap) + AttrMap *attrMap, bool *idxisreplident) { Oid idxoid; + *idxisreplident = false; + /* * We never need index oid for partitioned tables, always rely on leaf * partition's index. @@ -877,7 +884,10 @@ FindLogicalRepLocalIndex(Relation localrel, LogicalRepRelation *remoterel, */ idxoid = GetRelationIdentityOrPK(localrel); if (OidIsValid(idxoid)) + { + *idxisreplident = 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 85cbc734f5f..ee0229dbf88 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -405,15 +405,13 @@ static void apply_handle_insert_internal(ApplyExecutionData *edata, static void apply_handle_update_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo, TupleTableSlot *remoteslot, - LogicalRepTupleData *newtup, - Oid localindexoid); + LogicalRepTupleData *newtup); static void apply_handle_delete_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo, TupleTableSlot *remoteslot, - Oid localindexoid); + LogicalRepRelMapEntry *relmapentry); static bool FindReplTupleInLocalRel(ApplyExecutionData *edata, Relation localrel, - LogicalRepRelation *remoterel, - Oid localidxoid, + LogicalRepRelMapEntry *relmapentry, TupleTableSlot *remoteslot, TupleTableSlot **localslot); static void apply_handle_tuple_routing(ApplyExecutionData *edata, @@ -2522,11 +2520,8 @@ check_relation_updatable(LogicalRepRelMapEntry *rel) if (rel->updatable) return; - /* - * We are in error mode so it's fine this is somewhat slow. It's better to - * give user correct error. - */ - if (OidIsValid(GetRelationIdentityOrPK(rel->localrel))) + /* Use the entry, so this matches what updatable was decided from. */ + if (rel->idxisreplident) { ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), @@ -2653,7 +2648,7 @@ apply_handle_update(StringInfo s) remoteslot, &newtup, CMD_UPDATE); else apply_handle_update_internal(edata, edata->targetRelInfo, - remoteslot, &newtup, rel->localindexoid); + remoteslot, &newtup); finish_edata(edata); @@ -2677,8 +2672,7 @@ static void apply_handle_update_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo, TupleTableSlot *remoteslot, - LogicalRepTupleData *newtup, - Oid localindexoid) + LogicalRepTupleData *newtup) { EState *estate = edata->estate; LogicalRepRelMapEntry *relmapentry = edata->targetRel; @@ -2691,9 +2685,7 @@ apply_handle_update_internal(ApplyExecutionData *edata, EvalPlanQualInit(&epqstate, estate, NULL, NIL, -1, NIL); ExecOpenIndices(relinfo, false); - found = FindReplTupleInLocalRel(edata, localrel, - &relmapentry->remoterel, - localindexoid, + found = FindReplTupleInLocalRel(edata, localrel, relmapentry, remoteslot, &localslot); ExecClearTuple(remoteslot); @@ -2808,7 +2800,7 @@ apply_handle_delete(StringInfo s) remoteslot, NULL, CMD_DELETE); else apply_handle_delete_internal(edata, edata->targetRelInfo, - remoteslot, rel->localindexoid); + remoteslot, rel); finish_edata(edata); @@ -2832,11 +2824,10 @@ static void apply_handle_delete_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo, TupleTableSlot *remoteslot, - Oid localindexoid) + LogicalRepRelMapEntry *relmapentry) { EState *estate = edata->estate; Relation localrel = relinfo->ri_RelationDesc; - LogicalRepRelation *remoterel = &edata->targetRel->remoterel; EPQState epqstate; TupleTableSlot *localslot; bool found; @@ -2844,7 +2835,7 @@ apply_handle_delete_internal(ApplyExecutionData *edata, EvalPlanQualInit(&epqstate, estate, NULL, NIL, -1, NIL); ExecOpenIndices(relinfo, false); - found = FindReplTupleInLocalRel(edata, localrel, remoterel, localindexoid, + found = FindReplTupleInLocalRel(edata, localrel, relmapentry, remoteslot, &localslot); /* If found delete it. */ @@ -2880,16 +2871,20 @@ apply_handle_delete_internal(ApplyExecutionData *edata, * the corresponding local relation using either replica identity index, * primary key, index or if needed, sequential scan. * + * 'relmapentry' is the relation map entry for 'localrel'; it tells which + * index to use, if any, and whether that index is the relation's replica + * identity or primary key. + * * Local tuple, if found, is returned in '*localslot'. */ static bool FindReplTupleInLocalRel(ApplyExecutionData *edata, Relation localrel, - LogicalRepRelation *remoterel, - Oid localidxoid, + LogicalRepRelMapEntry *relmapentry, TupleTableSlot *remoteslot, TupleTableSlot **localslot) { EState *estate = edata->estate; + Oid localidxoid = relmapentry->localindexoid; bool found; /* @@ -2901,23 +2896,41 @@ FindReplTupleInLocalRel(ApplyExecutionData *edata, Relation localrel, *localslot = table_slot_create(localrel, &estate->es_tupleTable); Assert(OidIsValid(localidxoid) || - (remoterel->replident == REPLICA_IDENTITY_FULL)); + (relmapentry->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)); + if (relmapentry->idxisreplident) + { + /* + * We cannot assert this is still the replica identity or primary + * key. DROP INDEX CONCURRENTLY and REINDEX CONCURRENTLY clear + * indisvalid and indisreplident without conflicting with our + * RowExclusiveLock, so GetRelationIdentityOrPK() may no longer + * return it. Unique and non-partial is what the scan actually + * relies on, and no DDL can take those away. + */ + Assert(idxrel->rd_index->indisunique); + Assert(heap_attisnull(idxrel->rd_indextuple, + Anum_pg_index_indpred, NULL)); + } + else + { + /* Otherwise every match is compared, so we need a whole row. */ + Assert(relmapentry->remoterel.replident == REPLICA_IDENTITY_FULL); + Assert(IsIndexUsableForReplicaIdentityFull(BuildIndexInfo(idxrel), + relmapentry->attrmap)); + } index_close(idxrel, AccessShareLock); #endif - found = RelationFindReplTupleByIndex(localrel, localidxoid, - LockTupleExclusive, - remoteslot, *localslot); + found = RelationFindReplTupleByIndexExt(localrel, localidxoid, + relmapentry->idxisreplident, + LockTupleExclusive, + remoteslot, *localslot); } else found = RelationFindReplTupleSeq(localrel, LockTupleExclusive, @@ -3017,8 +3030,7 @@ apply_handle_tuple_routing(ApplyExecutionData *edata, case CMD_DELETE: apply_handle_delete_internal(edata, partrelinfo, - remoteslot_part, - part_entry->localindexoid); + remoteslot_part, part_entry); break; case CMD_UPDATE: @@ -3036,9 +3048,7 @@ apply_handle_tuple_routing(ApplyExecutionData *edata, bool found; /* Get the matching local tuple from the partition. */ - found = FindReplTupleInLocalRel(edata, partrel, - &part_entry->remoterel, - part_entry->localindexoid, + found = FindReplTupleInLocalRel(edata, partrel, part_entry, remoteslot_part, &localslot); if (!found) { @@ -3135,8 +3145,7 @@ apply_handle_tuple_routing(ApplyExecutionData *edata, /* DELETE old tuple found in the old partition. */ apply_handle_delete_internal(edata, partrelinfo, - localslot, - part_entry->localindexoid); + localslot, part_entry); /* INSERT new tuple into the new partition. */ diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index deba7a64af0..c82cd39ec08 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 skipduplicates, + 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..3ba6ddbcd6e 100644 --- a/src/include/replication/logicalrelation.h +++ b/src/include/replication/logicalrelation.h @@ -32,6 +32,10 @@ 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 idxisreplident; /* is localindexoid the relation's replica + * identity or primary key, rather than an + * index usable for a REPLICA IDENTITY FULL + * remote relation? */ Oid localindexoid; /* which index to use, or InvalidOid if none */ /* Sync state. */ -- 2.55.0