From 3a28d78089289794fda86cdbd275fc4756c6c6aa Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 26 Jan 2023 17:09:12 -0500 Subject: [PATCH] Improve TimestampDifferenceMilliseconds to cope with overflow sanely. We'd like to use TimestampDifferenceMilliseconds with the stop_time possibly being TIMESTAMP_INFINITY, but up to now it's disclaimed responsibility for overflow cases. Define it to clamp its output to the range [0, INT_MAX], handling overflow correctly. (INT_MAX rather than LONG_MAX seems appropriate, because the function is already described as being intended for calculating wait times for WaitLatch et al, and that infrastructure only handles waits up to INT_MAX. Also, this choice gets rid of cross-platform behavioral differences.) Having done that, we can replace some ad-hoc code in walreceiver.c with a simple call to TimestampDifferenceMilliseconds. While at it, fix some buglets in existing callers of TimestampDifferenceMilliseconds: basebackup_copy.c had not read the memo about TimestampDifferenceMilliseconds never returning a negative value, and postmaster.c had not read the memo about Min() and Max() being macros with multiple-evaluation hazards. Neither of these quite seem worth back-patching. Patch by me; thanks to Nathan Bossart for review. Discussion: https://postgr.es/m/3126727.1674759248@sss.pgh.pa.us --- src/backend/backup/basebackup_copy.c | 3 ++- src/backend/postmaster/postmaster.c | 11 ++++++----- src/backend/replication/walreceiver.c | 12 +++--------- src/backend/utils/adt/timestamp.c | 21 +++++++++++++-------- 4 files changed, 24 insertions(+), 23 deletions(-) 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); } -- 2.30.2