On Mon, Aug 10, 2026 at 6:45 PM Dilip Kumar <[email protected]> wrote:
>
> I will work on other follow-up patches soon.
>
Few comments:
=============
1.
+InsertConflictLogTuple(Relation conflictlogrel, HeapTuple tuple,
+ const char *errcontext_str)
{
...
...
+ /*
+ * Set up an error context so that a failure to insert (e.g. an
+ * out-of-space error) carries information identifying the conflict we
+ * were trying to log.
+ */
+ errcallback.callback = conflict_log_insert_errcontext;
+ errcallback.arg = (void *) errcontext_str;
+ errcallback.previous = error_context_stack;
+ error_context_stack = &errcallback;
Do we need this err context setup? I am seeing following LOGs after
setting conflict_log_destination='table':
[logical replication apply worker] LOG: conflict detected on relation
"public.t1": conflict=delete_origin_differs
[logical replication apply worker] DETAIL: Conflict details are
logged to the conflict log table: pg_conflict_log_16392
[logical replication apply worker] CONTEXT: processing remote data
for replication origin "pg_16392" during message type "DELETE" for
replication target relation "public.t1" in transaction 697, finished
at 0/017E3C60
[logical replication apply worker] ERROR: testing insert failure
[logical replication apply worker] CONTEXT: while logging conflict
"delete_origin_differs" detected on relation "t1"
We are reporting conflict "delete_origin_differs" twice. IIRC, this
err_context was required in an earlier approach where we use to delete
the ERROR reporting after insertion into table so that if there is
failure while inserting into table, we won't miss the conflict
information.
2.
+ * The 'local_conflicts' column is typed as an array of JSON objects (json[])
+ * rather than a single json object to keep the exposed schema future-proof.
+ * Although currently only resolved LOG-level conflicts (which involve a single
+ * local row) are recorded in this table, future capabilities (such as conflict
+ * resolution handlers or logging multi-row constraint conflicts) may need to
+ * record multiple conflicting local rows for a single remote operation.
+ * Defining it as an array from the start keeps the table schema stable and
+ * avoids backward-incompatible schema changes or complex upgrade
handling later.
Can we slightly shorten this comment as in attached?
3.
+static Datum build_local_conflicts_json_array(List *conflicttuples);
+static HeapTuple prepare_conflict_log_tuple(EState *estate, Relation rel,
+ Relation conflictlogrel,
+ ConflictType conflict_type,
+ TupleTableSlot *searchslot,
+ List *conflicttuples,
+ TupleTableSlot *remoteslot,
+ char **errcontext_str);
+static void InsertConflictLogTuple(Relation conflictlogrel, HeapTuple tuple,
+ const char *errcontext_str);
Why the function naming for InsertConflictLogTuple in a different case
as compared to other static functions?
4. "Handling Multi-row Conflicts: A single remote tuple may conflict
with multiple local tuples (e.g., in the case of
multiple_unique_conflicts). To handle this, the infrastructure creates
a single row in the conflict log table for each remote tuple. The
details of all conflicting local rows are aggregated into a single
JSON array in the local_conflicts column.
Seeing above in the commit message, it seems you forgot to update the
commit message.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/logical/conflict.c
b/src/backend/replication/logical/conflict.c
index cbfa317e09a..e2900577741 100644
--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -69,14 +69,10 @@ typedef struct ConflictLogColumnDef
* scalar columns (relid, conflict_type, commit timestamp) while these json
* columns are per-conflict payload to inspect, not search keys.
*
- * The 'local_conflicts' column is typed as an array of JSON objects (json[])
- * rather than a single json object to keep the exposed schema future-proof.
- * Although currently only resolved LOG-level conflicts (which involve a single
- * local row) are recorded in this table, future capabilities (such as conflict
- * resolution handlers or logging multi-row constraint conflicts) may need to
- * record multiple conflicting local rows for a single remote operation.
- * Defining it as an array from the start keeps the table schema stable and
- * avoids backward-incompatible schema changes or complex upgrade handling
later.
+ * 'local_conflicts' is typed as an array of JSON objects (json[]), not a
+ * single json object, so that a future conflict type needing to record
+ * multiple local rows for one remote operation doesn't require a
+ * backward-incompatible schema change.
*/
static const ConflictLogColumnDef ConflictLogSchema[] = {
{.attname = "relid", .atttypid = OIDOID},
