Thread: Typo in pgbench messages.
Hi,
I found messages inserted a space before the "%" in pgbench.
I think this is typo because there are no space before the "%" in other messages.
What do you think?
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index f166a77e3a..4ebe5e6ea4 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -5598,11 +5598,11 @@ printResults(StatsData *total,
return;
if (throttle_delay && latency_limit)
- printf("number of transactions skipped: " INT64_FORMAT " (%.3f %%)\n",
+ printf("number of transactions skipped: " INT64_FORMAT " (%.3f%%)\n",
total->skipped, 100.0 * total->skipped / total->cnt);
if (latency_limit)
- printf("number of transactions above the %.1f ms latency limit: " INT64_FORMAT "/" INT64_FORMAT " (%.3f
%%)\n",
+ printf("number of transactions above the %.1f ms latency limit: " INT64_FORMAT "/" INT64_FORMAT "
(%.3f%%)\n",
latency_limit / 1000.0, latency_late, ntx,
(ntx > 0) ? 100.0 * latency_late / ntx : 0.0);
--
KAWAMOTO Masaya <kawamoto@sraoss.co.jp>
SRA OSS, Inc. Japan
> Hi,
>
> I found messages inserted a space before the "%" in pgbench.
> I think this is typo because there are no space before the "%" in other messages.
> What do you think?
I think you are right. In English there's should be no space between number and "%".
AFAIK other parts of PostgreSQL follow the rule.
> diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
> index f166a77e3a..4ebe5e6ea4 100644
> --- a/src/bin/pgbench/pgbench.c
> +++ b/src/bin/pgbench/pgbench.c
> @@ -5598,11 +5598,11 @@ printResults(StatsData *total,
> return;
>
> if (throttle_delay && latency_limit)
> - printf("number of transactions skipped: " INT64_FORMAT " (%.3f %%)\n",
> + printf("number of transactions skipped: " INT64_FORMAT " (%.3f%%)\n",
> total->skipped, 100.0 * total->skipped / total->cnt);
>
> if (latency_limit)
> - printf("number of transactions above the %.1f ms latency limit: " INT64_FORMAT "/" INT64_FORMAT " (%.3f
%%)\n",
> + printf("number of transactions above the %.1f ms latency limit: " INT64_FORMAT "/" INT64_FORMAT "
(%.3f%%)\n",
> latency_limit / 1000.0, latency_late, ntx,
> (ntx > 0) ? 100.0 * latency_late / ntx : 0.0);
Looks good to me.
Best reagards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
Thanks for your comment!
Sorry, I did not attach the patch file.
This patch focas on master branch.
Best regards,
On Thu, 24 Feb 2022 12:15:55 +0900 (JST)
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
> > Hi,
> >
> > I found messages inserted a space before the "%" in pgbench.
> > I think this is typo because there are no space before the "%" in other messages.
> > What do you think?
>
> I think you are right. In English there's should be no space between number and "%".
> AFAIK other parts of PostgreSQL follow the rule.
>
> > diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
> > index f166a77e3a..4ebe5e6ea4 100644
> > --- a/src/bin/pgbench/pgbench.c
> > +++ b/src/bin/pgbench/pgbench.c
> > @@ -5598,11 +5598,11 @@ printResults(StatsData *total,
> > return;
> >
> > if (throttle_delay && latency_limit)
> > - printf("number of transactions skipped: " INT64_FORMAT " (%.3f %%)\n",
> > + printf("number of transactions skipped: " INT64_FORMAT " (%.3f%%)\n",
> > total->skipped, 100.0 * total->skipped / total->cnt);
> >
> > if (latency_limit)
> > - printf("number of transactions above the %.1f ms latency limit: " INT64_FORMAT "/" INT64_FORMAT " (%.3f
%%)\n",
> > + printf("number of transactions above the %.1f ms latency limit: " INT64_FORMAT "/" INT64_FORMAT "
(%.3f%%)\n",
> > latency_limit / 1000.0, latency_late, ntx,
> > (ntx > 0) ? 100.0 * latency_late / ntx : 0.0);
>
> Looks good to me.
>
> Best reagards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp
>
>
--
KAWAMOTO Masaya <kawamoto@sraoss.co.jp>
SRA OSS, Inc. Japan
Attachment
>> I think you are right. In English there's should be no space between number and "%". >> AFAIK other parts of PostgreSQL follow the rule. I think it's better to back-patch this to stable branches if there's no objection. Thought? Best reagards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Thu, Feb 24, 2022 at 06:38:57PM +0900, Tatsuo Ishii wrote: > >> I think you are right. In English there's should be no space between number and "%". > >> AFAIK other parts of PostgreSQL follow the rule. > > I think it's better to back-patch this to stable branches if there's > no objection. Thought? That's only cosmetic, so I would just bother about HEAD if I were to change something like that (I would not bother at all, personally). One argument against a backpatch is that this would be disruptive with tools that parse and analyze the output generated by pgbench. Fabien, don't you have some tools and/or wrappers doing exactly that? -- Michael
Attachment
Bonjour Michaël,
>> I think it's better to back-patch this to stable branches if there's
>> no objection. Thought?
>
> That's only cosmetic, so I would just bother about HEAD if I were to
> change something like that (I would not bother at all, personally).
>
> One argument against a backpatch is that this would be disruptive with
> tools that parse and analyze the output generated by pgbench. Fabien,
> don't you have some tools and/or wrappers doing exactly that?
Yep, I like to "| cut -d' ' -fx" and other "line.split(' ')" or whatever.
I think that the break of typographical rules is intentional to allow such
simplistic low-level stream handling through pipes or scripts. I'd prefer
that the format is not changed. Maybe a comment could be added to explain
the reason behind it.
--
Fabien.
> On 24 Feb 2022, at 13:58, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>> One argument against a backpatch is that this would be disruptive with
>> tools that parse and analyze the output generated by pgbench. Fabien,
>> don't you have some tools and/or wrappers doing exactly that?
>
> Yep, I like to "| cut -d' ' -fx" and other "line.split(' ')" or whatever.
>
> I think that the break of typographical rules is intentional to allow such simplistic low-level stream handling
throughpipes or scripts. I'd prefer that the format is not changed. Maybe a comment could be added to explain the
reasonbehind it.
That doesn't sound like an overwhelmingly convincing argument to print some
messages with 'X %' and others with 'X%'.
--
Daniel Gustafsson https://vmware.com/
On Thu, 24 Feb 2022 14:44:05 +0100
Daniel Gustafsson <daniel@yesql.se> wrote:
> > On 24 Feb 2022, at 13:58, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
> >> One argument against a backpatch is that this would be disruptive with
> >> tools that parse and analyze the output generated by pgbench. Fabien,
> >> don't you have some tools and/or wrappers doing exactly that?
> >
> > Yep, I like to "| cut -d' ' -fx" and other "line.split(' ')" or whatever.
> >
> > I think that the break of typographical rules is intentional to allow such simplistic low-level stream handling
throughpipes or scripts. I'd prefer that the format is not changed. Maybe a comment could be added to explain the
reasonbehind it.
>
> That doesn't sound like an overwhelmingly convincing argument to print some
> messages with 'X %' and others with 'X%'.
I agree with Daniel. If inserting a space in front of % was intentional for
handling the result in such tools, we should fix other places where 'X%' is
used in the pgbench output.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Hello Daniel, >> I think that the break of typographical rules is intentional to allow >> such simplistic low-level stream handling through pipes or scripts. I'd >> prefer that the format is not changed. Maybe a comment could be added >> to explain the reason behind it. > > That doesn't sound like an overwhelmingly convincing argument to print > some messages with 'X %' and others with 'X%'. Indeed. The no-space % are for database loading progress and the final report which I happen not to process through pipes and are more user-facing interfaces/reports. The stream piping need applies more to repeated lines such as those output by progress, which happen to avoid percentages anyway so the questions does not arise. So fine with me wrt having a more homogeneous report. -- Fabien.
Hi Fabien, >> That doesn't sound like an overwhelmingly convincing argument to print >> some messages with 'X %' and others with 'X%'. > > Indeed. The no-space % are for database loading progress and the final > report which I happen not to process through pipes and are more > user-facing interfaces/reports. The stream piping need applies more to > repeated lines such as those output by progress, which happen to avoid > percentages anyway so the questions does not arise. > > So fine with me wrt having a more homogeneous report. So are you fine with Kawamoto-san's patch? Best reagards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
>> So fine with me wrt having a more homogeneous report. > > So are you fine with Kawamoto-san's patch? Yes. Patch applies cleanly (hmmm, it would have been better to have it as an attachement). Make & make check ok. Fine with me. -- Fabien.
>> So are you fine with Kawamoto-san's patch? > > Yes. > > Patch applies cleanly (hmmm, it would have been better to have it as > an attachement). Make & make check ok. Fine with me. Thank you for the review. Fix pushed to master branch. Best reagards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp