Re: Print latency percentiles in pgbench

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