Re: Perform streaming logical transactions by background workers and parallel apply

On Sun, Jan 15, 2023 at 10:39 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)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.