Hi,
On Fri, Sep 18, 2026 at 8:55 PM Amit Kapila <amit.kapila16@gmail.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
