Re: Fix apply worker crash when subscriber table has only a deferrable primary key - Mailing list pgsql-hackers

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