On Thu, Oct 1, 2026 at 8:12 AM Nisha Moond <[email protected]> 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.
