RE: Rework LogicalOutputPluginWriterUpdateProgress

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