RE: Proposal: Conflict log history table for Logical Replication

On Monday, July 20, 2026 7:15 PM 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;
> ...
> > + 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?
>

One thing I suspect could be creating a "this looks off" impression is the newly
added PARALLEL_TRANS_ERROR flag. It reads as if we have introduced a new channel
for the parallel worker to hand error information to the leader, when in fact
the error still travels through the existing error queue exactly as before; the
flag's only job is to keep the leader from prematurely reporting a "lost
connection" while the worker finishes writing the deferred conflict log tuple.

To make that intent clearer, I think we can drop the flag entirely and lean on
what already exists:

A worker's ERROR already reaches the leader through the existing error queue,
surfaced by CHECK_FOR_INTERRUPTS() while it waits in pa_wait_for_xact_finish().
That is the same path by which the leader normally handles a parallel worker
failure, e.g., it stops the other parallel workers and restarts apply from the
last committed transaction. So nothing new is needed to convey the error to the
leader.

The only gap the flag was covering is the silent-exit case, where the worker
goes away without reporting an error. For that, the leader can just check
whether the worker is still present in the logical replication worker pool (by
its slot and generation, the same way logicalrep_pa_worker_stop() already does),
instead of relying on a flag.

That removes the worker-side flag and the spinlock from the error path, and
keeps the parallel worker's error handling much closer to the leader apply
worker's.

Does this address the concern you had? And more broadly, does this direction
seem reasonable to you and others, or do you see a case it doesn't cover? Happy
to go another way if people feel differently.

I've attached a diff on top of the original 0001 patch to illustrate the idea.

Best Regards,
Zhijie Hou