On Tues, Feb 28, 2023 at 11:31 AM Osumi, Takamichi/大墨 昂道 <osumi.takamichi@fujitsu.com> wrote:
> Hi,
>
>
> On Monday, February 27, 2023 6:30 PM wangw.fnst@fujitsu.com
> <wangw.fnst@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
