Improve TimestampDifferenceMilliseconds to cope with overflow sanely.
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
This commit is contained in:
parent
24ff700f6a
commit
3a28d78089
|
@ -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;
|
||||
|
||||
|
|
|
@ -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;
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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);
|
||||
}
|
||||
|
|
Loading…
Reference in New Issue