Re: Proposal: Conflict log history table for Logical Replication

On Tue, Aug 11, 2026 at 3:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Aug 10, 2026 at 6:45 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> 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.

Yeah we don't need this now

> 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?

Done

> 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?

It was previously an extern function, since it is now static, we
changed the case. Fixed.

> 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.

Fixed

--
Regards,
Dilip Kumar
Google