Re: Suppressing useless wakeups in walreceiver

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

Yeah, I understand that and am having no better ideas so I'm fine with
that.

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

Yes.

> The calls should be exactly where last_recv_timestamp is set in
> master, no?

The calcuations at other than the last one iteration are waste of
cycles and I thought the loop as a kind of tight-loop.  So I don't
mind we consider the additional cycles are acceptable.  (Just I'm not
sure.)

> > -          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 like it.  It could be slightly off but it doesn't matter at all.

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

Thanks:)

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

Yeah, looks fine.

+static void
+WalRcvComputeNextWakeup(WalRcvInfo *state,
+                        WalRcvWakeupReason reason,
+                        TimestampTz now)
+{
+    switch (reason)
+    {
...
+    default:
+        break;
+    }
+}

Mmm.. there's NUM_WALRCV_WAKEUPS.. But I think we don't need to allow
the case.  Isn't it better to replace the break with Assert()?


It might be suitable for another patch, but we can do that
together. If we passed "now" to the function XLogWalRcvProcessMsg, we
would be able to remove further several calls to
GetCurrentTimestamp(), in the function itself and
ProcessWalSndrMessage (the name is improperly abbreviated..).

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center