On Thur, Mar 9, 2023 13:26 PM Peter Smith <smithpb2...@gmail.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
