Re: Conflict detection for multiple_unique_conflicts in logical replication

On Thu, Mar 20, 2025 at 5:23 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Thu, Mar 20, 2025 at 3:06 PM Nisha Moond wrote:
> >
> > Attached is v6 patch with above comments addressed.
>
> Thanks updating the patch. I have some comments:
>
> 1.
>
> The naming style of variables changed in this function seems a bit Inconsistent
> with existing ones, I feel we'd better use similar style, e.g., conflictSlots => conflictslots
>
> I included the suggested changes in 0001.
>
>  ReportApplyConflict(EState *estate, ResultRelInfo *relinfo, int elevel,
>                                         ConflictType type, TupleTableSlot *searchslot,
> -                                       TupleTableSlot *localslot, TupleTableSlot *remoteslot,
> -                                       Oid indexoid, TransactionId localxmin,
> -                                       RepOriginId localorigin, TimestampTz localts)
> +                                       List *conflictSlots, TupleTableSlot *remoteslot,
> +                                       List *conflictIndexes, List *localxmins,
> +                                       List *localorigins, List *localts)
>
> 2.
>
> I modified the documents a bit for consistency. Please see 0001
> attachment.
>
> 3.
>
> I have been thinking whether the codes in ReportApplyConflict() can be improved
> further, e.g., avoid the extra checks in do while().
>
> One idea could be that each caller of
> ReportApplyConflict() can always pass a valid list for all
> list-parameter(e.g., conflictIndexes, localxmins ...). And for the cases when the
> caller could not provide a valid item, it would save an invalid value
> in the list and pass it to the function.
>
> In this approach, ReportApplyConflict() seems cleaner. I am sharing a POC diff
> (0002) for reference, it can pass regression test, but I have not confirmed
> every case yet.
>
> 4.
>
> +                       origin = list_nth_int(localorigins, conflictNum);
> ...
> +                       localts = lappend(localts, DatumGetPointer(Int64GetDatum(committs)));
>
> I personally feel this could be improved, because
> 1) RepOriginId, being a 16-bit value, is smaller than an int, which might not
>    cause issues but appears somewhat odd when storing a 32-bit value within it;
> 2) The approach used to store 'committs' seems inelegant (and I didn't find
>    precedents).
>
> An alternative approach is to introduce a new structure, ConflictTupleInfo,
> containing items like slot, origin,  view.committs, and xmin for list integration.
> This way, the code could be simpler, and we can avoid the above coding. Please
> see 0003 for reference. (Note that some comments in this patch could be
> improved)
>

Thanks, Hou-san, for the review and fix patches. I’ve incorporated
your suggestions.
Attached are the v7 patches, including patch 002, which implements
stats collection for 'multiple_unique_conflicts' in
pg_stat_subscription_stats.

--
Thanks,
Nisha