Re: Proposal: Conflict log history table for Logical Replication

On Mon, Jul 6, 2026 at 12:52 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
>
> Attached patch fixes comments raised by Nisha, Shaveta and separates
> out the logical-replication.sgml changes to a new patch.
>

Thanks. Please find a few comments for the review done so far (0001 alone):


1)
docs of v64-001 does not compile alone unless we install 002 as well.

Perhaps thes sections are probelematic:
+              Conflict details are recorded in the server log. This is
+              the default behavior. See
+              <xref linkend="logical-replication-conflict-file-based-logging"/>
+              for details.

+              querying and analysis of conflicts. See
+              <xref
linkend="logical-replication-conflict-table-based-logging"/>
+              for details.

These try to point to 002's new sections. We can move these lines as
well to 002.

2)
+ if (pending_conflict_log.tuple != NULL)
+ {
+ heap_freetuple(pending_conflict_log.tuple);
+ pending_conflict_log.tuple = NULL;
+ }
+ if (pending_conflict_log.errcontext_str != NULL)

We can leave a blank line between 2 if-blocks to make these more
readbale, else they give a feeling of if-else block.

3)
ProcessPendingConflictLogTuple, InsertConflictLogTuple and
ReportApplyConflict: all uses cleanup similar to below:

if (pending_conflict_log.tuple != NULL)
{
        heap_freetuple(pending_conflict_log.tuple);
        pending_conflict_log.tuple = NULL;
}
if (pending_conflict_log.errcontext_str != NULL)
{
        pfree(pending_conflict_log.errcontext_str);
        pending_conflict_log.errcontext_str = NULL;
}

A small helper (say ResetPendingConflictLog()) would remove
duplication and make future maintenance easier.

4)
In build_local_conflicts_json_array(), we are first building a list of
JSON datums and then copying them into an array. This is okay, but
since num_conflicts can be computed upfront, do you think we could
directly allocate the array, avoiding the temproary list.

Instead of below steps:

+ foreach_ptr(ConflictTupleInfo, conflicttuple, conflicttuples)
+ {
....
+ json_datum = DirectFunctionCall1(row_to_json, datum);
...
+ /* Add to the array element. */
+ json_datums = lappend(json_datums, (void *) json_datum);
+ }

+ num_conflicts = list_length(json_datums);
+
+ json_datum_array = palloc_array(Datum, num_conflicts);
+
+ i = 0;
+ foreach(lc, json_datums)
+ {
+ json_datum_array[i] = (Datum) lfirst(lc);
+ i++;
+ }


can we do:

num_conflicts = list_length(conflicttuples);
json_datum_array = palloc_array(Datum, num_conflicts);

foreach_ptr(ConflictTupleInfo, conflicttuple, conflicttuples)
{
....
 json_datum_array[i++] = DirectFunctionCall1(row_to_json, datum);
}

thanks
Shveta