Re: Conflict detection for multiple_unique_conflicts in logical replication

On Wed, Mar 19, 2025 at 11:11 AM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> Please find the attached v5-0001 patch without the stats part.
>

Review:
=======
1.
+ foreach_ptr(TupleTableSlot, slot, conflictSlots)
+ {
+ indexoid = lfirst_oid(list_nth_cell(conflictIndexes, conflictNum));
+
+ Assert(!OidIsValid(indexoid) ||
+ CheckRelationOidLockedByMe(indexoid, RowExclusiveLock, true));
+
+ if (!localorigin)
+ GetTupleTransactionInfo(slot, &localxmin, &localorigin, &localts);
+
+ /*
+ * Build the error detail message containing the conflicting key and
+ * tuple information. The details for each conflict will be appended
+ * to err_detail.
+ */
+ errdetail_apply_conflict(estate, relinfo, type, searchslot,
+ slot, remoteslot, indexoid,
+ localxmin, localorigin, localts, &err_detail);
+
+ conflictNum++;
+ }

Won't this get TupleTransactionInfo only for the first conflicting
tuple instead of getting it separately for each tuple?

2. We make an array/list of local tuple's origin info from the caller
and send it to this ReportApplyConflict. Additionally, we can use
do...while loop to avoid calling errdetail_apply_conflict multiple
times in the function ReportApplyConflict(). We break the loop if
there are no conflictSlots after generating the first detail message.

3. Related to the new test file 't/035_multiple_unique_conflicts.pl',
is it worth to add a separate test file only for
multiple_unique_conflicts? I see that for all other conflicts, we have
tests spread over existing tests where such conflict-related tests
were present. Shall we name it as t/035_conflicts.pl? That will allow
us to add more tests related to other conflicts like update_delete in
the same file?

--
With Regards,
Amit Kapila.