Hi, On Fri, Sep 18, 2026 at 8:55 PM Amit Kapila <[email protected]> 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
