Hi,
On Monday, February 27, 2023 6:30 PM wangw.f...@fujitsu.com
<wangw.f...@fujitsu.com> wrote:
> Attach the new patch.
Thanks for sharing v3. Minor review comments and question.
(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.
(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.
(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 ?
(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].
[1] -
https://www.postgresql.org/message-id/OS3PR01MB6275374EBE7C8CABBE6730099EAF9%40OS3PR01MB6275.jpnprd01.prod.outlook.com
[2] -
https://www.postgresql.org/message-id/CAHut%2BPt3ZEMo-KTF%3D5KJSU%2BHdWJD19GPGGCKOmBeM47484Ychw%40mail.gmail.com
Best Regards,
Takamichi Osumi
