On Tuesday, January 17, 2023 12:55 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Jan 17, 2023 at 8:59 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Tue, Jan 17, 2023 at 8:35 AM Masahiko Sawada > <sawada.m...@gmail.com> wrote: > > > > > > On Mon, Jan 16, 2023 at 3:19 PM Amit Kapila <amit.kapil...@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_star > t). > > 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.
Thanks for fixing these. I have confirmed that all regression tests passed after applying the patches. And the patches look good to me. Best regards, Hou zj