Re: Proposal: Conflict log history table for Logical Replication

On Tue, Jun 23, 2026 at 2:22 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, 22 Jun 2026 at 20:52, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Mon, Jun 22, 2026 at 4:32 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
> > >
> > > On Sun, Jun 21, 2026 at 7:19 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > >
> > > > On Thu, Jun 18, 2026 at 9:33 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Tue, Jun 16, 2026 at 6:54 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > > > >
> > > > > > IMHO we should just log WARNING and continue the apply worker on
> > > > > > conflict insertion failure, lets see what other thinks on this.
> > > > > >
> > > > >
> > > > > I have the same opinion. Allowing CLT to block the apply worker would
> > > > > be undesirable; CLT is a history/logs collection feature and should
> > > > > not interrupt core logical replication work.
> > > > >
> > > >
> > > > I think the insert can fail in rare cases like disk getting full while
> > > > writing WAL or some internal memory ERROR and the ERROR could be
> > > > persistent which means the LOG will be filled with the same WARNING if
> > > > there are many conflicts. Also, users may not like missing out on
> > > > conflict information. So, we can ERROR out and let users fix the
> > > > situation. Additionally, the nested try-catch to downgrade ERROR to
> > > > WARNING also looks ugly and a source of future bugs and maintenance
> > > > burden. The attached patch tries to fix this by ERRORing out on
> > > > insertion failure and attaching the required conflict info as a
> > > > context of ERROR. The patch also improved the ReportApplyConflict()
> > > > non-ERROR paths by displaying the conflict information in server LOGs
> > > > before inserting the same into CLT so that if insertion fails, the
> > > > complete conflict info can be present in server LOGs. See
> > > > v52-1-amit.Improve-error-handling-for-conflict-log-table-ins.
> > > >
> > > > Additionally, there is another problem with 0003 where when a parallel
> > > > apply worker hits an ERROR-level conflict it logs the conflict to the
> > > > conflict log table in a new transaction in its error path, after
> > > > aborting the failed apply transaction. But the leader detects worker
> > > > failure in pa_wait_for_xact_finish() by waiting on the worker's
> > > > transaction lock, and AbortOutOfAnyTransaction() releases that lock:
> > > > the leader unblocks, sees a non-finished state, raises "lost
> > > > connection to the logical replication parallel apply worker", and
> > > > tears the worker down -- which can SIGTERM it mid-insert and lose the
> > > > conflict log row, besides being a misleading message. The attached
> > > > top-up patch v52-2-amit.fix_parallel_apply_logging fixes that by
> > > > introducing PARALLEL_TRANS_ERROR state.
> > >
> > > I reproduced the above issue and verified the fix for it in
> > > v52-2-amit.fix_parallel_apply_logging. Here is a TAP test for the
> > > same.
> > > The attached top-up patch applies on top of the latest v53-0005 patch.
> > >
> > I have merged Amit's patch and Nisha's tap test patch and tested all
> > the cases discussed, and those are working fine.
>
> Few comments:
> 1) Currently we are storing these in shared memory. Looking at the
> implementation, these fields are purely worker-private state used to
> ferry data across the error boundary from prepare_conflict_log_tuple()
> (inside the PG_TRY block) to ProcessPendingConflictLogTuple() (inside
> the PG_CATCH block).If it is not required by another process, should
> it be moved out of shared memory.
> + /* A conflict log tuple that is prepared but not yet inserted. */
> + HeapTuple conflict_log_tuple;
> +
> + /*
> + * Error-context string describing the conflict above, used to
> annotate any
> + * error raised while inserting conflict_log_tuple into the conflict log
> + * table. Allocated, like conflict_log_tuple, in ApplyContext.
> + */
> + char *conflict_log_errcontext;

Yeah there is no need for them to be in shared memory, but do we have
any other data sturcture where these fits naturally, or we can make
them global variables?

> 2) Should conflict_log_errcontext be initialized in
> logicalrep_worker_launch like other members:
> +++ b/src/include/replication/worker_internal.h
> @@ -100,6 +100,16 @@ typedef struct LogicalRepWorker
> */
> TransactionId oldest_nonremovable_xid;
>
> + /* A conflict log tuple that is prepared but not yet inserted. */
> + HeapTuple conflict_log_tuple;
> +
> + /*
> + * Error-context string describing the conflict above, used to
> annotate any
> + * error raised while inserting conflict_log_tuple into the conflict log
> + * table. Allocated, like conflict_log_tuple, in ApplyContext.
> + */
> + char *conflict_log_errcontext;

Yeah we can decide on this once we fix the comment 1)

> 3) Is the usage of heap_insert intentional here, shouldn't it be using
> generic table access method API table_tuple_insert?
> +InsertConflictLogTuple(Relation conflictlogrel)
> +{
> + ErrorContextCallback errcallback;
> +
> + /* A valid tuple must be prepared and stored in MyLogicalRepWorker. */
> + Assert(MyLogicalRepWorker->conflict_log_tuple != NULL);
> +
> + /*
> + * Set up an error context so that a failure to insert (e.g.
> the conflict
> + * log table was dropped, or an out-of-space error) carries information
> + * identifying the conflict we were trying to log.
> + */
> + errcallback.callback = conflict_log_insert_errcontext;
> + errcallback.arg = MyLogicalRepWorker->conflict_log_errcontext;
> + errcallback.previous = error_context_stack;
> + error_context_stack = &errcallback;
> +
> + heap_insert(conflictlogrel, MyLogicalRepWorker->conflict_log_tuple,
> + GetCurrentCommandId(true),
> HEAP_INSERT_NO_LOGICAL, NULL);

The reason is to not logically log the tuple by passing the flag
HEAP_INSERT_NO_LOGICAL

> 4) Is the condition remote_commit_ts > 0 done intentionally?
> + if (remote_commit_ts > 0)
> + values[attno++] = TimestampTzGetDatum(remote_commit_ts);
> + else
> + nulls[attno++] = true;
>
> As I had seen some negative values for certain timestamps. Shouldn't
> the check be != 0?
> SELECT extract(epoch FROM '1969-12-31 23:59:59+00'::timestamptz);
> extract
> -----------
> -1.000000
> (1 row)

I think the 0 can also be generated for timestamptz, but since we are
initializing `timestamptz` with 0, checking it seems correct.= 0, but
I need to put more thought into this.

PFA the updated patch set, which fixes the comments Nisha and Shveta
raised on 0001 and 0002.

--
Regards,
Dilip Kumar
Google