RE: Rework LogicalOutputPluginWriterUpdateProgress

On Thur, Mar 9, 2023 13:26 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> Here are some review comments for v6-0001

Thanks for your comments.

> ======
> General.
>
> 1.
> There are lots of new comments saying:
> /* don't call update progress, we didn't really make any */
>
> but is the wording "call update progress" meaningful?
>
> Should that be written something more like:
> /* No progress has been made so there is no need to call
> UpdateProgressAndKeepalive. */

Changed.
Shortened your suggested comment using a grammar tool. So, the modified comment
looks like this:
```
No progress has been made, so don't call UpdateProgressAndKeepalive
```

> ~~~
>
> 4.
>
> @@ -1370,6 +1377,8 @@ stream_abort_cb_wrapper(ReorderBuffer *cache,
> ReorderBufferTXN *txn,
>
> /* Pop the error context stack */
> error_context_stack = errcallback.previous;
> +
> + UpdateProgressAndKeepalive(ctx, (txn->toptxn == NULL));
> }
>
> ~
>
> Are the double parentheses necessary?

I think the code looks clearer this way.

> ======
> src/backend/replication/walsender.c
>
> 6. WalSndUpdateProgressAndKeepalive
>
> Since the 'ctx' is unused here, it might be nicer to annotate that to
> make it clear it is deliberate and suppress any possible warnings
> about unused params.
>
> e.g. something like:
>
> WalSndUpdateProgressAndKeepalive(
> pg_attribute_unused() LogicalDecodingContext *ctx,
> XLogRecPtr lsn,
> TransactionId xid,
> bool did_write,
> bool finished_xact)

Because many functions don't use this approach, I’m not sure what the rules are
for using it in PG. And I think that we should discuss this on a separate thread
to check which similar functions need this kind of modification in PG source
code.

Regards,
Wang wei