On Tue, Aug 10, 2021 at 10:27 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Tue, Aug 10, 2021 at 3:07 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached the latest patches that incorporated all comments I got
> > so far. Please review them.
> >
>
> Some initial review comments on the v6-0001 patch:
Thanks for reviewing the patch!
>
>
> src/backend/replication/logical/proto.c:
> (1)
>
> + TimestampTz committs;
>
> I think it looks better to name "committs" as "commit_ts", and also is
> more consistent with naming for other member "remote_xid".
Fixed.
>
> src/backend/replication/logical/worker.c:
> (2)
> To be consistent with all other function headers, should start
> sentence with capital: "get" -> "Get"
>
> + * get string representing LogicalRepMsgType.
Fixed
>
> (3) It looks a bit cumbersome and repetitive to set/update the members
> of apply_error_callback_arg in numerous places.
>
> I suggest making the "set_apply_error_context..." and
> "reset_apply_error_context..." functions as "static inline void"
> functions (moving them to the top part of the source file, and
> removing the existing function declarations for these).
>
> Also, can add something similar to below:
>
> static inline void
> set_apply_error_callback_xid(TransactionId xid)
> {
> apply_error_callback_arg.remote_xid = xid;
> }
>
> static inline void
> set_apply_error_callback_xid_info(TransactionId xid, TimestampTz commit_ts)
> {
> apply_error_callback_arg.remote_xid = xid;
> apply_error_callback_arg.commit_ts = commit_ts;
> }
>
> so that instances of, for example:
>
> apply_error_callback_arg.remote_xid = prepare_data.xid;
> apply_error_callback_arg.committs = prepare_data.commit_time;
>
> can be:
>
> set_apply_error_callback_tx_info(prepare_data.xid, prepare_data.commit_time);
Okay. I've added set_apply_error_callback_xact() function to set
transaction information to apply error callback. Also, I inlined those
helper functions since we call them every change.
>
> (4) The apply_error_callback() function is missing a function header/comment.
Added.
The fixes for the above comments are incorporated in the v7 patch I
just submitted[1].
Regards,
[1] https://www.postgresql.org/message-id/CAD21AoALAq_0q_Zz2K0tO%3DkuUj8aBrDdMJXbey1P6t4w8snpQQ%40mail.gmail.com
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
