Hello, +1 to the NaN rejection, tests, and documenting the '=' form.
One small addition: duplicates like --latency-percentiles=50,50 are accepted; probably fine, but worth confirming that's intentional. Best, Phil Alger -- EDB On Wednesday, November 4th, 2026 at 08:51, Hữu Đôn Hoàng <[email protected]> wrote: > 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 > > > > >
publickey - [email protected] - 0x504BCE4B.asc
Description: application/pgp-keys
signature.asc
Description: OpenPGP digital signature
