The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
There is no check for (last_reply_timestamp <= 0 || wal_sender_timeout <= 0) as in other places
(in WalSndKeepaliveIfNecessary for example).
I don't think moving update of 'now' down to end of loop body is correct:
there are calls to ProcessConfigFile with SyncRepInitConfig, ProcessRepliesIfAny that can
last non-negligible time. It could lead to over sleeping due to larger computed sleeptime.
Though I could be mistaken.
I'm not sure about moving `if (!pg_is_send_pending())` in a body loop after WalSndKeepaliveIfNecessary.
Is it necessary? But it looks harmless at least.
Could patch be reduced to check after first `if (!pg_is_sendpending())` ? like:
if (!pq_is_send_pending())
- return;
+ {
+ if (last_reply_timestamp <= 0 || wal_sender_timeout <= 0)
+ {
+ CHECK_FOR_INTERRUPTS();
+ return;
+ }
+ if (now <= TimestampTzPlusMilliseconds(last_reply_timestamp, wal_sender_timeout / 2))
+ return;
+ }
If not, what problem prevents?
The new status of this patch is: Waiting on Author
