Nathan Bossart <nathandboss...@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);
}
