git.postgresql.org Git - postgresql.git/commitdiff

git projects / postgresql.git / commitdiff

Improve TimestampDifferenceMilliseconds to cope with overflow sanely.

2023年1月26日 22:09:12 +0000 (17:09 -0500)

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

src/backend/backup/basebackup_copy.c patch | blob | blame | history

src/backend/postmaster/postmaster.c patch | blob | blame | history

src/backend/replication/walreceiver.c patch | blob | blame | history

src/backend/utils/adt/timestamp.c patch | blob | blame | history

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);

}

This is the main PostgreSQL git repository.

RSS Atom