Here are some review comments for v6-0001
======
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. */
======
2. rollback_prepared_cb_wrapper
/*
* If the plugin support two-phase commits then rollback prepared callback
* is mandatory
+ *
+ * FIXME: This should have been caught much earlier.
*/
if (ctx->callbacks.rollback_prepared_cb == NULL)
ereport(ERROR,
~
Why is this seemingly unrelated FIXME still in the patch? I thought it
was posted a while ago (See [1] comment #8) that this would be
deleted.
~~~
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?
~~~
5. UpdateProgressAndKeepalive
I had previously suggested (See [2] comment #3) that the code might be
simplified if the "is_keepalive_threshold_exceeded(ctx)" check was
pushed down into this function, but it seems like nobody else gave any
opinion for/against that idea yet... so the question still stands.
======
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)
------
[1]
https://www.postgresql.org/message-id/OS3PR01MB6275C6CA72222C0C23730A319EAD9%40OS3PR01MB6275.jpnprd01.prod.outlook.com
[2] https://www.postgresql.org/message-id/CAHut%2BPt3ZEMo-KTF%3D5KJSU%2BHdWJD19GPGGCKOmBeM47484Ychw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.
