Re: Conflict detection and logging in logical replication

On Tue, Aug 13, 2024 at 10:09 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> 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.