Nathan Bossart <nathandbossart@gmail.com> writes:
> I think we might risk overflowing "long" when all the wakeup times are
> DT_NOEND:
> * 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").
> Maybe we can adjust that function or create a new one to deal with this.
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?
BTW, as long as we're discussing roundoff gotchas, I noticed while
testing your previous patch that there's some inconsistency between
TimestampDifferenceExceeds and TimestampDifferenceMilliseconds.
What you submitted at [1] did this:
+ if (TimestampDifferenceExceeds(last_start, now,
+ wal_retrieve_retry_interval))
+ ...
+ else
+ {
+ long elapsed;
+
+ elapsed = TimestampDifferenceMilliseconds(last_start, now);
+ wait_time = Min(wait_time, wal_retrieve_retry_interval - elapsed);
+ }
and I discovered that that could sometimes busy-wait by repeatedly
falling through to the "else", but then calculating elapsed ==
wal_retrieve_retry_interval and hence setting wait_time to zero.
I fixed it in the committed version [2] by always computing "elapsed"
and then checking if that's strictly less than
wal_retrieve_retry_interval, but I bet there's existing code with the
same issue. I think we need to take a closer look at making
TimestampDifferenceMilliseconds' roundoff behavior match the outcome of
TimestampDifferenceExceeds comparisons.
regards, tom lane
[1] https://www.postgresql.org/message-id/20230110174345.GA1292607%40nathanxps13
[2] https://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=5a3a95385