Hi,
On Monday, February 27, 2023 6:30 PM wangw.fnst@fujitsu.com <wangw.fnst@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
toupdate 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_keepaliveafter pop the error stack, even if the corresponding callback is missing ? I thought we
canjust 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
candidatesare 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
