Re: Fix apply worker crash when subscriber table has only a deferrable primary key

Hi,

On Wed, Sep 30, 2026 at 6:35 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Wed, Sep 30, 2026 at 2:47 PM Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> > Hi Amit,
> >
> > > The caller of RelationFindDeletedTupleInfoSeq() already has
> > > information localindexoid/idxisreplident, why can't we use those
> > > values instead of computing the same information again? I am afraid
> > > that computing such an information again could lead to symptoms what
> > > we fixed in the recent commit
> > > ad36e3608c8cb6f0848737ec81e548d4d3a0af3c.
> >
> > Your point meant not to get the info from the relcache because it can be
> > invalidated by the concurrent DDLs, right? I think it's possible, but the
> > additional computation might be needed since bitmapset for key columns are not
> > cached on the relmap now. Attached top-up patch implemented the idea, can you
> > see it's same as your expectation?
> > Test code just showed my understanding, not intended to be included for now.
> >
>
> My understanding is also the same. Thanks for the patch; I’ve verified the fix.
>
> Here is the updated version, merged with v3-0001.
>
> v4-0001: Updated stale comments in RelationFindDeletedTupleInfoSeq(),
> corrected the new comments in FindDeletedTupleInLocalRel(), and added
> an assertion that the whole row is compared only when the publisher
> uses REPLICA IDENTITY FULL.
> v4-0002: Moved both tests in 035_conflicts.pl into a separate patch:
> the deferrable primary key case and your concurrent DROP INDEX case,
> as these are not intended for commit.
>
> Both patches apply cleanly on HEAD and PG19, and I’ve tested them on
> both branches.

Thanks for the patch, it works for me. I just have a few comments:

1.
+ * If 'idxoid' is valid, its key columns are used for comparison. The index
+ * must be an identity or primary key index. Otherwise, all columns are used
+ * for comparison.

We could move these comments before the explanation of "'oldestxmin' acts as a
cutoff transaction ID" so they follow the parameter order.

2.
I think we shall rename 'idxoid' to 'identindex' or 'identidxoid' to make it
clearer what should be passed?

3.
+ /* Without such an index, every column is compared. */
+ Assert(relmapentry->idxisreplident ||
+ relmapentry->remoterel.replident == REPLICA_IDENTITY_FULL);

I think we could remove this Assert, since check_relation_updatable and
the related logic already guarantee it, and the check happens not far from
here. If we really want to keep it, we could follow Kuroda-san's suggestion and
move this Assert into RelationFindDeletedTupleInfoSeq(), so the caller code
stays simpler, like:

else
return RelationFindDeletedTupleInfoSeq(localrel, relmapentry->idxisreplident
?
localidxoid : InvalidOid, remoteslot,

oldestxmin, delete_xid,

delete_origin, delete_time);
4.
+ bms_free(indexbitmap);

Similar to the other logicalrep functions here, we can remove this free, the
memory context is reset for each change anyway.

Best Regards,
Zhijie Hou