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.
(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
currentpatch, there are 3 different ways to express it (the other one is 'keep-alive') and it would be better to unify
theterm, at least within the same patch ?
Best Regards,
Takamichi Osumi
