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

diff --git a/src/backend/backup/basebackup_copy.c b/src/backend/backup/basebackup_copy.c
index 05470057f5b64e6125337810eef38cda53c01b78..2bb6c89f8c0cd5af607509e1ba6070b4e24aad3d 100644 (file)
--- 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 5b775cf7d0695ee966c003f87e730428d1f87254..62fba5fceedcf28dbf65a143293999cd6a2c8989 100644 (file)
--- 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 e95398db05a2097f56c20346e92834872777fd27..b0cfddd54815ff4bbbb8a632510970137885dee3 100644 (file)
--- 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 928c330897383e9ffda6ca9f01b8aa9479ad1923..47e059a409b98c25647b975917e8c9d0c876687d 100644 (file)
--- 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);
 }