On Sun, Jan 15, 2023 at 10:39 PM Tomas Vondra <tomas.von...@enterprisedb.com> 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