On Tues, Feb 28, 2023 at 11:31 AM Osumi, Takamichi/大墨 昂道 <osumi.takami...@fujitsu.com> wrote: > Hi, > > > On Monday, February 27, 2023 6:30 PM wangw.f...@fujitsu.com > <wangw.f...@fujitsu.com> wrote: > > Attach the new patch. > Thanks for sharing v3. Minor review comments and question.
Thanks for your comments. > (1) UpdateDecodingProgressAndKeepalive header comment > > The comment should be updated to explain maybe why we reset some other > flags as discussed in [1] and the functionality to update and keepalive of the > function simply. Added the comments atop the function UpdateDecodingProgressAndKeepalive about when to call this function. > (2) OutputPluginPrepareWrite > > Probably the changed error string is too wide. > > @@ -662,7 +657,7 @@ void > OutputPluginPrepareWrite(struct LogicalDecodingContext *ctx, bool last_write) > { > if (!ctx->accept_writes) > - elog(ERROR, "writes are only accepted in commit, begin and > change > callbacks"); > + elog(ERROR, "writes are only accepted in callbacks in the > OutputPluginCallbacks structure (except startup, shutdown, filter_by_origin > and > filter_prepare callbacks)"); > > I thought you can break the error message into two string lines. Or, you can > rephrase it to different expression. I tried to improve this message and broke it into two lines in the new patch. > (3) Minor question > > The patch introduced the goto statements into the cb_wrapper functions. Is the > purpose to call the update_progress_and_keepalive after pop the error stack, > even if the corresponding callback is missing ? I thought we can just have > "else" > clause for the check of the existence of callback, but did you choose the > current > goto style for readability ? I think both styles look fine to me. I haven't modified this for this version. I'll reconsider if anyone else has similar thoughts later. > (4) Name of is_skip_threshold_change > > I also feel the name of is_skip_threshold_change can be changed to > "exceeded_keepalive_threshold" or something. Other candidates are proposed > by Peter-san in [2]. Renamed this function to is_keepalive_threshold_exceeded. Please see the new patch in [1]. [1] - https://www.postgresql.org/message-id/OS3PR01MB6275C6CA72222C0C23730A319EAD9%40OS3PR01MB6275.jpnprd01.prod.outlook.com Regards, Wang wei