On Sat, Jan 28, 2023 at 10:26:25AM +0530, Amit Kapila wrote: > On Fri, Jan 27, 2023 at 4:07 AM Tom Lane <t...@sss.pgh.pa.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. TBH I'm beginning to wonder whether all this is really worth it to prevent waking up once per second. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com