Re: Suppressing useless wakeups in walreceiver

On Mon, Oct 10, 2022 at 03:22:15PM +0530, Bharath Rupireddy wrote:
> Some comments on v4-0002:

Thanks for taking a look.

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

Yes, I used PG_INT64_MAX in v5.

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

Yes, if XLogWalRcvSendReply() takes a long time, we might defer sending the
hot_standby_feedback message until a later time. The only reason I changed
this was to avoid extra calls to GetCurrentTimestamp(), which might be
expensive on some platforms. Outside of the code snippet you pointed out,
I think WalReceiverMain() has a similar problem. That being said, I'm
generally skeptical that this sort of thing is detrimental given the
current behavior (i.e., wake up every 100ms), the usual values of these
GUCs (i.e., tens of seconds), and the fact that any tasks that are
inappropriately skipped will typically be retried in the next iteration of
the loop.

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

I used INT64CONST() in v5, as that seemed the most portable, but I stopped
short of adding comments to explain all the conversions. IMO the
conversions are relatively obvious, and the units of the GUCs can be easily
seen in guc_tables.c.

> 4. How about simplifying WalRcvComputeNextWakeup() something like below?

Other than saving a couple lines of code, IMO this doesn't meaningfully
simplify the function or improve readability.

> 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);

This did cross my mind, but I opted to avoid creating new functions because
1) they aren't likely to be reused very much, and 2) I actually think it
might hurt readability by forcing developers to traipse around the file to
figure out what these functions are actually doing. It's not like it would
save many lines of code, either.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com