On Tue, Jan 17, 2023 at 8:35 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Jan 16, 2023 at 3:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > 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?
>
I think so.
> How about adding an assertion in apply_handle_stream_start() to make
> sure the stream_xid is invalid?
>
I think it would be better to add such an assert in
apply_handle_begin/apply_handle_begin_prepare because there won't be a
problem if we start_stream message even when stream_xid is valid.
However, maybe it is better to add in all three functions
(apply_handle_begin/apply_handle_begin_prepare/apply_handle_stream_start).
What do you think?
> ---
> 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?
>
Yeah, that may be better. Shall we do that as part of this patch only
or as a separate patch?
--
With Regards,
Amit Kapila.