Re: Logical replication can lose an update after concurrent index invalidation

On Fri, Aug 28, 2026 at 4:45 PM Mihail Nikalayeu
<mihailnikalayeu(at)gmail(dot)com> wrote:
>
> In v2 I put a new flag into padding and used the same pattern for the
> function as in 9e47718250d.
>

Few comments:
=============
* The existing calls that use "Oid localindexoid" parameter grew a
parallel "bool isidentity" beside it. Can we consider passing the
LogicalRepRelMapEntry * instead that collapses two parameters into
one?

* How about naming struct member as idxisreplident instead of
isidentity as that reads closer to the catalog vocabulary?

*
FindReplTupleInLocalRel(ApplyExecutionData *edata, Relation localrel,
LogicalRepRelation *remoterel,
Oid localidxoid,
+ bool isidentity,
TupleTableSlot *remoteslot,
TupleTableSlot **localslot)
{
@@ -3239,14 +3252,14 @@ 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 ||

The Assert(isidentity ...) part actually verifies nothing when
isidentity is true, previously the assert was checking it against
catalog value due to which it trips, so we need to change this assert
but I think we can do better than proposed. Two ideas:

a. Something on the lines of:
#ifdef USE_ASSERT_CHECKING
if (!isidentity)
{
Relation idxrel =
index_open(localidxoid, AccessShareLock);

/* Not the identity, so it must be usable for
REPLICA IDENTITY FULL */
Assert(remoterel->replident == REPLICA_IDENTITY_FULL);
Assert(IsIndexUsableForReplicaIdentityFull(idxrel,

edata->targetRel->attrmap));
index_close(idxrel, AccessShareLock);
}
#endif

b. An RI or PK index is unique and non-partial, and neither DIC nor
REINDEX CONCURRENTLY changes that, so something like the following
should also work:
#ifdef USE_ASSERT_CHECKING
Relation idxrel = index_open(localidxoid,
AccessShareLock);

if (isidentity)
{
/* A replica identity or primary key index
identifies the row. */
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(remoterel->replident == REPLICA_IDENTITY_FULL);
Assert(IsIndexUsableForReplicaIdentityFull(idxrel,

edata->targetRel->attrmap));
}
index_close(idxrel, AccessShareLock);
#endif

> abidiff now shows that:
>
> 'struct LogicalRepRelMapEntry' changed:
> type size hasn't changed
> 1 data member insertion:
> 'bool isidentity', at offset 89 (in bytes)
>
> I think it is ok for such a type. I couldn't find any usage in extensions.
>
> Should we update .abi-compliance-history in such a case?
>

Yes, I think this deserves a update in .abi-compliance-history but as
noted in the file comments (In general, entries should be added
reactively after an abi-compliance-check buildfarm failure.), we can
add it afterwards but certainly note down the need of same in the
commit message. For example, see commits: 1cd020324ef5 and a0f98b2755.

BTW, I have a question, how can we ascertain that we need a new API
for RelationFindReplTupleByIndex in back-branches? Yes, it is an
exposed API but on a quick search via AI, I couldn't find any of its
usage, can you once re-verify and share your opinion on the same?

--
With Regards,
Amit Kapila.