Thread: Re: Fix for Extra Parenthesis in pgbench progress message
> Hi, > > I noticed an issue in the pgbench progress message where an extra > closing parenthesis )) appears, as shown below: > > 7000000 of 10000000 tuples (70%) of pgbench_accounts done (elapsed > 19.75 s, remaining 8.46 s)) Yeah, annoying. > This occurs when running commands like pgbench -i -s100 and is caused > by leftover characters when using \r with fprintf. I made a patch to > address this by adding extra spaces before \r, which clears any > remaining characters. While effective, I recognize this solution may > not be the most sophisticated. The patch works perfectly for the case that there is one extra brace as shown in your example. However I think it will not work if there are two or more extra braces. Best reagards, -- Tatsuo Ishii SRA OSS K.K. English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
On 2024/11/02 20:43, Tatsuo Ishii wrote:
>> Hi,
>>
>> I noticed an issue in the pgbench progress message where an extra
>> closing parenthesis )) appears, as shown below:
>>
>> 7000000 of 10000000 tuples (70%) of pgbench_accounts done (elapsed
>> 19.75 s, remaining 8.46 s))
>
> Yeah, annoying.
>
>> This occurs when running commands like pgbench -i -s100 and is caused
>> by leftover characters when using \r with fprintf. I made a patch to
>> address this by adding extra spaces before \r, which clears any
>> remaining characters. While effective, I recognize this solution may
>> not be the most sophisticated.
>
> The patch works perfectly for the case that there is one extra brace
> as shown in your example. However I think it will not work if there
> are two or more extra braces.
Are you suggesting adding more space characters before the carriage return
in the progress reporting line, like this? Since the line includes both
elapsed and remaining times, its total length doesn’t change much.
I think adding five spaces before the carriage return should be enough.
Thoughts?
- chars = fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) of %s done (elapsed %.2f s,
remaining%.2f s) %c",
+ chars = fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) of %s done (elapsed %.2f s,
remaining%.2f s)%5s%c",
j, total,
(int) ((j * 100) / total),
- table, elapsed_sec, remaining_sec, eol);
+ table, elapsed_sec, remaining_sec, "", eol);
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
>> The patch works perfectly for the case that there is one extra brace >> as shown in your example. However I think it will not work if there >> are two or more extra braces. > > Are you suggesting adding more space characters before the carriage > return > in the progress reporting line, like this? No. I thought about dynamically calculating spaces needed to be printed something like attached patch. > Since the line includes > both > elapsed and remaining times, its total length doesn’t change much. Maybe. But I am also worried about the case when we would want to change the log line format in the future. We might introduce this kind of bug again. By dynamically calculating the number of necessary spaces, we don't need to think about the concern. Best reagards, -- Tatsuo Ishii SRA OSS K.K. English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index ba247f3f85..4c05076dff 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -4944,6 +4944,7 @@ initPopulateTable(PGconn *con, const char *table, int64 base, int n; int64 k; int chars = 0; + int previous_chars = 0; PGresult *res; PQExpBufferData sql; char copy_statement[256]; @@ -5004,10 +5005,18 @@ initPopulateTable(PGconn *con, const char *table, int64 base, double elapsed_sec = PG_TIME_GET_DOUBLE(pg_time_now() - start); double remaining_sec = ((double) total - j) * elapsed_sec / j; - chars = fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) of %s done (elapsed %.2f s, remaining%.2f s)%c", + chars = fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) of %s done (elapsed %.2f s, remaining%.2f s)", j, total, (int) ((j * 100) / total), - table, elapsed_sec, remaining_sec, eol); + table, elapsed_sec, remaining_sec); + if (previous_chars != 0) + { + int n_spaces = chars - previous_chars; + fprintf(stderr, "%*s", n_spaces, ""); + chars += n_spaces; + } + fprintf(stderr, "%c", eol); + previous_chars = chars; } /* let's not call the timing for each row, but only each 100 rows */ else if (use_quiet && (j % 100 == 0))
>>> The patch works perfectly for the case that there is one extra brace >>> as shown in your example. However I think it will not work if there >>> are two or more extra braces. >> >> Are you suggesting adding more space characters before the carriage >> return >> in the progress reporting line, like this? > > No. I thought about dynamically calculating spaces needed to be > printed something like attached patch. > >> Since the line includes >> both >> elapsed and remaining times, its total length doesn’t change much. > > Maybe. But I am also worried about the case when we would want to > change the log line format in the future. We might introduce this kind > of bug again. By dynamically calculating the number of necessary > spaces, we don't need to think about the concern. Probably my patch is too invasive and not worth the trouble for the stable branches. What about back patching your patch to the stable stable branches, and applying my patch to the master branch? Best reagards, -- Tatsuo Ishii SRA OSS K.K. English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp