Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pL7P2-0007a7-LC for pgsql-hackers@arkaria.postgresql.org; Thu, 26 Jan 2023 18:55:16 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1pL7O3-0004uC-It for pgsql-hackers@arkaria.postgresql.org; Thu, 26 Jan 2023 18:54:15 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pL7O3-0004u3-A0 for pgsql-hackers@lists.postgresql.org; Thu, 26 Jan 2023 18:54:15 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pL7O0-0003gU-Ni for pgsql-hackers@postgresql.org; Thu, 26 Jan 2023 18:54:14 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 30QIs87E3126728; Thu, 26 Jan 2023 13:54:08 -0500 From: Tom Lane To: Nathan Bossart cc: Thomas Munro , "Hayato Kuroda (Fujitsu)" , "pgsql-hackers@postgresql.org" Subject: Re: suppressing useless wakeups in logical/worker.c In-reply-to: <2888102.1674705840@sss.pgh.pa.us> References: <20221202195503.GD2277157@nathanxps13> <20221205173523.GA2714833@nathanxps13> <20230109174217.GA1112698@nathanxps13> <2557815.1674603908@sss.pgh.pa.us> <20230125235004.GA1327755@nathanxps13> <20230126003319.GA1507251@nathanxps13> <2878152.1674700077@sss.pgh.pa.us> <2888102.1674705840@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Wed, 25 Jan 2023 23:04:00 -0500" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <3126603.1674759123.0@sss.pgh.pa.us> Date: Thu, 26 Jan 2023 13:54:08 -0500 Message-ID: <3126727.1674759248@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <3126603.1674759123.1@sss.pgh.pa.us> 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fix-TimestampDifferenceMilliseconds.patch"; charset="us-ascii" Content-ID: <3126603.1674759123.2@sss.pgh.pa.us> Content-Description: fix-TimestampDifferenceMilliseconds.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..b1d1963729 100644 --- a/src/backend/utils/adt/timestamp.c +++ b/src/backend/utils/adt/timestamp.c @@ -1690,12 +1690,12 @@ 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"). + * 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 "dif= f" + * calculation might overflow. We do support stop_time =3D=3D TIMESTAMP_= INFINITY, + * which will result in INT_MAX wait time. * * We expect start_time <=3D 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_ti= me, TimestampTz stop_time) = if (diff <=3D 0) return 0; + else if (diff >=3D (INT_MAX * INT64CONST(1000) - 999)) + return (long) INT_MAX; else return (long) ((diff + 999) / 1000); } ------- =_aaaaaaaaaa0--