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

On Mon, Sep 21, 2026 at 2:03 PM Zhijie Hou <houzhijie22(at)gmail(dot)com> wrote:
>
> 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?
>

LGTM.

> 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.
>

I think this is a lower priority case. Let's do this separately.

--
With Regards,
Amit Kapila.