On Wed, Mar 19, 2025 at 4:18 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Wed, Mar 19, 2025 at 11:11 AM Nisha Moond <nisha.moond...@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
v6-0001-Implement-the-conflict-detection-for-multiple_uni.patch
Description: Binary data
