Re: Proposal: Conflict log history table for Logical Replication

On Mon, Jul 20, 2026 at 3:14 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Jul 9, 2026 at 7:40 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >
> > The concern is about this code:
> >
> > + PG_TRY();
> > + {
> > + LogicalParallelApplyLoop(mqh);
> > + }
> > + PG_CATCH();
> > + {
> > + MemoryContext oldcontext;
> > + ErrorData *edata;
> > +
> ...
> > + replorigin_xact_clear(true);
> > +
> ...
> ...
> > + oldcontext = MemoryContextSwitchTo(TopMemoryContext);
> > + edata = CopyErrorData();
> > + MemoryContextSwitchTo(oldcontext);
> > +
> > + FlushErrorState();
> > + error_context_stack = NULL;
> > +
> ...
> ...
> > + pa_set_xact_state(MyParallelShared, PARALLEL_TRANS_ERROR);
> > +
> > + AbortOutOfAnyTransaction();
> > +
> ....
> > + ProcessPendingConflictLogTuple();
> > +
> > + /* Re-throw the original error, which reports it to the leader. */
> > + ReThrowError(edata);
> > + }
> > + PG_END_TRY();
> >
> > I see your point about this being the outermost try/catch, but this
> > still doesn't look good to me. It seems different than the way other
> > workers do error recovery, and I am really doubtful that is is safe.
> >
>
> Let me first make the case why I think the current mechanism is safe,
> and then lay out a few alternatives in case the handling in the CATCH
> block is what is bothering you.
>
> Why the current code in patch is safe. After
> AbortOutOfAnyTransaction() the backend is at TBLOCK_DEFAULT with all
> locks, buffers, resource owners, and snapshots released, and not in a
> critical section; so starting a fresh transaction there to do the
> insert should be okay. This isn't a new pattern for the apply worker
> either: DisableSubscriptionAndExit() already aborts and then runs a
> fresh transaction that updates the catalog and commits, from this same
> catch. For the parallel apply worker, the leader does not tear the
> worker down mid-insert: pa_wait_for_xact_finish() spins in a while
> (pa_get_xact_state(...) == PARALLEL_TRANS_ERROR) loop waiting for the
> worker to report the real error via the error queue, and the comment
> there notes this "keeps the worker alive long enough to finish writing
> the conflict log tuple." The worker sets PARALLEL_TRANS_ERROR before
> the abort and sends the error only after the insert, so the ordering
> holds.
>
> The one way the current code does look different from how other
> workers recover is that the recover-to-idle sequences elsewhere, the
> top-level sigsetjmp handlers and DisableSubscriptionAndExit(), bracket
> the error capture and AbortOutOfAnyTransaction() with
> HOLD_INTERRUPTS()/RESUME_INTERRUPTS(), whereas the conflict path does
> not. I don't think that is needed here:
>
> - AbortTransaction() already wraps its own body in
> HOLD_INTERRUPTS()/RESUME_INTERRUPTS(), so the transaction teardown is
> protected against a pending interrupt regardless of the caller, and
> AbortOutOfAnyTransaction() has no CHECK_FOR_INTERRUPTS of its own.
> CopyErrorData() and FlushErrorState() before it don't service
> interrupts either.
> - The HOLD_INTERRUPTS() in a recover-and-continue handler such as
> autovacuum's also exists to reset any already-pending cancel/timeout
> before it resumes its main loop. The apply worker does not recover and
> continue; on error it re-throws to the bgworker's top-level handler,
> which reports the error and exits, and the launcher restarts it. So
> there is no next iteration for this catch to protect.
>
> Can you be specific as to what makes you think the current patch code
> in catch block is unsafe?
>
> The other options to avoid this form of error-recovery could be:
>
> 1. Do the abort/insert in ReportApplyConflict() (before raising the
> error). For ERROR-level conflicts we could abort the current
> transaction, insert the conflict row in a fresh transaction, and then
> raise the ERROR — all from ReportApplyConflict(), where we're still in
> normal execution. Aborting there should be fine as it's the apply
> worker's own transaction. This avoids all the extra handling we are
> doing in the catch block. See
> v63-topup-0001-Insert-conflict-log-tuple-in-ReportApplyCo. OTOH, one
> can argue that this is a special path for aborting a transaction
> mid-way.

In a hypothetical world where we support automatic conflict resolution
in logical replication, we would not rely on raising an error in order
to record the conflict information if the resolution method is
'last-writes-win' for example. I guess that the apply worker logs the
conflict at LOG level and continues applying changes. In such a case,
I think it might be better for the CLT feature not to insert a
conflict information tuple based on the event where the execution
raises an error like the topup patch does.

That said, looking at the topop patch, I'm not sure it's okay to call
AbortOutOfAnyTransaction() in ReportApplyConflict(). It aborts the
transaction and cleans up all resources even though we still have
several functions in the call stack. If some of the functions have
PG_TRY()/PG_CATCH() in the future to clean up its resources, it would
end up with a double-free problem.

Also, I think the patch should test the case where multiple conflicts
with different log levels happen in one transaction. I've not tested
with the patch but I'm concerned that the inserting the LOG conflict
information to CTl is also aborted altogether when raising the ERROR
conflict.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com