Hello. At Thu, 27 Jan 2022 23:50:04 +1300, Thomas Munro <thomas.mu...@gmail.com> wrote in > While working on WaitEventSet-ifying various codepaths, I found it > strange that walreceiver wakes up 10 times per second while idle. > Here's a draft patch to compute the correct sleep time.
Agree to the objective. However I feel the patch makes the code somewhat less readable maybe because WalRcvComputeNextWakeup hides the timeout deatils. Of course other might thing differently. - ping_sent = false; - XLogWalRcvProcessMsg(buf[0], &buf[1], len - 1, - startpointTLI); + WalRcvComputeNextWakeup(&state, + WALRCV_WAKEUP_TIMEOUT, + last_recv_timestamp); + WalRcvComputeNextWakeup(&state, + WALRCV_WAKEUP_PING, + last_recv_timestamp); The calculated target times are not used within the closest loop and the loop is quite busy. WALRCV_WAKEUP_PING is the replacement of ping_sent, but I think the computation of both two target times would be better done after the loop only when the "if (len > 0)" block was passed. - XLogWalRcvSendReply(false, false); + XLogWalRcvSendReply(&state, GetCurrentTimestamp(), + false, false); The GetCurrentTimestamp() is same with last_recv_timestamp when the recent recv() had any bytes received. So we can avoid the call to GetCurrentTimestamp in that case. If we do the change above, the same flag notifies the necessity of separete GetCurrentTimestamp(). I understand the reason for startpointTLI being stored in WalRcvInfo but don't understand about primary_has_standby_xmin. It is just moved from a static variable of XLogWalRcvSendHSFeedback to the struct member that is modifed and read only by the same function. The enum item WALRCV_WAKEUP_TIMEOUT looks somewhat uninformative. How about naming it WALRCV_WAKEUP_TERMIATE (or WALRCV_WAKEUP_TO_DIE)? WALRCV_WAKEUP_TIMEOUT and WALRCV_WAKEUP_PING doesn't fire when wal_receiver_timeout is zero. In that case we should not set the timeouts at all to avoid spurious wakeups? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
