Hi Roman,
I've continued reviewing the patch, on master at 30faf13889b.
1. --latency-percentiles=nan passes the range check and then fails
an assertion:
$ pgbench --latency-percentiles=nan -t 10 bench
pgbench: pgbench.c:1517: latencyHistogramPercentile: Assertion
`p > 0.0 && p <= 100.0' failed.
Every comparison with NaN is false, so "percentile <= 0.0 ||
percentile > 100.0" does not reject it. Writing the check as
!(percentile > 0.0 && percentile <= 100.0) would.
2. The patch adds no tests. It would be good to cover the parse
errors (0, 101, abc, ",50", 11 values, nan) in
002_pgbench_no_server.pl and check the new output lines in
001_pgbench_with_server.pl.
3. Because the argument is optional, the list has to be attached
with "=". With a space, the list is taken as the database name:
$ pgbench --latency-percentiles 50,99 -t 10 bench
pgbench: error: too many command-line arguments (first is "bench")
$ pgbench --latency-percentiles 50,99 -t 10
pgbench: error: connection to server ... failed: FATAL: database
"50,99" does not exist
Neither message points at the option, so I think the docs should
say that the value must be given as --latency-percentiles=LIST.
4. The docs say the reported percentile "lies within the histogram
bucket that contains the requested rank", but not how wide a
bucket is. It would help to state the resolution (about 3% of the
value) and the cases where it does not hold:
- A percentile can fall outside [min, max] because of the
interpolation inside a bucket. With a script containing only
"\sleep 2 ms" (500 transactions) I got:
latency min = 2.023 ms
latency p0.1 = 2.016 ms
Clamping the result to the min/max that SimpleStats already
tracks would avoid this.
- Below 32 us an octave is a single bucket, so nothing is
interpolated: for 1000 latencies between 20 and 29 us, p1, p50
and p99 all come out as 16 us.
Regards,
Don Hoang
