Re: Conflict detection for multiple_unique_conflicts in logical replication

On Wed, Mar 19, 2025 at 4:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Mar 19, 2025 at 11:11 AM Nisha Moond <nisha.moond412@gmail.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?
>

Fixed.

> 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.
>

Implemented the suggestions.

> 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?
>

+1, renamed the test file as t/035_conflicts.pl.

Attached is v6 patch with above comments addressed.

--
Thanks,
Nisha