Re: Conflict detection and logging in logical replication

[Amit Kapila]

On Wed, Aug 14, 2024 at 8:05 AM Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
> Here is the V14 patch.
>

Review comments:
1.
ReportApplyConflict()
{
...
+ ereport(elevel,
+ errcode(ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION),
+ errmsg("conflict detected on relation \"%s.%s\": conflict=%s",
+    get_namespace_name(RelationGetNamespace(localrel)),
...

Is it a good idea to use ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION for
all conflicts? I think it is okay to use for insert_exists and
update_exists. The other error codes to consider for conflicts other
than insert_exists and update_exists are
ERRCODE_T_R_SERIALIZATION_FAILURE, ERRCODE_CARDINALITY_VIOLATION,
ERRCODE_NO_DATA, ERRCODE_NO_DATA_FOUND,
ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION,
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE.

BTW, even for insert/update_exists, won't it better to use
ERRCODE_UNIQUE_VIOLATION?

2.
+build_tuple_value_details()
{
...
+ if (searchslot)
+ {
+ /*
+ * If a valid index OID is provided, build the replica identity key
+ * value string. Otherwise, construct the full tuple value for REPLICA
+ * IDENTITY FULL cases.
+ */

AFAICU, this can't happen for insert/update_exists. If so, we should
add an assert for those two conflict types.

3.
+build_tuple_value_details()
{
...
+    /*
+     * Although logical replication doesn't maintain the bitmap for the
+     * columns being inserted, we still use it to create 'modifiedCols'
+     * for consistency with other calls to ExecBuildSlotValueDescription.
+     */
+    modifiedCols = bms_union(ExecGetInsertedCols(relinfo, estate),
+                 ExecGetUpdatedCols(relinfo, estate));
+    desc = ExecBuildSlotValueDescription(relid, remoteslot, tupdesc,
+                       modifiedCols, 64);

Can we mention in the comments the reason for not including generated columns?

Apart from the above, the attached contains some cosmetic changes.

-- 
With Regards,
Amit Kapila.

diff --git a/src/backend/replication/logical/conflict.c 
b/src/backend/replication/logical/conflict.c
index 0f51564aa2..c56814cf50 100644
--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -1,14 +1,14 @@
 /*-------------------------------------------------------------------------
  * conflict.c
- *        Functionality for detecting and logging conflicts.
+ *        Support routines for logging conflicts.
  *
  * Copyright (c) 2024, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
  *       src/backend/replication/logical/conflict.c
  *
- * This file contains the code for detecting and logging conflicts on
- * the subscriber during logical replication.
+ * This file contains the code for logging conflicts on the subscriber during
+ * logical replication.
  *-------------------------------------------------------------------------
  */
 
@@ -105,7 +105,7 @@ GetTupleTransactionInfo(TupleTableSlot *localslot, 
TransactionId *xmin,
  * OIDs should not be passed here.
  *
  * The caller must ensure that the index with the OID 'indexoid' is locked so
- * that we can display the conflicting key value.
+ * that we can fetch and display the conflicting key value.
  */
 void
 ReportApplyConflict(EState *estate, ResultRelInfo *relinfo, int elevel,
@@ -403,7 +403,8 @@ build_tuple_value_details(EState *estate, ResultRelInfo 
*relinfo,
  * Helper functions to construct a string describing the contents of an index
  * entry. See BuildIndexValueDescription for details.
  *
- * The caller should ensure that the index with the OID 'indexoid' is locked.
+ * The caller must ensure that the index with the OID 'indexoid' is locked so
+ * that we can fetch and display the conflicting key value.
  */
 static char *
 build_index_value_desc(EState *estate, Relation localrel, TupleTableSlot *slot,
diff --git a/src/include/replication/conflict.h 
b/src/include/replication/conflict.h
index 971dfa98dc..02cb84da7e 100644
--- a/src/include/replication/conflict.h
+++ b/src/include/replication/conflict.h
@@ -1,6 +1,6 @@
 /*-------------------------------------------------------------------------
  * conflict.h
- *        Exports for conflict detection and log
+ *        Exports for conflicts logging.
  *
  * Copyright (c) 2024, PostgreSQL Global Development Group
  *
@@ -13,7 +13,7 @@
 #include "utils/timestamp.h"
 
 /*
- * Conflict types that could be encountered when applying remote changes.
+ * Conflict types that could occur while applying remote changes.
  */
 typedef enum
 {
@@ -36,9 +36,9 @@ typedef enum
        CT_DELETE_MISSING,
 
        /*
-        * Other conflicts, such as exclusion constraint violations, involve 
rules
-        * that are more complex than simple equality checks. These conflicts 
are
-        * left for future improvements.
+        * Other conflicts, such as exclusion constraint violations, involve 
more
+        * complex rules than simple equality checks. These conflicts are left 
for
+        * future improvements.
         */
 } ConflictType;