Re: Suppressing useless wakeups in walreceiver

From: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
To: thomas(dot)munro(at)gmail(dot)com
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Suppressing useless wakeups in walreceiver
Date: 2022-01-28 07:26:22
Message-ID: 20220128.162622.1547625804564085870.horikyota.ntt@gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hello.

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.

- 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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro Horiguchi 2022-01-28 07:57:42 Re: Is it correct to update db state in control file as "shutting down" during end-of-recovery checkpoint?
Previous Message Laurenz Albe 2022-01-28 07:22:19 Re: Why is INSERT-driven autovacuuming based on pg_class.reltuples?