On Wed, Mar 8, 2023 23:55 PM Osumi, Takamichi/大墨 昂道 <osumi.takamichi@fujitsu.com> wrote:
> Hi,
>
>
> On Wednesday, March 8, 2023 11:54 AM From: wangw.fnst@fujitsu.com
> <wangw.fnst@fujitsu.com> wrote:
> > Attach the new patch.
> Thanks for sharing v6 ! Few minor comments for the same.
Thanks for your comments.
> (1) commit message
>
> The old function name 'is_skip_threshold_change' is referred currently. We need
> to update it to 'is_keepalive_threshold_exceeded' I think.
Fixed.
> (2) OutputPluginPrepareWrite
>
> @@ -662,7 +656,8 @@ 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 output plugin callbacks, "
> + "except startup, shutdown, filter_by_origin, and filter_prepare.");
>
> We can remove the period at the end of error string.
Removed.
> (3) is_keepalive_threshold_exceeded's comments
>
> +/*
> + * Helper function to check whether a large number of changes have been
> skipped
> + * continuously.
> + */
> +static bool
> +is_keepalive_threshold_exceeded(LogicalDecodingContext *ctx)
>
> I suggest to update the comment slightly something like below.
> From:
> ...whether a large number of changes have been skipped continuously
> To:
> ...whether a large number of changes have been skipped without being sent to
> the output plugin continuously
Make sense.
Also, I slightly corrected the original function comment with a grammar check
tool. So, the modified comment looks like this:
```
Helper function to check for continuous skipping of many changes without sending
them to the output plugin.
```
> (4) term for 'keepalive'
>
> +/*
> + * Update progress tracking and send keep alive (if required).
> + */
>
> The 'keep alive' might be better to be replaced with 'keepalive', which looks
> commonest in other source codes. In the current patch, there are 3 different
> ways to express it (the other one is 'keep-alive') and it would be better to unify
> the term, at least within the same patch ?
Yes, agree.
Unified the comment you mentioned here ('keep alive') and the comment in the
commit message ('keep-alive') as 'keepalive'.
Regards,
Wang wei
