On Thu, Mar 20, 2025 at 5:23 PM Zhijie Hou (Fujitsu) <houzj.f...@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
v7-0001-Implement-the-conflict-detection-for-multiple_uni.patch
Description: Binary data
v7-0002-Stats-collection-for-the-multiple_unique_conflict.patch
Description: Binary data
