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. (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. (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. (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 (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 ? Best Regards, Takamichi Osumi