Re: suppressing useless wakeups in logical/worker.c

I wrote:
>>> It'd probably be reasonable to file down that sharp edge by instead
>>> specifying that TimestampDifferenceMilliseconds will clamp overflowing
>>> differences to LONG_MAX.  Maybe there should be a clamp on the underflow
>>> side too ... but should it be to LONG_MIN or to zero?

After looking closer, I see that TimestampDifferenceMilliseconds
already explicitly states that its output is intended for WaitLatch
and friends, which makes it perfectly sane for it to clamp the result
to [0, INT_MAX] rather than depending on the caller to not pass
out-of-range values.

I checked existing callers, and found one place in basebackup_copy.c
that had not read the memo about TimestampDifferenceMilliseconds
never returning a negative value, and another in postmaster.c that
had not read the memo about Min() and Max() being macros.  There
are none that are unhappy about clamping to INT_MAX, and at least
one that was already assuming it could just cast the result to int.

Hence, I propose the attached.  I haven't gone as far as to change
the result type from long to int; that seems like a lot of code
churn (if we are to update WaitLatch and all callers to match)
and it won't really buy anything semantically.

            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..b1d1963729 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -1690,12 +1690,12 @@ 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").
+ * is to harmonize with that; furthermore, we clamp the result to at most
+ * INT_MAX milliseconds, because that's all that WaitLatch() allows.
  *
- * Both inputs must be ordinary finite timestamps (in current usage,
- * they'll be results from GetCurrentTimestamp()).
+ * At least one input must be an ordinary finite timestamp, else the "diff"
+ * calculation might overflow.  We do support stop_time == TIMESTAMP_INFINITY,
+ * which will result in INT_MAX wait time.
  *
  * We expect start_time <= stop_time.  If not, we return zero,
  * since then we're already past the previously determined stop_time.
@@ -1710,6 +1710,8 @@ TimestampDifferenceMilliseconds(TimestampTz start_time, TimestampTz stop_time)

     if (diff <= 0)
         return 0;
+    else if (diff >= (INT_MAX * INT64CONST(1000) - 999))
+        return (long) INT_MAX;
     else
         return (long) ((diff + 999) / 1000);
 }