On Monday, July 20, 2026 7:15 PM Amit Kapila <[email protected]> wrote: > On Thu, Jul 9, 2026 at 7:40 PM Robert Haas <[email protected]> > 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
v64_0002_houzj-0001-Detect-parallel-apply-worker-exit-via.patch
Description: v64_0002_houzj-0001-Detect-parallel-apply-worker-exit-via.patch
