On Fri, Mar 21, 2025 at 1:48 PM Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
> On Fri, Mar 21, 2025 at 12:50 PM Nisha Moond wrote:
>
> > 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 for updating the patches.
>
> Here are some more comments:
>
> 1.
>
> The comments atop of ReportApplyConflict() should be updated to reflect the
> updates made to the input parameters.
>
Right, I also noticed this and updated it in the attached. Apart from
this, I have made a number of cosmetic changes in the attached. Please
review and include it in the next version unless you see any problem
with it.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/executor/execReplication.c
b/src/backend/executor/execReplication.c
index a07f7f09f32..ede89ea3cf9 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -515,7 +515,7 @@ CheckAndReportConflict(ResultRelInfo *resultRelInfo, EState
*estate,
}
}
- /* Report the conflict if found */
+ /* Report the conflict, if found */
if (conflicttuples)
ReportApplyConflict(estate, resultRelInfo, ERROR,
list_length(conflicttuples) > 1 ? CT_MULTIPLE_UNIQUE_CONFLICTS : type,
diff --git a/src/backend/replication/logical/conflict.c
b/src/backend/replication/logical/conflict.c
index b1614d3aaf6..257a60b9391 100644
--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -91,18 +91,13 @@ GetTupleTransactionInfo(TupleTableSlot *localslot,
TransactionId *xmin,
* 'searchslot' should contain the tuple used to search the local tuple to be
* updated or deleted.
*
- * 'conflictslots' list contains the existing local tuples, if any, that
- * conflicts with the remote tuple. 'localxmins', 'localorigins', and 'localts'
- * provide the transaction information related to the existing local tuples.
- *
* 'remoteslot' should contain the remote new tuple, if any.
*
- * The 'conflictindexes' list represents the OIDs of the unique index that
- * triggered the constraint violation error. We use this to report the key
- * values for conflicting tuple.
+ * conflicttuples is a list of local tuples that caused the conflict and the
+ * conflict related information. See ConflictTupleInfo.
*
- * The caller must ensure that the index with the OID 'indexoid' is locked so
- * that we can fetch and display the conflicting key value.
+ * The caller must ensure that all the indexes passed in ConflictTupleInfo are
+ * locked so that we can fetch and display the conflicting key values.
*/
void
ReportApplyConflict(EState *estate, ResultRelInfo *relinfo, int elevel,
@@ -114,12 +109,8 @@ ReportApplyConflict(EState *estate, ResultRelInfo
*relinfo, int elevel,
initStringInfo(&err_detail);
- /*
- * Iterate over conflicting tuples, along with their commit timestamps,
- * origins, and the conflicting indexes to assemble an errdetail() line.
- */
+ /* Form errdetail message by combining conflicting tuples information.
*/
foreach_ptr(ConflictTupleInfo, conflicttuple, conflicttuples)
- {
errdetail_apply_conflict(estate, relinfo, type, searchslot,
conflicttuple->slot, remoteslot,
conflicttuple->indexoid,
@@ -127,7 +118,6 @@ ReportApplyConflict(EState *estate, ResultRelInfo *relinfo,
int elevel,
conflicttuple->origin,
conflicttuple->ts,
&err_detail);
- }
/* Conflict stats are not gathered for multiple_unique_conflicts */
if (type != CT_MULTIPLE_UNIQUE_CONFLICTS)
diff --git a/src/include/replication/conflict.h
b/src/include/replication/conflict.h
index 06d5d05c560..da7845744cd 100644
--- a/src/include/replication/conflict.h
+++ b/src/include/replication/conflict.h
@@ -53,7 +53,6 @@ typedef enum
#define CONFLICT_NUM_TYPES (CT_DELETE_MISSING + 1)
-
/*
* Information for the exiting local tuple that caused the conflict.
*/
@@ -63,7 +62,7 @@ typedef struct ConflictTupleInfo
Oid indexoid; /* conflicting index */
TransactionId xmin; /* transaction ID that modified
the existing
* local tuple
*/
- RepOriginId origin; /* which origin modified the
exiting local
+ RepOriginId origin; /* origin that modified the
exiting local
* tuple */
TimestampTz ts; /* when the exiting local tuple
was modified
* by the
origin */
