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

From Hayato Kuroda (Fujitsu)
Subject RE: Fix apply worker crash when subscriber table has only a deferrable primary key
Date
Msg-id OS7PR01MB1831779ED93F5470CA1D57605F58C2@OS7PR01MB18317.jpnprd01.prod.outlook.com
Whole thread
In response to Fix apply worker crash when subscriber table has only a deferrable primary key  (Nisha Moond <nisha.moond412@gmail.com>)
Responses Re: Fix apply worker crash when subscriber table has only a deferrable primary key
List pgsql-hackers
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);

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

Best regards,
Hayato Kuroda
FUJITSU LIMITED


Attachment

pgsql-hackers by date:

Previous
From: David Rowley
Date:
Subject: Re: Set calcSumX2 = true in numeric_(poly_)deserialize
Next
From: Bharath Rupireddy
Date:
Subject: Re: parallel autovacuum: Propagate track_cost_delay_timing to parallel workers