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

On Mon, Jan 16, 2023 at 3:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, Jan 15, 2023 at 10:39 PM Tomas Vondra
> <tomas.vondra@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.

Thank you for the patch.

@@ -1710,6 +1712,7 @@ apply_handle_stream_stop(StringInfo s)
        }

        in_streamed_transaction = false;
+       stream_xid = InvalidTransactionId;

We reset stream_xid also in stream_close_file() but probably it's no
longer necessary?

How about adding an assertion in apply_handle_stream_start() to make
sure the stream_xid is invalid?

---
It's not related to this issue but I realized that if the action
returned by get_transaction_apply_action() is not handled in the
switch statement, we do only Assert(false). Is it better to raise an
error like "unexpected apply action %d" just in case in order to
detect failure cases also in the production environment?


Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com