RE: Rework LogicalOutputPluginWriterUpdateProgress - Mailing list pgsql-hackers

From wangw.fnst@fujitsu.com
Subject RE: Rework LogicalOutputPluginWriterUpdateProgress
Date
Msg-id OS3PR01MB627544BA7D8524D314657AF19EBA9@OS3PR01MB6275.jpnprd01.prod.outlook.com
Whole thread Raw
In response to RE: Rework LogicalOutputPluginWriterUpdateProgress  ("Takamichi Osumi (Fujitsu)" <osumi.takamichi@fujitsu.com>)
List pgsql-hackers
On Wed, Mar 8, 2023 23:55 PM Osumi, Takamichi/大墨 昂道 <osumi.takamichi@fujitsu.com> wrote:
> 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.

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

pgsql-hackers by date:

Previous
From: "wangw.fnst@fujitsu.com"
Date:
Subject: RE: Rework LogicalOutputPluginWriterUpdateProgress
Next
From: "wangw.fnst@fujitsu.com"
Date:
Subject: RE: Rework LogicalOutputPluginWriterUpdateProgress