Re: Proposal: Conflict log history table for Logical Replication

On Thu, Nov 27, 2025 at 6:30 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Dilip. Some review comments for v7-0001.
>
> ======
> src/backend/replication/logical/conflict.c
>
> 1.
> + /* Insert conflict details to conflict log table. */
> + if (conflictlogrel)
> + {
> + /*
> + * Prepare the conflict log tuple. If the error level is below
> + * ERROR, insert it immediately. Otherwise, defer the insertion to
> + * a new transaction after the current one aborts, ensuring the log
> + * tuple is not rolled back.
> + */
> + conflictlogtuple = prepare_conflict_log_tuple(estate,
> + relinfo->ri_RelationDesc,
> + conflictlogrel,
> + conflicttuple->xmin,
> + conflicttuple->ts, type,
> + conflicttuple->origin,
> + searchslot, conflicttuple->slot,
> + remoteslot);
> + if (elevel < ERROR)
> + {
> + InsertConflictLogTuple(conflictlogrel, conflictlogtuple);
> + heap_freetuple(conflictlogtuple);
> + }
> + else
> + MyLogicalRepWorker->conflict_log_tuple = conflictlogtuple;
> +
> + table_close(conflictlogrel, AccessExclusiveLock);
> + }
> + }
> +
>
> IMO, some refactoring would help simplify conflictlogtuple processing. e.g.
>
> i) You don't need any separate 'conflictlogtuple' var
> - Use MyLogicalRepWorker->conflict_log_tuple always for this purpose
> ii) prepare_conflict_log_tuple()
> - Change this to a void; it will always side-effect
> MyLogicalRepWorker->conflict_log_tuple
> - Assert MyLogicalRepWorker->conflict_log_tuple must be NULL on entry
> iii) InsertConflictLogTuple()
> - The 2nd param it not needed if you always use
> MyLogicalRepWorker->conflict_log_tuple
> - Asserts MyLogicalRepWorker->conflict_log_tuple is not NULL, then writes it
> - BTW, I felt that heap_freetuple could also be done here too
> - Finally, sets to MyLogicalRepWorker->conflict_log_tuple to NULL
> (ready for the next conflict)
>
> ~~~
>
> InsertConflictLogTuple:
>
> 2.
> +/*
> + * InsertConflictLogTuple
> + *
> + * Persistently records the input conflict log tuple into the conflict log
> + * table. It uses HEAP_INSERT_NO_LOGICAL to explicitly block logical decoding
> + * of the tuple inserted into the conflict log table.
> + */
> +void
> +InsertConflictLogTuple(Relation conflictlogrel, HeapTuple tup)
> +{
> + int options = HEAP_INSERT_NO_LOGICAL;
> +
> + heap_insert(conflictlogrel, tup, GetCurrentCommandId(true), options, NULL);
> +}
>
> See the above review comment (iii), for some suggested changes to this function.
>
> ~~~
>
> prepare_conflict_log_tuple:
>
> 3.
> + * The caller is responsible for explicitly freeing the returned heap tuple
> + * after inserting.
> + */
> +static HeapTuple
> +prepare_conflict_log_tuple(EState *estate, Relation rel,
>
> As per the above review comment (iii), I thought the Insert function
> could handle the freeing.
>
> ~~~
>
> 4.
> + oldctx = MemoryContextSwitchTo(ApplyContext);
> + tup = heap_form_tuple(RelationGetDescr(conflictlogrel), values, nulls);
> + MemoryContextSwitchTo(oldctx);
>
> - return index_value;
> + return tup;
>
> Per the above comment (ii), change this to assign to
> MyLogicalRepWorker->conflict_log_tuple.
>
> ======
> src/backend/replication/logical/worker.c
>
> start_apply:
>
> 5.
> + /*
> + * Insert the pending conflict log tuple under a new transaction.
> + */
>
> /Insert the/Insert any/
>
> ~~~
>
> 6.
> + InsertConflictLogTuple(conflictlogrel,
> + MyLogicalRepWorker->conflict_log_tuple);
> + heap_freetuple(MyLogicalRepWorker->conflict_log_tuple);
> + MyLogicalRepWorker->conflict_log_tuple = NULL;
>
> Per earlier reqview comment (iii), remove the 2nd parm to
> InsertConflictLogTuple, and those other 2 statements can also be
> handled within InsertConflictLogTuple.
>
> ======
> src/include/replication/worker_internal.h
>
> 7.
> + /* Store conflict log tuple to be inserted before worker exit. */
> + HeapTuple conflict_log_tuple;
> +
>
> Per my above suggestions, this member comment becomes something more
> like "A conflict log tuple which is prepared but not yet written. */
>

I have fixed all these comments and also the comments of 0002, now I
feel we can actually merge 0001 and 0002, so I have merged both of
them.

Now pending work status
1) fixed review comments of 0003
2) Run pgindent -- planning to do it after we complete the first level
of review
3) Subscription TAP test for logging the actual conflicts

--
Regards,
Dilip Kumar
Google