On Mon, 26 Oct 2026 at 13:56, Hayato Kuroda (Fujitsu) <[email protected]> wrote: > > Hi, > > One of my colleagues requested me to update the commit message. I did for > 0002 and > 0004. I did not do for 0001 and 0003 because it's not intended to be included. >
Thanks for the updated patch. Few minor suggestions: 1) apply_error_context_stack used to mean "the stack including apply_error_callback", and with this patch it deliberately means the opposite. The name now says the reverse of what the variable holds. How about renaming it to apply_loop_outer_error_context_stack or a better one if you have, and adding a short comment at the definition saying that it is the stack before LogicalRepApplyLoop() pushed apply_error_callback, and that parallel apply worker errors are reported with it? 2) The patch reverses the choice made in 216a784829c, which states: "Context beyond that should use the error context callbacks that were in effect in LogicalRepApplyLoop()". The reasoning for this change is sound. Could the commit message explicitly mention that it changes the earlier choice and explain why? How about something like(feel free to change it): An error reported by a parallel apply worker is relayed by the leader apply worker. Before rethrowing it, ProcessParallelApplyMessage() replaces error_context_stack with a stack saved in LogicalRepApplyLoop(), so that the context callbacks of whatever the leader is doing at that moment (for example, a callback pushed while applying another transaction) are not run for this error. The saved stack was taken after apply_error_callback was pushed, so that callback was still run. It formats the leader's own apply state (the relation, the remote transaction and the message being applied), which has nothing to do with the transaction that failed in the worker. If the leader was applying another transaction at that moment, the error got a misleading CONTEXT line naming the relation the leader was working on, instead of just the worker's own context. This changes what commit 216a784829c chose, namely to keep the callbacks in effect in LogicalRepApplyLoop(), apply_error_callback included. That is not useful here, because the worker's own context, relayed in the error message, already says where the error happened. Save the stack before pushing apply_error_callback. Regards, Vignesh
