Hello.
At Thu, 27 Jan 2022 23:50:04 +1300, Thomas Munro <thomas.munro@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