On Tue, Aug 13, 2024 at 10:09 AM Zhijie Hou (Fujitsu)
<[email protected]> wrote:
>
> Here is V13 patch set which addressed above comments.
>
1.
+ReportApplyConflict(int elevel, ConflictType type, EState *estate,
+ ResultRelInfo *relinfo,
The change looks better but it would still be better to keep elevel
and type after relinfo. The same applies to other places as well.
2.
+ * The caller should ensure that the index with the OID 'indexoid' is locked.
+ *
+ * Refer to errdetail_apply_conflict for the content that will be included in
+ * the DETAIL line.
+ */
+void
+ReportApplyConflict(int elevel, ConflictType type, EState *estate,
Is it possible to add an assert to ensure that the index is locked by
the caller?
3.
+static char *
+build_tuple_value_details(EState *estate, ResultRelInfo *relinfo,
+ TupleTableSlot *searchslot,
+ TupleTableSlot *localslot,
+ TupleTableSlot *remoteslot,
+ Oid indexoid)
{
...
...
+ /*
+ * If 'searchslot' is NULL and 'indexoid' is valid, it indicates that we
+ * are reporting the unique constraint violation conflict, in which case
+ * the conflicting key values will be reported.
+ */
+ if (OidIsValid(indexoid) && !searchslot)
+ {
...
...
}
This indirect way of inferencing constraint violation looks fragile.
The caller should pass the required information explicitly and then
you can have the required assertions here.
Apart from the above, I have made quite a few changes in the code
comments and LOG messages in the attached.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/logical/conflict.c
b/src/backend/replication/logical/conflict.c
index 8f7e5bfdd4..4995651644 100644
--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -79,10 +79,11 @@ GetTupleTransactionInfo(TupleTableSlot *localslot,
TransactionId *xmin,
}
/*
- * Report a conflict when applying remote changes.
+ * This function is used to report a conflict while applying replication
+ * changes.
*
- * 'searchslot' should contain the tuple used to search for the local tuple to
- * be updated or deleted.
+ * 'searchslot' should contain the tuple used to search the local tuple to be
+ * updated or deleted.
*
* 'localslot' should contain the existing local tuple, if any, that conflicts
* with the remote tuple. 'localxmin', 'localorigin', and 'localts' provide the
@@ -91,17 +92,17 @@ GetTupleTransactionInfo(TupleTableSlot *localslot,
TransactionId *xmin,
* 'remoteslot' should contain the remote new tuple, if any.
*
* The 'indexoid' represents the OID of the replica identity index or the OID
- * of the unique index that triggered the constraint violation error. Note that
- * while other indexes may also be used (see
+ * of the unique index that triggered the constraint violation error. We use
+ * this to report the key values for conflicting tuple.
+ *
+ * Note that while other indexes may also be used (see
* IsIndexUsableForReplicaIdentityFull for details) to find the tuple when
* applying update or delete, such an index scan may not result in a unique
* tuple and we still compare the complete tuple in such cases, thus such index
* OIDs should not be passed here.
*
- * The caller should ensure that the index with the OID 'indexoid' is locked.
- *
- * Refer to errdetail_apply_conflict for the content that will be included in
- * the DETAIL line.
+ * The caller must ensure that the index with the OID 'indexoid' is locked so
+ * that we can display the conflicting key value.
*/
void
ReportApplyConflict(int elevel, ConflictType type, EState *estate,
@@ -157,10 +158,10 @@ InitConflictIndexes(ResultRelInfo *relInfo)
/*
* Add an errdetail() line showing conflict detail.
*
- * The DETAIL line comprises two parts:
+ * The DETAIL line comprises of two parts:
* 1. Explanation of the conflict type, including the origin and commit
* timestamp of the existing local tuple.
- * 2. Display of conflicting key, existing local tuple, remote new tuple and
+ * 2. Display of conflicting key, existing local tuple, remote new tuple, and
* replica identity columns, if any. The remote old tuple is excluded as its
* information is covered in the replica identity columns.
*/
@@ -196,11 +197,11 @@ errdetail_apply_conflict(ConflictType type, EState
*estate,
localxmin, timestamptz_to_str(localts));
/*
- * The origin which modified the row has been
dropped. This
- * situation may occur if the origin was
created by a
- * different apply worker, but its associated
subscription and
- * origin were dropped after updating the row,
or if the
- * origin was manually dropped by the user.
+ * The origin that modified this row has been
removed. This
+ * can happen if the origin was created by a
different apply
+ * worker and its associated subscription and
origin were
+ * dropped after updating the row, or if the
origin was
+ * manually dropped by the user.
*/
else
appendStringInfo(&err_detail, _("Key
already exists in unique index \"%s\", modified by a non-existent origin in
transaction %u at %s."),
@@ -221,7 +222,7 @@ errdetail_apply_conflict(ConflictType type, EState *estate,
appendStringInfo(&err_detail, _("Updating the
row that was modified by a different origin \"%s\" in transaction %u at %s."),
origin_name,
localxmin, timestamptz_to_str(localts));
- /* The origin which modified the row has been dropped */
+ /* The origin that modified this row has been removed.
*/
else
appendStringInfo(&err_detail, _("Updating the
row that was modified by a non-existent origin in transaction %u at %s."),
localxmin,
timestamptz_to_str(localts));
@@ -229,7 +230,7 @@ errdetail_apply_conflict(ConflictType type, EState *estate,
break;
case CT_UPDATE_MISSING:
- appendStringInfo(&err_detail, _("Did not find the row
to be updated."));
+ appendStringInfo(&err_detail, _("Could not find the row
to be updated."));
break;
case CT_DELETE_DIFFER:
@@ -240,7 +241,7 @@ errdetail_apply_conflict(ConflictType type, EState *estate,
appendStringInfo(&err_detail, _("Deleting the
row that was modified by a different origin \"%s\" in transaction %u at %s."),
origin_name,
localxmin, timestamptz_to_str(localts));
- /* The origin which modified the row has been dropped */
+ /* The origin that modified this row has been removed.
*/
else
appendStringInfo(&err_detail, _("Deleting the
row that was modified by a non-existent origin in transaction %u at %s."),
localxmin,
timestamptz_to_str(localts));
@@ -248,7 +249,7 @@ errdetail_apply_conflict(ConflictType type, EState *estate,
break;
case CT_DELETE_MISSING:
- appendStringInfo(&err_detail, _("Did not find the row
to be deleted."));
+ appendStringInfo(&err_detail, _("Could not find the row
to be deleted."));
break;
}
@@ -268,12 +269,11 @@ errdetail_apply_conflict(ConflictType type, EState
*estate,
}
/*
- * Helper function to build the additional details after the main DETAIL line
- * that describes the values of the conflicting key, existing local tuple,
- * remote tuple and replica identity columns.
+ * Helper function to build the additional details for conflicting key,
+ * existing local tuple, remote tuple, and replica identity columns.
*
* If the return value is NULL, it indicates that the current user lacks
- * permissions to view all the columns involved.
+ * permissions to view the columns involved.
*/
static char *
build_tuple_value_details(EState *estate, ResultRelInfo *relinfo,
