Re: suppressing useless wakeups in logical/worker.c

On Wed, Feb 1, 2023 at 5:35 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> On Sat, Jan 28, 2023 at 10:26:25AM +0530, Amit Kapila wrote:
> > On Fri, Jan 27, 2023 at 4:07 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Returning to the prior patch ... I don't much care for this:
> >>
> >> + /* Maybe there will be a free slot in a second... */
> >> + retry_time = TimestampTzPlusSeconds(now, 1);
> >> + LogRepWorkerUpdateSyncStartWakeup(retry_time);
> >>
> >> We're not moving the goalposts very far on unnecessary wakeups if
> >> we have to do that. Do we need to get a wakeup on sync slot free?
> >> Although having to send that to every worker seems ugly. Maybe this
> >> is being done in the wrong place and we need to find a way to get
> >> the launcher to handle it.
>
> It might be feasible to set up a before_shmem_exit() callback that wakes up
> the apply worker (like is already done for the launcher). I think the
> apply worker is ordinarily notified via the tablesync worker's notify_pid,
> but AFAICT there's no guarantee that the apply worker hasn't restarted with
> a different PID.
>
> > + /*
> > + * Since process_syncing_tables() is called conditionally, the
> > + * tablesync worker start wakeup time might be in the past, and we
> > + * can't know for sure when it will be updated again. Rather than
> > + * spinning in a tight loop in this case, bump this wakeup time by
> > + * a second.
> > + */
> > + now = GetCurrentTimestamp();
> > + if (wakeup[LRW_WAKEUP_SYNC_START] < now)
> > + wakeup[LRW_WAKEUP_SYNC_START] =
> > TimestampTzPlusSeconds(wakeup[LRW_WAKEUP_SYNC_START], 1);
> >
> > Do we see unnecessary wakeups without this, or delay in sync?
>
> I haven't looked too cloesly to see whether busy loops are likely in
> practice.
>
> > BTW, do we need to do something about wakeups in
> > wait_for_relation_state_change()?
>
> ... and wait_for_worker_state_change(), and copy_read_data(). From a quick
> glance, it looks like fixing these would be a more invasive change.
>

What kind of logic do you have in mind to avoid waking up once per
second in those cases?

> TBH
> I'm beginning to wonder whether all this is really worth it to prevent
> waking up once per second.
>

If we can't do it for all cases, do you see any harm in doing it for
cases where we can achieve it without adding much complexity? We can
probably add comments for others so that if someone else has better
ideas in the future we can deal with those as well.

--
With Regards,
Amit Kapila.