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. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers