Re: Suppressing useless wakeups in walreceiver

On Fri, Jan 28, 2022 at 8:26 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
> At Thu, 27 Jan 2022 23:50:04 +1300, Thomas Munro <thomas(dot)munro(at)gmail(dot)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.

Thanks for looking!

The reason why I put the timeout computation into a function is
because there are about 3 places you need to do it: at the beginning,
when rescheduling for next time, and when the configuration file
changes and you might want to recompute them. The logic to decode the
GUCs and compute the times would be duplicated. I have added a
comment about that, and tried to make the code clearer.

Do you have another idea?

> - 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.

Those two wakeup times should only be adjusted when data is received.
The calls should be exactly where last_recv_timestamp is set in
master, no?

> - 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().

Yeah, I agree that we should remove more GetCurrentTimestamp() calls.
Here's another idea: let's remove last_recv_timestamp, and just use
'now', being careful to update it after sleeping and in the receive
loop (in case it gets busy for a long time), so that it's always fresh
enough.

> 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.

Yeah, this was unnecessary refactoring. I removed that change (we
could look into moving more state into the new state object instead of
using static locals and globals, but that can be for another thread, I
got carried away...)

> The enum item WALRCV_WAKEUP_TIMEOUT looks somewhat uninformative. How
> about naming it WALRCV_WAKEUP_TERMIATE (or WALRCV_WAKEUP_TO_DIE)?

Ok, let's try TERMINATE.

> 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?

Oh, yeah, I forgot to handle wal_receiver_timeout = 0. Fixed.