On 07/23/2018 03:57 PM, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> This does not need a configure switch.
>
> It probably is there because the OP realizes that most people wouldn't
> accept having this code compiled in.
>
>> What's the performance penalty? I am pretty sure that this is
>> measurable as wait events are stored for a backend for each I/O
>> operation as well, and you are calling a C routine within an inlined
>> function which is designed to be light-weight, doing only a four-byte
>> atomic operation.
>
> On machines with slow gettimeofday(), I suspect the cost of this
> patch would be staggering. Even with relatively fast gettimeofday,
> it doesn't look acceptable for calls in hot code paths (for instance,
> lwlock.c).
>
Yeah. I wonder if we could measure the time for a small fraction of the
wait events, and estimate the actual duration from that.
> A bigger problem is that it breaks stuff. There are countless
> calls to pgstat_report_wait_start/pgstat_report_wait_end that
> assume they have no side-effects (for example, on errno) and
> can never fail. I wouldn't trust GetCurrentTimestamp() for either.
> If the report_wait calls can't be dropped into code with *complete*
> certainty that they're safe, that's a big cost.
>
> Why exactly is this insisting on logging timestamps and not,
> say, just incrementing a counter? I think doing it like this
> is almost certain to end in rejection.
>
Because the number of times you hit wait event may not correlate with
the time you spent waiting on it. So a simple counter is not the most
useful thing.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services