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

Hi,

On Fri, Sep 18, 2026 at 8:55 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> Fixed in the attached. Apart from this I changed multiple comments to
> make those clear. One notable change is, I moved the newly added
> boolean after localindexoid as it reads better there because then we
> don't need to forward reference the fields. For back-branches, if it
> needs to be moved to an earlier location then we can do that in those
> versions but for HEAD and 19, the new location seems better.
>
> Also, shall we keep just one test, say Drop Index Concurrently instead
> of two as both tests do the same thing in a slightly different way? I
> have not done that but if you agree please update the patch
> accordingly.

I also think one DROP INDEX CONCURRENTLY test is sufficient to cover this bug.

The patch looks mostly good to me, I just have a few minor comments:

1.

extern bool RelationFindReplTupleByIndex(Relation rel, Oid idxoid,
+ bool isIdxSafeToSkipDuplicates,

Shall we use a shorter parameter name, like skipduplicates, to stay consistent
with the other parameters of this function?

2.

+ * XXX This can disagree with the index the apply worker searched by,
+ * see FindReplTupleInLocalRel(). It may not even be one that
+ * ExecOpenIndices() locked.
*/
Oid replica_index = GetRelationIdentityOrPK(localrel);

Just in case we want to fix this in the current patch, I think one option that
avoids adding a new parameter to multiple public functions would be to cache the
replica identity index OID in remote_ctx and expose a function to retrieve it.
We could then call that function here instead of GetRelationIdentityOrPK().

Otherwise, we would need to add a new index OID parameter to
ExecSimpleRelationUpdate, even though the identity index isn't otherwise used
there - and ReportApplyConflict would need a new parameter as well.

Best Regards,
Zhijie Hou