On Thu, Oct 1, 2026 at 11:13 AM Zhijie Hou <houzhijie22@gmail.com> wrote:
>
> Hi,
>
> On Wed, Sep 30, 2026 at 6:35 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > On Wed, Sep 30, 2026 at 2:47 PM Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.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.
>
Done.
> 2.
> I think we shall rename 'idxoid' to 'identindex' or 'identidxoid' to make it
> clearer what should be passed?
>
okay I chose - 'identidxoid'.
> 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:
>
Okay I see removing it loses nothing. Let's remove it then.
> else
> return RelationFindDeletedTupleInfoSeq(localrel, relmapentry->idxisreplident
> ?
> localidxoid : InvalidOid, remoteslot,
>
> oldestxmin, delete_xid,
>
> delete_origin, delete_time);
Done.
> 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.
>
Removed.
~~~
Also updated commit message as suggested by Vignesh at [1].
Attached updated patches v5. There are a couple of optimizations and
comment improvements in 002(testcode) too.
[1] https://www.postgresql.org/message-id/CALDaNm3uXt7oiPRzUd-H%2BGFfiVx3H3BJExV2fRVNj_LpmDqCtg%40mail.gmail.com
--
Thanks,
Nisha
