Hi,
On 1/23/23 21:30, Andres Freund wrote:
> That's been the case since my first post in the thread :). Mainly, it seems
> easier to detect underflow cases during subtraction that way. And the factor
> of 2 in range doesn't change a whole lot.
I just realized it the other day :).
>>>> If you have time to look at the pg_test_timing part, it'd be
>>>> appreciated. That's a it larger, and nobody looked at it yet. So I'm a bit
>>>> hesitant to push it.
>>> I haven't yet pushed the pg_test_timing (nor it's small prerequisite)
>>> patch.
>>>
>>> I've attached those two patches. Feel free to include them in your series if
>>> you want, then the CF entry (and thus cfbot) makes sense again...
>> I'll include them in my new patch set and also have a careful look at them.
I reviewed the prerequisite patch which introduces
INSTR_TIME_SET_SECONDS(), as well as the pg_test_timing patch. Here my
comments:
- The prerequisite patch looks good me.
- By default, the test query in the pg_test_timing doc runs serially.
What about adding SET max_parallel_workers_per_gather = 0 to make sure
it really always does (e.g. on a system with different settings for
parallel_tuple_cost / parallel_setup_cost)? Otherwise, the numbers will
be much more flaky.
- Why have you added a case distinction for diff == 0? Have you
encountered this case? If so, how? Maybe add a comment.
- To further reduce overhead we could call INSTR_TIME_SET_CURRENT()
multiple times. But then again: why do we actually care about the
per-loop time? Why not instead sum up diff and divide by the number of
iterations to exclude all the overhead in the first place?
- In the computation of the per-loop time in nanoseconds you can now use
INSTR_TIME_GET_NANOSEC() instead of INSTR_TIME_GET_DOUBLE() * NS_PER_S.
The rest looks good to me. The rebased patches are part of the patch set
I sent out yesterday in reply to another mail in this thread.
--
David Geier
(ServiceNow)