On Sun, Jan 15, 2023 at 10:39 PM Tomas Vondra <[email protected]> wrote: > > I think there's a bug in how get_transaction_apply_action() interacts > with handle_streamed_transaction() to decide whether the transaction is > streamed or not. Originally, the code was simply: > > /* not in streaming mode */ > if (!in_streamed_transaction) > return false; > > But now this decision was moved to get_transaction_apply_action(), which > does this: > > if (am_parallel_apply_worker()) > { > return TRANS_PARALLEL_APPLY; > } > else if (in_remote_transaction) > { > return TRANS_LEADER_APPLY; > } > > and handle_streamed_transaction() then uses the result like this: > > /* not in streaming mode */ > if (apply_action == TRANS_LEADER_APPLY) > return false; > > Notice this is not equal to the original behavior, because the two flags > (in_remote_transaction and in_streamed_transaction) are not inverse. > That is, > > in_remote_transaction=false > > does not imply we're processing streamed transaction. It's allowed both > flags are false, i.e. a change may be "non-transactional" and not > streamed, though the only example of such thing in the protocol are > logical messages. Which are however ignored in the apply worker, so I'm > not surprised no existing test failed on this. >
Right, this is the reason we didn't catch it in our testing.
> So I think get_transaction_apply_action() should do this:
>
> if (am_parallel_apply_worker())
> {
> return TRANS_PARALLEL_APPLY;
> }
> else if (!in_streamed_transaction)
> {
> return TRANS_LEADER_APPLY;
> }
>
Yeah, something like this would work but some of the callers other
than handle_streamed_transaction() also need to be changed. See
attached.
> FWIW I've noticed this after rebasing the sequence decoding patch, which
> adds another type of protocol message with the transactional vs.
> non-transactional behavior, similar to "logical messages" except that in
> this case the worker does not ignore that.
>
> Also, I think get_transaction_apply_action() would deserve better
> comments explaining how/why it makes the decisions.
>
Okay, I have added the comments in get_transaction_apply_action() and
updated the comments to refer to the enum TransApplyAction where all
the actions are explained.
--
With Regards,
Amit Kapila.
v1-0001-Fix-the-code-to-decide-the-apply-action.patch
Description: Binary data
