Re: suppressing useless wakeups in logical/worker.c - Mailing list pgsql-hackers
From | Tom Lane |
---|---|
Subject | Re: suppressing useless wakeups in logical/worker.c |
Date | |
Msg-id | 3211341.1674767391@sss.pgh.pa.us Whole thread Raw |
In response to | Re: suppressing useless wakeups in logical/worker.c (Nathan Bossart <nathandbossart@gmail.com>) |
Responses |
Re: suppressing useless wakeups in logical/worker.c
|
List | pgsql-hackers |
Nathan Bossart <nathandbossart@gmail.com> writes: > On Thu, Jan 26, 2023 at 03:04:30PM -0500, Tom Lane wrote: >> Hmm. I'm disinclined to add an assumption that the epoch is in the past, >> but I take your point that the subtraction would overflow with >> TIMESTAMP_INFINITY and a negative finite timestamp. Maybe we should >> make use of pg_sub_s64_overflow()? > That would be my vote. I think the 'diff <= 0' check might need to be > replaced with something like 'start_time > stop_time' so that we return 0 > for the underflow case. Right, so more like this. regards, tom lane diff --git a/src/backend/backup/basebackup_copy.c b/src/backend/backup/basebackup_copy.c index 05470057f5..2bb6c89f8c 100644 --- a/src/backend/backup/basebackup_copy.c +++ b/src/backend/backup/basebackup_copy.c @@ -215,7 +215,8 @@ bbsink_copystream_archive_contents(bbsink *sink, size_t len) * the system clock was set backward, so that such occurrences don't * have the effect of suppressing further progress messages. */ - if (ms < 0 || ms >= PROGRESS_REPORT_MILLISECOND_THRESHOLD) + if (ms >= PROGRESS_REPORT_MILLISECOND_THRESHOLD || + now < mysink->last_progress_report_time) { mysink->last_progress_report_time = now; diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 5b775cf7d0..62fba5fcee 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -1670,11 +1670,12 @@ DetermineSleepTime(void) if (next_wakeup != 0) { - /* Ensure we don't exceed one minute, or go under 0. */ - return Max(0, - Min(60 * 1000, - TimestampDifferenceMilliseconds(GetCurrentTimestamp(), - next_wakeup))); + int ms; + + /* result of TimestampDifferenceMilliseconds is in [0, INT_MAX] */ + ms = (int) TimestampDifferenceMilliseconds(GetCurrentTimestamp(), + next_wakeup); + return Min(60 * 1000, ms); } return 60 * 1000; diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c index e95398db05..b0cfddd548 100644 --- a/src/backend/replication/walreceiver.c +++ b/src/backend/replication/walreceiver.c @@ -445,7 +445,7 @@ WalReceiverMain(void) pgsocket wait_fd = PGINVALID_SOCKET; int rc; TimestampTz nextWakeup; - int nap; + long nap; /* * Exit walreceiver if we're not in recovery. This should not @@ -528,15 +528,9 @@ WalReceiverMain(void) for (int i = 0; i < NUM_WALRCV_WAKEUPS; ++i) nextWakeup = Min(wakeup[i], nextWakeup); - /* - * Calculate the nap time. WaitLatchOrSocket() doesn't accept - * timeouts longer than INT_MAX milliseconds, so we limit the - * result accordingly. Also, we round up to the next - * millisecond to avoid waking up too early and spinning until - * one of the wakeup times. - */ + /* Calculate the nap time, clamping as necessary. */ now = GetCurrentTimestamp(); - nap = (int) Min(INT_MAX, Max(0, (nextWakeup - now + 999) / 1000)); + nap = TimestampDifferenceMilliseconds(now, nextWakeup); /* * Ideally we would reuse a WaitEventSet object repeatedly diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c index 928c330897..47e059a409 100644 --- a/src/backend/utils/adt/timestamp.c +++ b/src/backend/utils/adt/timestamp.c @@ -1690,26 +1690,31 @@ TimestampDifference(TimestampTz start_time, TimestampTz stop_time, * * This is typically used to calculate a wait timeout for WaitLatch() * or a related function. The choice of "long" as the result type - * is to harmonize with that. It is caller's responsibility that the - * input timestamps not be so far apart as to risk overflow of "long" - * (which'd happen at about 25 days on machines with 32-bit "long"). - * - * Both inputs must be ordinary finite timestamps (in current usage, - * they'll be results from GetCurrentTimestamp()). + * is to harmonize with that; furthermore, we clamp the result to at most + * INT_MAX milliseconds, because that's all that WaitLatch() allows. * * We expect start_time <= stop_time. If not, we return zero, * since then we're already past the previously determined stop_time. * + * Subtracting finite and infinite timestamps works correctly, returning + * zero or INT_MAX as appropriate. + * * Note we round up any fractional millisecond, since waiting for just * less than the intended timeout is undesirable. */ long TimestampDifferenceMilliseconds(TimestampTz start_time, TimestampTz stop_time) { - TimestampTz diff = stop_time - start_time; + TimestampTz diff; - if (diff <= 0) + /* Deal with zero or negative elapsed time quickly. */ + if (start_time >= stop_time) return 0; + /* To not fail with timestamp infinities, we must detect overflow. */ + if (pg_sub_s64_overflow(stop_time, start_time, &diff)) + return (long) INT_MAX; + if (diff >= (INT_MAX * INT64CONST(1000) - 999)) + return (long) INT_MAX; else return (long) ((diff + 999) / 1000); }
pgsql-hackers by date: