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

Dear Nisha,

While testing another feature patch, I came across this base-code
issue. If a subscriber table's only key is a DEFERRABLE primary key,
and the published table does not use REPLICA IDENTITY FULL, then
UPDATE and DELETE apply trips an assertion.

Good catch, I confirmed the same.

The cause is that two code paths disagree.
logicalrep_rel_mark_updatable() finds no replica identity bitmap and
falls back to INDEX_ATTR_BITMAP_PRIMARY_KEY.
Since commit 270af6f0df7 (pg17), that bitmap includes deferrable
primary keys, so the relation is marked updatable.
FindLogicalRepLocalIndex(), however, uses GetRelationIdentityOrPK(),
which calls RelationGetPrimaryKeyIndex(rel, false) and rejects
deferrable keys. So it returns InvalidOid.

The analysis looks correct to me.

The attached patch makes mark_updatable() fall back to the primary key
only when RelationGetPrimaryKeyIndex(rel, false) returns it, which
matches the lookup path. With the same test, the subscriber will now
hit an error:
ERROR: logical replication target relation "public.t" has neither
REPLICA IDENTITY index nor PRIMARY KEY and published relation does not
have REPLICA IDENTITY FULL

I could not apply your patch on HEAD as-is, have you had some premise patches?
Anyway, I have one comment.

RelationFindDeletedTupleInfoSeq() also has a fallback code. Per my understanding,
the same tuple-detection rule should be used everywhere thus it also should be fixed,
right? Like attached.

[1]: /* * If the relation has a replica identity key or a primary key that is * unusable for locating deleted tuples (see * IsIndexUsableForFindingDeletedTuple), a full table scan becomes * necessary. In such cases, comparing the entire tuple is not required, * since the remote tuple might not include all column values. Instead, * the indexed columns alone are sufficient to identify the target tuple * (see logicalrep_rel_mark_updatable). */ indexbitmap = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_IDENTITY_KEY);
/*
* If the relation has a replica identity key or a primary key that is
* unusable for locating deleted tuples (see
* IsIndexUsableForFindingDeletedTuple), a full table scan becomes
* necessary. In such cases, comparing the entire tuple is not required,
* since the remote tuple might not include all column values. Instead,
* the indexed columns alone are sufficient to identify the target tuple
* (see logicalrep_rel_mark_updatable).
*/
indexbitmap = RelationGetIndexAttrBitmap(rel,
INDEX_ATTR_BITMAP_IDENTITY_KEY);

/* fallback to PK if no replica identity */
if (!indexbitmap)
indexbitmap = RelationGetIndexAttrBitmap(rel,
INDEX_ATTR_BITMAP_PRIMARY_KEY);

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Tue, Sep 29, 2026 at 8:46 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Nisha,

While testing another feature patch, I came across this base-code
issue. If a subscriber table's only key is a DEFERRABLE primary key,
and the published table does not use REPLICA IDENTITY FULL, then
UPDATE and DELETE apply trips an assertion.

Good catch, I confirmed the same.

The cause is that two code paths disagree.
logicalrep_rel_mark_updatable() finds no replica identity bitmap and
falls back to INDEX_ATTR_BITMAP_PRIMARY_KEY.
Since commit 270af6f0df7 (pg17), that bitmap includes deferrable
primary keys, so the relation is marked updatable.
FindLogicalRepLocalIndex(), however, uses GetRelationIdentityOrPK(),
which calls RelationGetPrimaryKeyIndex(rel, false) and rejects
deferrable keys. So it returns InvalidOid.

The analysis looks correct to me.

Thank you Kuroda-san for review.

The attached patch makes mark_updatable() fall back to the primary key
only when RelationGetPrimaryKeyIndex(rel, false) returns it, which
matches the lookup path. With the same test, the subscriber will now
hit an error:
ERROR: logical replication target relation "public.t" has neither
REPLICA IDENTITY index nor PRIMARY KEY and published relation does not
have REPLICA IDENTITY FULL

I could not apply your patch on HEAD as-is, have you had some premise patches?
Anyway, I have one comment.

The patch applies cleanly for me, and I re-tested it on the latest
HEAD (6a93535798aa) as well. Could you please now verify v2 once from
your side?

RelationFindDeletedTupleInfoSeq() also has a fallback code. Per my understanding,
the same tuple-detection rule should be used everywhere thus it also should be fixed,
right? Like attached.

Thanks for pointing this out. I verified the impact in
RelationFindDeletedTupleInfoSeq().

After my v1, RelationFindDeletedTupleInfoSeq() is not reachable for a
table whose only key is a deferrable PK when the publisher uses
DEFAULT or RI/PK, since such tables are now rejected.

But, it is still reachable when the publisher uses RI-FULL. In this
case, the sequential scan falls back to the deferrable PK columns,
which should not be used as replica identity. This can match a dead
row on the key alone and incorrectly report update_deleted instead of
update_missing.

A manual testcase to see the issue:
-- [PUB]:
CREATE TABLE t_del (a int, b text);
ALTER TABLE t_del REPLICA IDENTITY FULL;
INSERT INTO t_del VALUES (1, 'pub'), (2, 'pub');
CREATE PUBLICATION p FOR TABLE t_del;

-- [SUB]:
CREATE TABLE t_del (a int, b text);
CREATE SUBSCRIPTION s connection '...' publication p with
(copy_data=false, retain_dead_tuples =true);

-- [SUB] session A:
BEGIN;
SELECT pg_current_xact_id(); -- leave open

-- [SUB] session B:
ALTER TABLE t_del ADD PRIMARY KEY (a) DEFERRABLE;
SELECT xmin FROM pg_index WHERE indexrelid = 't_del_pkey'::regclass;
-- > A's xid
INSERT INTO t_del VALUES (1, 'local');
DELETE FROM t_del WHERE a = 1; -- deleted tuple

-- [PUB]:
UPDATE t_del SET b = 'upd' WHERE a = 1; -- old row sent: (1,'pub')

-- [SUB] log:
LOG: conflict detected on relation "public.t_del": conflict=update_deleted
-- Wrong: (1,'pub') never existed on the subscriber; the dead
(1,'local') matched on the key alone.

With suggested fix, update_missing is detected correctly for this case.

[1]:
/*
* If the relation has a replica identity key or a primary key that is
* unusable for locating deleted tuples (see
* IsIndexUsableForFindingDeletedTuple), a full table scan becomes
* necessary. In such cases, comparing the entire tuple is not required,
* since the remote tuple might not include all column values. Instead,
* the indexed columns alone are sufficient to identify the target tuple
* (see logicalrep_rel_mark_updatable).
*/
indexbitmap = RelationGetIndexAttrBitmap(rel,
INDEX_ATTR_BITMAP_IDENTITY_KEY);

/* fallback to PK if no replica identity */
if (!indexbitmap)
indexbitmap = RelationGetIndexAttrBitmap(rel,
INDEX_ATTR_BITMAP_PRIMARY_KEY);

Thanks for the patch, I've combined your suggested fix and attched
updated patch v2.

--
Thanks,
Nisha

Hi,

Thanks for working!

The patch applies cleanly for me, and I re-tested it on the latest
HEAD (6a93535798aa) as well. Could you please now verify v2 once from
your side?

My fault: I did not pull ad36e360.

Thanks for pointing this out. I verified the impact in
RelationFindDeletedTupleInfoSeq().

After my v1, RelationFindDeletedTupleInfoSeq() is not reachable for a
table whose only key is a deferrable PK when the publisher uses
DEFAULT or RI/PK, since such tables are now rejected.

But, it is still reachable when the publisher uses RI-FULL. In this
case, the sequential scan falls back to the deferrable PK columns,
which should not be used as replica identity. This can match a dead
row on the key alone and incorrectly report update_deleted instead of
update_missing.

Yes, it was my intention.

Thanks for the patch, I've combined your suggested fix and attched
updated patch v2.

I checked and no comments for the implementation.
Regarding the back patch, the initial issue (FindReplTupleInLocalRel() can cause
a crash) should be done till PG17, but second one (RelationFindDeletedTupleInfoSeq()
can do a wrong decision) should be done only for PG19/master, right? If so the
patch should be separated. Also, a test can be added in 035_conflicts for the
second issue.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Tue, Sep 29, 2026 at 3:20 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Thanks for the patch, I've combined your suggested fix and attched
updated patch v2.

I checked and no comments for the implementation.
Regarding the back patch, the initial issue (FindReplTupleInLocalRel() can cause
a crash) should be done till PG17, but second one (RelationFindDeletedTupleInfoSeq()
can do a wrong decision) should be done only for PG19/master, right?

Makes sense; I overlooked the fact.

If so the patch should be separated. Also, a test can be added in 035_conflicts for the
second issue.

I’ve attached the separate patches; both apply independently on HEAD
and their respective backport branches.

I’ve also added a test case in 035_conflicts.pl, for the
RelationFindDeletedTupleInfoSeq() fix.

--
Thanks,
Nisha

On Tue, Sep 29, 2026 at 7:36 AM Nisha Moond <nisha.moond412@gmail.com> wrote:

On Tue, Sep 29, 2026 at 3:20 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Thanks for the patch, I've combined your suggested fix and attched
updated patch v2.

I checked and no comments for the implementation.
Regarding the back patch, the initial issue (FindReplTupleInLocalRel() can cause
a crash) should be done till PG17, but second one (RelationFindDeletedTupleInfoSeq()
can do a wrong decision) should be done only for PG19/master, right?

Makes sense; I overlooked the fact.

If so the patch should be separated. Also, a test can be added in 035_conflicts for the
second issue.

I’ve attached the separate patches; both apply independently on HEAD
and their respective backport branches.

I’ve also added a test case in 035_conflicts.pl, for the
RelationFindDeletedTupleInfoSeq() fix.

*
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -593,8 +593,11 @@ RelationFindDeletedTupleInfoSeq(Relation rel,
TupleTableSlot *searchslot,
  indexbitmap = RelationGetIndexAttrBitmap(rel,
  INDEX_ATTR_BITMAP_IDENTITY_KEY);

- /* fallback to PK if no replica identity */
- if (!indexbitmap)
+ /*
+ * fallback to PK if no replica identity, but only if the PK is not
+ * deferrable.
+ */
+ if (!indexbitmap && OidIsValid(RelationGetPrimaryKeyIndex(rel, false)))
  indexbitmap = RelationGetIndexAttrBitmap(rel,

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. I suggest let's first fix
this one and then we can discuss your other patch.

--
With Regards,
Amit Kapila.

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.

I suggest let's first fix this one and then we can discuss your other patch.

Yeah, it's related with PG19 issue thus it has higher priority.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

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.

I suggest let's first fix this one and then we can discuss your other patch.

Yeah, it's related with PG19 issue thus it has higher priority.

+1

--
Thanks,
Nisha

Dear Nisha,

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.

Thanks. One comments for 0001:

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

I feel the first condition is something like transitive. Here we want to confirm
whether 1) we can pick indexed columns or 2) all columns are replicated. So it's
better to directly check the existence of the index:

```
Assert(OidIsValid(idxoid) ||
relmapentry->remoterel.replident == REPLICA_IDENTITY_FULL);
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Wed, 30 Sept 2026 at 16:05, 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.

Thanks for the patch, one suggestion:
The two relcache lookups that previously obtained the replica identity
columns and, as a fallback, the primary key columns:
indexbitmap = RelationGetIndexAttrBitmap(rel,
INDEX_ATTR_BITMAP_IDENTITY_KEY);

if (!indexbitmap)
indexbitmap = RelationGetIndexAttrBitmap(rel,
INDEX_ATTR_BITMAP_PRIMARY_KEY);

are now replaced by deriving the columns from idxoid:
if (OidIsValid(idxoid))
{
Relation idxrel = index_open(idxoid, AccessShareLock);
...
}

The idxoid is obtained from localindexoid/idxisreplident in the
relation map entry. These values are populated by
FindLogicalRepLocalIndex(), which uses GetRelationIdentityOrPK():
Oid
GetRelationIdentityOrPK(Relation rel)
{
Oid idxoid = RelationGetReplicaIndex(rel);

if (!OidIsValid(idxoid))
idxoid = RelationGetPrimaryKeyIndex(rel, false);

return idxoid;
}

Thus, the same identity-index-then-primary-key fallback logic is
already handled when the relation map entry is built. A valid idxoid
represents either the replica identity index or the primary key index,
so the removed relcache lookups do not result in any loss of
functionality.

If you are ok, can we include something like this in the commit
message, as it makes it clear why the old lookups can be removed and
helps explain the change for anyone reviewing the commit later.

The changes look good overall.

Regards,
Vignesh

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.

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

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]/messages/by-id/CALDaNm3uXt7oiPRzUd-H+GFfiVx3H3BJExV2fRVNj_LpmDqCtg@mail.gmail.com.

Attached updated patches v5. There are a couple of optimizations and
comment improvements in 002(testcode) too.

[1]: /messages/by-id/CALDaNm3uXt7oiPRzUd-H+GFfiVx3H3BJExV2fRVNj_LpmDqCtg@mail.gmail.com

--
Thanks,
Nisha

On Thu, 1 Oct 2026 at 12:10, Nisha Moond <nisha.moond412@gmail.com> wrote:

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.

Thanks for the updated version, the patch looks good to me.

Regards,
Vignesh

Hi,

On Thu, Oct 1, 2026 at 2:39 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

Attached updated patches v5. There are a couple of optimizations and
comment improvements in 002(testcode) too.

Thanks for the patch. It looks good to me overall, I just have coding
suggestions:

1.

Relation idxrel = index_open(idxoid, AccessShareLock);

We shall use NoLock here, since the index must have been locked already.

2.

+ for (int i = 0; i < idxrel->rd_index->indnkeyatts; i++)
+ {
+ AttrNumber attnum = idxrel->rd_index->indkey.values[i];
+
+ Assert(AttributeNumberIsValid(attnum));
+ indexbitmap = bms_add_member(indexbitmap,
+ attnum - FirstLowInvalidHeapAttributeNumber);
+ }

This Assert also looks unnecessary, removing it would make the code more
concise:

for (int i = 0; i < idxrel->rd_index->indnkeyatts; i++)
indexbitmap = bms_add_member(indexbitmap,
idxrel->rd_index->indkey.values[i] -
FirstLowInvalidHeapAttributeNumber);

The rest looks good to me.

Best Regards,
Zhijie Hou

On Thu, Oct 1, 2026 at 4:16 PM Zhijie Hou <houzhijie22@gmail.com> wrote:

Hi,

On Thu, Oct 1, 2026 at 2:39 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

Attached updated patches v5. There are a couple of optimizations and
comment improvements in 002(testcode) too.

Thanks for the patch. It looks good to me overall, I just have coding
suggestions:

1.

Relation idxrel = index_open(idxoid, AccessShareLock);

We shall use NoLock here, since the index must have been locked already.

Makes sense, the index is always already locked when we get here. I
also added a comment.

2.

+ for (int i = 0; i < idxrel->rd_index->indnkeyatts; i++)
+ {
+ AttrNumber attnum = idxrel->rd_index->indkey.values[i];
+
+ Assert(AttributeNumberIsValid(attnum));
+ indexbitmap = bms_add_member(indexbitmap,
+ attnum - FirstLowInvalidHeapAttributeNumber);
+ }

This Assert also looks unnecessary, removing it would make the code more
concise:

for (int i = 0; i < idxrel->rd_index->indnkeyatts; i++)
indexbitmap = bms_add_member(indexbitmap,
idxrel->rd_index->indkey.values[i] -
FirstLowInvalidHeapAttributeNumber);

Done.

--
Thanks,
Nisha

On Thu, Oct 1, 2026 at 8:12 AM Nisha Moond <nisha.moond412@gmail.com> wrote:

Few comments:
============
1.
- * If the relation has a replica identity key or a primary key that is
- * unusable for locating deleted tuples (see
- * IsIndexUsableForFindingDeletedTuple), a full table scan becomes
- * necessary. In such cases, comparing the entire tuple is not required,
- * since the remote tuple might not include all column values. Instead,
- * the indexed columns alone are sufficient to identify the target tuple
- * (see logicalrep_rel_mark_updatable).
+ * We get here when the caller's index, if any, cannot be used for
+ * locating deleted tuples (see IsIndexUsableForFindingDeletedTuple). If
+ * that index is the replica identity or primary key, the remote tuple
+ * might not include all column values, but the index's key columns alone
+ * are sufficient to identify the target tuple. Otherwise, the remote
+ * relation has REPLICA IDENTITY FULL, so compare the entire tuple.

Why is this comment changed? I find the previous comment better.

2.
+ * If 'identidxoid' is valid, it must be the replica identity or primary key
+ * index, and only its key columns are compared. Otherwise, all columns are
+ * compared.

Saying must here may not be good as we can't have an assert for it?

3.
+ * Pass the index only if it is the replica identity or primary key,
+ * so that its key columns are compared. Use the relation map's choice
+ * rather than looking it up again, since concurrent DDL may have
+ * changed the relation's replica identity.
+ */
+ return RelationFindDeletedTupleInfoSeq(localrel,

Is this comment really required? IT can be inferred easily from the code.

4. I think keeping a deferrable key test case only is sufficient.

--
With Regards,
Amit Kapila.

Hi,

On Fri, Oct 2, 2026 at 5:29 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Oct 1, 2026 at 8:12 AM Nisha Moond <nisha.moond412@gmail.com> wrote:

Few comments:
============
1.
- * If the relation has a replica identity key or a primary key that is
- * unusable for locating deleted tuples (see
- * IsIndexUsableForFindingDeletedTuple), a full table scan becomes
- * necessary. In such cases, comparing the entire tuple is not required,
- * since the remote tuple might not include all column values. Instead,
- * the indexed columns alone are sufficient to identify the target tuple
- * (see logicalrep_rel_mark_updatable).
+ * We get here when the caller's index, if any, cannot be used for
+ * locating deleted tuples (see IsIndexUsableForFindingDeletedTuple). If
+ * that index is the replica identity or primary key, the remote tuple
+ * might not include all column values, but the index's key columns alone
+ * are sufficient to identify the target tuple. Otherwise, the remote
+ * relation has REPLICA IDENTITY FULL, so compare the entire tuple.

Why is this comment changed? I find the previous comment better.

Agreed. I reverted it to the original version and only adjusted it slightly to
mention the passed-in index.

2.
+ * If 'identidxoid' is valid, it must be the replica identity or primary key
+ * index, and only its key columns are compared. Otherwise, all columns are
+ * compared.

Saying must here may not be good as we can't have an assert for it?

Removed this word.

3.
+ * Pass the index only if it is the replica identity or primary key,
+ * so that its key columns are compared. Use the relation map's choice
+ * rather than looking it up again, since concurrent DDL may have
+ * changed the relation's replica identity.
+ */
+ return RelationFindDeletedTupleInfoSeq(localrel,

Is this comment really required? IT can be inferred easily from the code.

I also think this is not required, removed.

4. I think keeping a deferrable key test case only is sufficient.

I merged a deferrable key test from 0002 into 0001 in this version.

Here is the V7 patch that addressed above comments. I confirmed it applies
cleanly on all required branches and the test passed.

Best Regards,
Zhijie Hou

On Thu, Oct 1, 2026 at 11:33 PM Zhijie Hou <houzhijie22@gmail.com> wrote:

I merged a deferrable key test from 0002 into 0001 in this version.

Here is the V7 patch that addressed above comments. I confirmed it applies
cleanly on all required branches and the test passed.

Thanks, I've pushed the patch.

--
With Regards,
Amit Kapila.

On Sat, Oct 3, 2026 at 2:15 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Oct 1, 2026 at 11:33 PM Zhijie Hou <houzhijie22@gmail.com> wrote:

I merged a deferrable key test from 0002 into 0001 in this version.

Here is the V7 patch that addressed above comments. I confirmed it applies
cleanly on all required branches and the test passed.

Thanks, I've pushed the patch.

Thanks for pushing the patch.
Here is the rebased v4 patch for the remaining issue in this thread.

--
Thanks,
Nisha

Hi Nisha,

Thanks for pushing the patch.
Here is the rebased v4 patch for the remaining issue in this thread.

Thanks for updating the patch!

While reading the patch again, I came up with another idea to use already-found
local index. I.e., move logicalrep_rel_mark_updatable() after the
FindLogicalRepLocalIndex() then refer entry->idxisreplident and entry->localindexoid.
It may change codes bit more but we can use the single source of truth.

How do you feel?

Best regards,
Hayato Kuroda
FUJITSU LIMITED