Re: Suppressing useless wakeups in walreceiver

On Wed, Oct 5, 2022 at 11:38 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> I moved as much as I could to 0001 in v4.

Some comments on v4-0002:

1. You might want to use PG_INT64_MAX instead of INT64_MAX for portability?

2. With the below change, the time walreceiver spends in
XLogWalRcvSendReply() is also included for XLogWalRcvSendHSFeedback
right? I think it's a problem given that XLogWalRcvSendReply() can
take a while. Earlier, this wasn't the case, each function calculating
'now' separately. Any reason for changing this behaviour? I know that
GetCurrentTimestamp(); isn't cheaper all the time, but here it might
result in a change in the behaviour.
-            XLogWalRcvSendReply(false, false);
-            XLogWalRcvSendHSFeedback(false);
+            TimestampTz now = GetCurrentTimestamp();
+
+            XLogWalRcvSendReply(state, now, false, false);
+            XLogWalRcvSendHSFeedback(state, now, false);

3. I understand that TimestampTz type is treated as microseconds.
Would you mind having a comment in below places to say why we're
multiplying with 1000 or 1000000 here? Also, do we need 1000L or
1000000L or type cast to
TimestampTz?
+            state->wakeup[reason] = now + (wal_receiver_timeout / 2) * 1000;
+            state->wakeup[reason] = now + wal_receiver_timeout * 1000;
+            state->wakeup[reason] = now +
wal_receiver_status_interval * 1000000;

4. How about simplifying WalRcvComputeNextWakeup() something like below?
static void
WalRcvComputeNextWakeup(WalRcvInfo *state, WalRcvWakeupReason reason,
                        TimestampTz now)
{
    TimestampTz ts = INT64_MAX;

    switch (reason)
    {
    case WALRCV_WAKEUP_TERMINATE:
        if (wal_receiver_timeout > 0)
            ts = now + (TimestampTz) (wal_receiver_timeout * 1000L);
        break;
    case WALRCV_WAKEUP_PING:
        if (wal_receiver_timeout > 0)
            ts = now + (TimestampTz) ((wal_receiver_timeout / 2) * 1000L);
        break;
    case WALRCV_WAKEUP_HSFEEDBACK:
        if (hot_standby_feedback && wal_receiver_status_interval > 0)
            ts = now + (TimestampTz) (wal_receiver_status_interval * 1000000L);
        break;
    case WALRCV_WAKEUP_REPLY:
        if (wal_receiver_status_interval > 0)
            ts = now + (TimestampTz) (wal_receiver_status_interval * 1000000);
        break;
    default:
        /* An error is better here. */
    }

    state->wakeup[reason] = ts;
}

5. Can we move below code snippets to respective static functions for
better readability and code reuse?
This:
+                /* Find the soonest wakeup time, to limit our nap. */
+                nextWakeup = INT64_MAX;
+                for (int i = 0; i < NUM_WALRCV_WAKEUPS; ++i)
+                    nextWakeup = Min(state.wakeup[i], nextWakeup);
+                nap = Max(0, (nextWakeup - now + 999) / 1000);

And this:

+                    now = GetCurrentTimestamp();
+                    for (int i = 0; i < NUM_WALRCV_WAKEUPS; ++i)
+                        WalRcvComputeNextWakeup(&state, i, now);

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com