From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nathan Bossart <nathandbossart@gmail.com>
cc: Thomas Munro <thomas.munro@gmail.com>,
        "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
        "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: suppressing useless wakeups in logical/worker.c
In-reply-to: <20230126202301.GA1703463@nathanxps13>
References: <2557815.1674603908@sss.pgh.pa.us>
 <20230125235004.GA1327755@nathanxps13>
 <CA+hUKGJ=oFEX=TjeSCnGmqr_KseGLJxy=RnJMwRqPrX7N-Q2sg@mail.gmail.com>
 <20230126003319.GA1507251@nathanxps13> <2878152.1674700077@sss.pgh.pa.us>
 <CA+hUKGLpH=y4=Zx+=oLE8WvLqLHWNC-PEDARrrt=x=J_REL9Fg@mail.gmail.com>
 <2888102.1674705840@sss.pgh.pa.us> <3126727.1674759248@sss.pgh.pa.us>
 <20230126194812.GA1702315@nathanxps13> <3165443.1674763470@sss.pgh.pa.us>
 <20230126202301.GA1703463@nathanxps13>
Comments: In-reply-to Nathan Bossart <nathandbossart@gmail.com>
	message dated "Thu, 26 Jan 2023 12:23:01 -0800"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3211335.1674767386.0@sss.pgh.pa.us>
Date: Thu, 26 Jan 2023 16:09:51 -0500
Message-ID: <3211341.1674767391@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3211341.1674767391%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3211335.1674767386.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-TimestampDifferenceMilliseconds-2.patch";
	charset="us-ascii"
Content-ID: <3211335.1674767386.2@sss.pgh.pa.us>
Content-Description: fix-TimestampDifferenceMilliseconds-2.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/backup/basebackup_copy.c b/src/backend/backup/bas=
ebackup_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 >=3D PROGRESS_REPORT_MILLISECOND_THRESHOLD)
+		if (ms >=3D PROGRESS_REPORT_MILLISECOND_THRESHOLD ||
+			now < mysink->last_progress_report_time)
 		{
 			mysink->last_progress_report_time =3D 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 !=3D 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 =3D (int) TimestampDifferenceMilliseconds(GetCurrentTimestamp(),
+												   next_wakeup);
+		return Min(60 * 1000, ms);
 	}
 =

 	return 60 * 1000;
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replicati=
on/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 =3D 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 =3D 0; i < NUM_WALRCV_WAKEUPS; ++i)
 					nextWakeup =3D 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 =3D GetCurrentTimestamp();
-				nap =3D (int) Min(INT_MAX, Max(0, (nextWakeup - now + 999) / 1000));
+				nap =3D 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/tim=
estamp.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, Timest=
ampTz 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 <=3D 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 =3D stop_time - start_time;
+	TimestampTz diff;
 =

-	if (diff <=3D 0)
+	/* Deal with zero or negative elapsed time quickly. */
+	if (start_time >=3D 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 >=3D (INT_MAX * INT64CONST(1000) - 999))
+		return (long) INT_MAX;
 	else
 		return (long) ((diff + 999) / 1000);
 }

------- =_aaaaaaaaaa0--