Re: Misuse of TimestampDifference() in the autoprewarm feature of pg_prewarm
Alexey Kondratov <a.kondratov@postgrespro.ru>
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Robert Haas
<robertmhaas@gmail.com>
Date: 2020-11-10T16:58:44Z
Lists: pgsql-hackers
Attachments
- v3-0003-pg_prewarm-add-tap-test-for-autoprewarm-feature.patch (text/x-diff) patch v3-0003
- v3-0002-pg_prewarm-fix-autoprewarm_interval-behaviour.patch (text/x-diff) patch v3-0002
- v3-0001-Implement-TimestampDifferenceMilliseconds.patch (text/x-diff) patch v3-0001
On 2020-11-09 23:25, Tom Lane wrote: > Alexey Kondratov <a.kondratov@postgrespro.ru> writes: >> On 2020-11-09 21:53, Tom Lane wrote: >>> 0002 seems like a pretty clear bug fix, though I wonder if this is >>> exactly >>> what we want to do going forward. It seems like a very large >>> fraction of >>> the callers of TimestampDifference would like to have the value in >>> msec, >>> which means we're doing a whole lot of expensive and error-prone >>> arithmetic to break down the difference to sec/usec and then put it >>> back together again. Let's get rid of that by inventing, say >>> TimestampDifferenceMilliseconds(...). > >> Yeah, I get into this problem after a bug in another extension — >> pg_wait_sampling. I have attached 0002, which implements >> TimestampDifferenceMilliseconds(), so 0003 just uses this new function >> to solve the initial issues. If it looks good to you, then we can >> switch >> all similar callers to it. > > Yeah, let's move forward with that --- in fact, I'm inclined to > back-patch it. (Not till the current release cycle is done, though. > I don't find this important enough to justify a last-moment patch.) > > BTW, I wonder if we shouldn't make TimestampDifferenceMilliseconds > round any fractional millisecond up rather than down. Rounding down > seems to create a hazard of uselessly waking just before the delay is > completed. Better to wake just after. > Yes, it make sense. I have changed TimestampDifferenceMilliseconds() to round result up if there is a reminder. After looking on the autoprewarm code more closely I have realised that this 'double dump' issues was not an issues at all. I have just misplaced a debug elog(), so its second output in the log was only indicating that we calculated delay_in_ms one more time. Actually, even with wrong calculation of delay_in_ms the only problem was that we were busy looping with ~1 second interval instead of waiting on latch. It is still a buggy behaviour, but much less harmful than I have originally thought. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Commits
-
Fix and simplify some usages of TimestampDifference().
- 210564a74409 9.5.25 landed
- e87139b43345 10.16 landed
- cd39c23a21ac 9.6.21 landed
- 3a89ea0eb64a 11.11 landed
- afce7908d706 13.2 landed
- 171c457cd060 12.6 landed
- ec29427ce2a4 14.0 landed