From b98f634862ec72518fb7b053de6bb145767ab7bc Mon Sep 17 00:00:00 2001 From: Zakhar Pashkin Date: Thu, 5 Nov 2026 12:18:53 +0400 Subject: [PATCH v2] pgbench: validate latency percentile options Reject NaN by expressing the accepted percentile interval positively. Keep strtod conversion and errno checks, and reject an empty final list element using the existing invalid-list diagnostic. Move the existing repeated-option guard to the common option handler so that bare flags follow the same rule as explicit lists. Add argument regressions that check the diagnostic, status and stdout, and successful-run checks for defaults, boundaries, ten entries, order, duplicates and whitespace. Clarify the proposed CLI contract in the docs. This is an incremental correction and test contribution on top of Roman Khapov's latency-percentile feature proposal, CF #7567. --- doc/src/sgml/ref/pgbench.sgml | 10 +++- src/bin/pgbench/pgbench.c | 9 ++- src/bin/pgbench/t/001_pgbench_with_server.pl | 58 ++++++++++++++++++++ src/bin/pgbench/t/002_pgbench_no_server.pl | 57 +++++++++++++++++++ 4 files changed, 126 insertions(+), 8 deletions(-) diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 060d53a..e51b599 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -830,9 +830,13 @@ pgbench options d Report the minimum, maximum and percentiles of the transaction latency in the main and per-script reports, in addition to the average and standard deviation. The option accepts an optional - comma-separated list of percentiles to show; each value must be - greater than 0 and not greater than 100, and at most 10 values - can be specified. When the option is given without a value, the + comma-separated list of percentiles to show; each value must be a + finite number greater than 0 and not greater than 100, and at most + 10 values can be specified. If a list is provided, attach it to the + option with =, for example + --latency-percentiles=50,95,99. The list must not + contain empty elements, including after a final comma. The option + can only be specified once. When it is given without a value, the default set 50,95,99 is used. diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 33dad4b..b3380cb 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -6923,9 +6923,6 @@ parseLatencyPercentiles(const char *opt) { const char *p = opt; - if (num_latency_percentiles > 0) - pg_fatal("--latency-percentiles can only be specified once"); - while (*p) { char *end; @@ -6934,7 +6931,7 @@ parseLatencyPercentiles(const char *opt) errno = 0; percentile = strtod(p, &end); if (end == p || errno != 0 || - percentile <= 0.0 || percentile > 100.0) + !(percentile > 0.0 && percentile <= 100.0)) pg_fatal("invalid latency percentile value: \"%s\"", p); if (num_latency_percentiles >= MAX_LATENCY_PERCENTILES) @@ -6944,7 +6941,7 @@ parseLatencyPercentiles(const char *opt) latency_percentiles[num_latency_percentiles++] = percentile; p = end; - if (*p == ',') + if (*p == ',' && p[1] != '\0') p++; else if (*p != '\0') pg_fatal("invalid latency percentile list: \"%s\"", opt); @@ -7368,6 +7365,8 @@ main(int argc, char **argv) break; case 19: /* latency-percentiles */ benchmarking_option_set = true; + if (num_latency_percentiles > 0) + pg_fatal("--latency-percentiles can only be specified once"); if (optarg != NULL) parseLatencyPercentiles(optarg); else diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl index b669ad9..d795b39 100644 --- a/src/bin/pgbench/t/001_pgbench_with_server.pl +++ b/src/bin/pgbench/t/001_pgbench_with_server.pl @@ -44,6 +44,64 @@ my $node = PostgreSQL::Test::Cluster->new('main'); $node->init(extra => [ '--locale', 'C' ]); $node->start; +# Check percentile option parsing and output without depending on measured +# durations or the histogram's approximation error. +my @percentile_options = ( + [ 'defaults', '--latency-percentiles', [ 50, 95, 99 ] ], + [ + 'fractional and upper boundary', + '--latency-percentiles=0.5,95.5,100', + [ 0.5, 95.5, 100 ] + ], + [ + 'ten values in supplied order', + '--latency-percentiles=100,5,95,50,5,10,20,30,40,90', + [ 100, 5, 95, 50, 5, 10, 20, 30, 40, 90 ] + ]); +my $percentile_script = 0; + +for my $case (@percentile_options) +{ + my ($name, $option, $values) = @$case; + my $script = '001_pgbench_percentiles_' . ++$percentile_script; + my $percentiles = join '', + map { 'latency p' . quotemeta($_) . ' = \\d+\\.\\d{3} ms\n' } @$values; + + $node->pgbench( + "--no-vacuum --transactions=2 $option", + 0, + [ + qr{processed: 2/2}, + qr{^latency min = \d+\.\d{3} ms$}m, + qr{^latency max = \d+\.\d{3} ms$}m, + qr{^$percentiles}m + ], + [qr{^$}], + "latency percentiles: $name", + { $script => 'SELECT 1;' }); +} + +$node->pgbench( + '--no-vacuum --transactions=2', + 0, + [ qr{processed: 2/2}, qr{\A(?!.*^latency (?:p\S+|min|max) =)}ms ], + [qr{^$}], + 'no latency percentiles without option', + { '001_pgbench_no_percentiles' => 'SELECT 1;' }); + +# Use an argument array to preserve whitespace accepted by strtod(). +$node->pgbench( + '--no-vacuum --transactions=2', + 0, + [ + qr{^latency p50 = \d+\.\d{3} ms$}m, + qr{^latency p95 = \d+\.\d{3} ms$}m + ], + [qr{^$}], + 'latency percentiles allow whitespace before numbers', + { '001_pgbench_percentiles_whitespace' => 'SELECT 1;' }, + '--latency-percentiles= 50, 95'); + # tablespace for testing, because partitioned tables cannot use pg_default # explicitly and we want to test that table creation with tablespace works # for partitioned tables. diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl index 0e8d92f..525efc6 100644 --- a/src/bin/pgbench/t/002_pgbench_no_server.pl +++ b/src/bin/pgbench/t/002_pgbench_no_server.pl @@ -226,6 +226,52 @@ my @options = ( [qr{multiple}] ],); +# Percentile parsing must fail before attempting a database connection. +push @options, map { + [ + "invalid latency percentile $_", "--latency-percentiles=$_", + [qr{invalid latency percentile value:}] + ] +} ( 'nan', 'NaN', 'inf', '-inf', '0', '-1', + '100.1', '101', 'abc', '1e400', '1e-400', ',50', + '50,,95', '50,nan,99'); + +push @options, + [ + 'empty latency percentile list', '--latency-percentiles=', + [qr{empty latency percentile list:}] + ], + [ + 'latency percentile trailing junk', '--latency-percentiles=50x', + [qr{invalid latency percentile list:}] + ], + [ + 'latency percentile trailing comma', '--latency-percentiles=50,', + [qr{invalid latency percentile list:}] + ], + [ + 'latency percentiles trailing comma', '--latency-percentiles=50,95,', + [qr{invalid latency percentile list:}] + ], + [ + 'too many latency percentiles', + '--latency-percentiles=1,2,3,4,5,6,7,8,9,10,11', + [qr{too many latency percentiles \(maximum is 10\)}] + ]; + +for my $first ('--latency-percentiles', '--latency-percentiles=50') +{ + for my $second ('--latency-percentiles', '--latency-percentiles=95') + { + push @options, + [ + "repeated latency percentile option: $first $second", + "$first $second", + [qr{--latency-percentiles can only be specified once}] + ]; + } +} + for my $o (@options) { my ($name, $opts, $err_checks) = @$o; @@ -233,6 +279,17 @@ for my $o (@options) 'pgbench option error: ' . $name); } +# The helper splits on whitespace, so pass these arguments directly. +for my $value ('50 ', '50 ,95') +{ + command_checks_all( + [ 'pgbench', "--latency-percentiles=$value" ], + 1, + [qr{^$}], + [qr{invalid latency percentile list:}], + "latency percentiles reject whitespace after a number: $value"); +} + program_help_ok('pgbench'); program_version_ok('pgbench'); program_options_handling_ok('pgbench'); -- 2.43.0