RE: Conflict detection for multiple_unique_conflicts in logical replication

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, 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)

Best Regards,
Hou zj