Re: Suppressing useless wakeups in walreceiver - Mailing list pgsql-hackers

On Mon, Oct 10, 2022 at 11:21 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
>

Thanks for the updated patches.

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

Agree. However...

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

AFICS, the aim of the patch isn't optimizing around
GetCurrentTimestamp() calls and the patch does seem to change quite a
bit of them which may cause a change of the behaviour. I think that
the GetCurrentTimestamp() optimizations can go to 0003 patch in this
thread itself or we can discuss it as a separate thread to seek more
thoughts.

The 'now' in many instances in the patch may not actually represent
the true current time and it includes time taken by other operations
as well. I think this will have problems.

+            XLogWalRcvSendReply(state, now, false, false);
+            XLogWalRcvSendHSFeedback(state, now, false);

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

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

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

IMO, the duplicate lines of code aren't necessary in
WalRcvComputeNextWakeup() and that function's code isn't that complex
by removing them.

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

The second snippet is being used twice already. IMO having static
functions (perhaps inline) is much better here.

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