Re: Proposal: Conflict log history table for Logical Replication

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.

2. Keep only teardown in the catch and move the insert out (minimal).
Leave just the plain teardown in the PG_CATCH (reset origin, save the
error, signal the leader, abort) and move the conflict-tuple insert
and the re-throw to after PG_END_TRY(), from a clean state. It's
behavior-preserving (same order, same leader handshake), needs no new
infrastructure, keeps the apply worker writing the row directly, and
keeps error recovery in the worker's top-level function. This is just
to ensure that we don't perform insert-into-clt inside catch but
otherwise the functionality will be the same.

3. The third, and most radical, option, which I am not in favor of:
hand the conflict off to a separate worker. ReportApplyConflict()
could send the conflict to a dedicated, database-connected worker that
inserts into the conflict table and acknowledges back, so the apply
worker never runs a transaction in its error path at all. It does
cleanly sidestep the catch, but at a disproportionate cost: it needs a
brand-new per-subscription worker, an shm_mq protocol to ship the
conflict payload and return an ack, failure and lifecycle handling,
and yet more coordination with the parallel-apply leader. As per my
current understanding, that is a large amount of new machinery for no
correctness gain over the current code. I do not think it is worth
pursuing.

Thoughts?

--
With Regards,
Amit Kapila.