On 02/08/17 19:35, Yura Sokolov wrote: > 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. >
We also need to do actual timeout handing so that the timeout is not deferred to the end of the transaction (Which is why I moved `if (!pg_is_send_pending())` under WalSndCheckTimeOut() and WalSndKeepaliveIfNecessary() calls). > 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?
We should do CHECK_FOR_INTERRUPTS() independently of pq_is_send_pending so that it's possible to stop walsender while it's processing large transaction.
Is there any chance of getting this bugfix into Pg 10?
We've just cut back branches, so it'd be a sensible time.
