On Monday, November 7, 2022 4:17 PM Peter Smith <smithpb2250@gmail.com>
>
> Here are my review comments for v42-0001
Thanks for the comments.
> ======
>
> 28. handle_streamed_transaction
>
> static bool
> handle_streamed_transaction(LogicalRepMsgType action, StringInfo s) {
> - TransactionId xid;
> + TransactionId current_xid;
> + ParallelApplyWorkerInfo *winfo;
> + TransApplyAction apply_action;
> + StringInfoData origin_msg;
> +
> + apply_action = get_transaction_apply_action(stream_xid, &winfo);
>
> /* not in streaming mode */
> - if (!in_streamed_transaction)
> + if (apply_action == TRANS_LEADER_APPLY)
> return false;
>
> - Assert(stream_fd != NULL);
> Assert(TransactionIdIsValid(stream_xid));
>
> + origin_msg = *s;
>
> ~
>
> 28b.
> Why not assign it at the declaration, the same as apply_handle_stream_prepare
> does?
The assignment is unnecessary for non-streaming transaction, so I delayed it.
> ~
>
> 44b.
> If this is always written to a file, then wouldn't a better function name be
> something including the word "serialize" - e.g.
> serialize_message()?
I feel it would be better to be consistent with the existing style stream_xxx_xx().
I think I have addressed all the comments, but since quite a few logics are
changed in the new version so I might missed something. And dome code wrapping need to
be adjusted, I plan to run pg_indent for next version.
Best regards,
Hou zj
