On Wed, Mar 8, 2023 23:55 PM Osumi, Takamichi/大墨 昂道 <osumi.takami...@fujitsu.com> wrote: > Hi, > > > On Wednesday, March 8, 2023 11:54 AM From: wangw.f...@fujitsu.com > <wangw.f...@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