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

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