Re: Conflict detection and logging in logical replication - Mailing list pgsql-hackers

On Sun, Aug 4, 2024 at 1:22 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, August 2, 2024 7:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
>
> Here is the V11 patch set which addressed above and Kuroda-san[1] comments.
>

A few design-level points:

*
@@ -525,10 +602,33 @@ ExecSimpleRelationInsert(ResultRelInfo *resultRelInfo,
  /* OK, store the tuple and create index entries for it */
  simple_table_tuple_insert(resultRelInfo->ri_RelationDesc, slot);

+ conflictindexes = resultRelInfo->ri_onConflictArbiterIndexes;
+
  if (resultRelInfo->ri_NumIndices > 0)
  recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
-    slot, estate, false, false,
-    NULL, NIL, false);
+    slot, estate, false,
+    conflictindexes ? true : false,
+    &conflict,
+    conflictindexes, false);
+
+ /*
+ * Checks the conflict indexes to fetch the conflicting local tuple
+ * and reports the conflict. We perform this check here, instead of
+ * performing an additional index scan before the actual insertion and
+ * reporting the conflict if any conflicting tuples are found. This is
+ * to avoid the overhead of executing the extra scan for each INSERT
+ * operation, even when no conflict arises, which could introduce
+ * significant overhead to replication, particularly in cases where
+ * conflicts are rare.
+ *
+ * XXX OTOH, this could lead to clean-up effort for dead tuples added
+ * in heap and index in case of conflicts. But as conflicts shouldn't
+ * be a frequent thing so we preferred to save the performance overhead
+ * of extra scan before each insertion.
+ */
+ if (conflict)
+ CheckAndReportConflict(resultRelInfo, estate, CT_INSERT_EXISTS,
+    recheckIndexes, slot);

I was thinking about this case where we have some pros and cons of
doing additional scans only after we found the conflict. I was
wondering how we will handle the resolution strategy for this when we
have to remote_apply the tuple for insert_exists/update_exists cases.
We would have already inserted the remote tuple in the heap and index
before we found the conflict which means we have to roll back that
change and then start a forest transaction to perform remote_apply
which probably has to update the existing tuple. We may have to
perform something like speculative insertion and then abort it. That
doesn't sound cheap either. Do you have any better ideas?

*
-ERROR:  duplicate key value violates unique constraint "test_pkey"
-DETAIL:  Key (c)=(1) already exists.
+ERROR:  conflict insert_exists detected on relation "public.test"
+DETAIL:  Key (c)=(1) already exists in unique index "t_pkey", which
was modified locally in transaction 740 at 2024-06-26
10:47:04.727375+08.

I think the format to display conflicts is not very clear. The
conflict should be apparent just by seeing the LOG/ERROR message. I am
thinking of something like below:

LOG: CONFLICT: <insert_exisits or whatever names we document>;
DESCRIPTION: If any .. ; RESOLUTION: (This one can be added later)
DEATAIL: remote_tuple (tuple values); local_tuple (tuple values);

With the above, one can easily identify the conflict's reason and
action taken by apply worker.

--
With Regards,
Amit Kapila.