On Tue, Jan 17, 2023 at 8:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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?
>
Please find attached the updated patches to address the above
comments. I think we can combine and commit them as one patch as both
are related.
--
With Regards,
Amit Kapila.
