On Thu, Oct 1, 2026 at 11:13 AM Zhijie Hou <[email protected]> wrote: > > Hi, > > On Wed, Sep 30, 2026 at 6:35 PM Nisha Moond <[email protected]> wrote: > > > > On Wed, Sep 30, 2026 at 2:47 PM Hayato Kuroda (Fujitsu) > > <[email protected]> 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
v5-0001-Use-the-relation-map-s-index-when-searching-delet.patch
Description: Binary data
v5-0002-Add-TAP-tests-for-update_deleted-detection-by-seq.patch
Description: Binary data
