From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: suppressing useless wakeups in logical/worker.c
Date: Thu, 26 Jan 2023 13:54:08 -0500
Message-ID: <3126727.1674759248@sss.pgh.pa.us> (raw)
In-Reply-To: <2888102.1674705840@sss.pgh.pa.us>
References: <20221202195503.GD2277157@nathanxps13>
<TYAPR01MB58669549D0137CC76A9BB6A0F5189@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<20221205173523.GA2714833@nathanxps13>
<20230109174217.GA1112698@nathanxps13>
<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>
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
Attachments:
[text/x-diff] fix-TimestampDifferenceMilliseconds.patch (4.0K, ../3126727.1674759248@sss.pgh.pa.us/2-fix-TimestampDifferenceMilliseconds.patch)
download | inline diff:diff --git a/src/backend/backup/basebackup_copy.c b/src/backend/backup/basebackup_copy.cindex 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.cindex 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.cindex 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.cindex 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);
}
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, nathandbossart@gmail.com, thomas.munro@gmail.com, kuroda.hayato@fujitsu.com
Subject: Re: suppressing useless wakeups in logical/worker.c
In-Reply-To: <3126727.1674759248@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox