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))
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.
A more refined solution, such as using string padding, might be ideal
for cases like this.
Best,
Yushi Ogiwara
Attachments:
fix_leftover_character.difftext/x-diff; name=fix_leftover_character.diffDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e658d060ad..be984e0c7f 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -5004,7 +5004,7 @@ 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) %c",
j, total,
(int) ((j * 100) / total),
table, elapsed_sec, remaining_sec, eol);
@@ -5018,7 +5018,7 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
/* have we reached the next interval (or end)? */
if ((j == total) || (elapsed_sec >= log_interval * LOG_STEP_SECONDS))
{
- 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) %c",
j, total,
(int) ((j * 100) / total),
table, elapsed_sec, remaining_sec, eol);
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
Attachments:
pgbench_extra_braces.patchtext/x-patch; charset=us-asciiDownload
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
On 2024/11/07 10:41, Tatsuo Ishii wrote:
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.
+1
+ if (previous_chars != 0)
+ {
+ int n_spaces = chars - previous_chars;
+ fprintf(stderr, "%*s", n_spaces, "");
+ chars += n_spaces;
+ }Currently, this is added only when use_quiet is false, but shouldn’t it also apply when use_quiet is true?
Also, what happens if chars is smaller than previous_chars?
I’m unsure why chars needs to be incremented by n_spaces.
I’ve created an updated patch based on your idea—could you take a look?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachments:
v3-0001-pgbench-Ensure-previous-progress-message-is-fully.patchtext/plain; charset=UTF-8; name=v3-0001-pgbench-Ensure-previous-progress-message-is-fully.patchDownload
From 7f0d10e9c2c65527fbb01acd777366d00596e72c Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Fri, 8 Nov 2024 00:34:55 +0900
Subject: [PATCH v3] pgbench: Ensure previous progress message is fully cleared
when updating.
During pgbench's table initialization, progress updates could display
leftover characters from the previous message if the new message
was shorter. This commit resolves the issue by appending spaces to
the current message to fully overwrite any remaining characters from
the previous line.
Back-patch to all the supported versions.
Author: Yushi Ogiwara, Tatsuo Ishii, Fujii Masao
Reviewed-by: Tatsuo Ishii, Fujii Masao
Discussion: https://postgr.es/m/9a9b8b95b6a709877ae48ad5b0c59bb9@oss.nttdata.com
---
src/bin/pgbench/pgbench.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ba247f3f85..19a82244b5 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 prev_chars = 0;
PGresult *res;
PQExpBufferData sql;
char copy_statement[256];
@@ -5004,10 +5005,10 @@ 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);
}
/* let's not call the timing for each row, but only each 100 rows */
else if (use_quiet && (j % 100 == 0))
@@ -5018,15 +5019,25 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
/* have we reached the next interval (or end)? */
if ((j == total) || (elapsed_sec >= log_interval * LOG_STEP_SECONDS))
{
- 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);
/* skip to the next interval */
log_interval = (int) ceil(elapsed_sec / LOG_STEP_SECONDS);
}
}
+
+ /*
+ * If the previous progress message is longer than the current one,
+ * add spaces to the current line to fully overwrite any remaining
+ * characters from the previous message.
+ */
+ if (prev_chars > chars)
+ fprintf(stderr, "%*c", prev_chars - chars, ' ');
+ fputc(eol, stderr);
+ prev_chars = chars;
}
if (chars != 0 && eol != '\n')
--
2.47.0
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.+1
+ if (previous_chars != 0) + { + int n_spaces = chars - previous_chars; + fprintf(stderr, "%*s", n_spaces, ""); + chars += n_spaces; + }Currently, this is added only when use_quiet is false, but shouldn’t
it also apply when use_quiet is true?
Yes. The patch just showed my idea and I thought I needed to apply the
case when use_quiet is true. But the fix should be applied to after
"else if (use_quiet && (j % 100 == 0))" as shown in your patch.
Also, what happens if chars is smaller than previous_chars?
My oversight. Better to check if chars is smaller than previous_chars.
I’m unsure why chars needs to be incremented by n_spaces.
Yeah, it's not necessary.
I’ve created an updated patch based on your idea—could you take a
look?
Sure.
I think you need to adjust
fprintf(stderr, "%*c\r", chars - 1, ' '); /* Clear the current line */
to:
fprintf(stderr, "%*c\r", chars, ' '); /* Clear the current line */
since now chars does not consider the EOL char. By clearing chars - 1,
the closing parenthesis will be left on the screen.
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/08 11:47, Tatsuo Ishii wrote:
I think you need to adjust
fprintf(stderr, "%*c\r", chars - 1, ' '); /* Clear the current line */
to:
fprintf(stderr, "%*c\r", chars, ' '); /* Clear the current line */
since now chars does not consider the EOL char. By clearing chars - 1,
the closing parenthesis will be left on the screen.
You're right! I've updated the patch and attached v4.
About the back-patch: initially, there were concerns it might be too invasive,
as you told upthread. However, after our discussion, I think the latest version
is straightforward enough, and I'm okay with back-patching it. Thoughts?
Since the minor release freeze is already in effect, if we commit this patch,
we'll need to wait until the next minor releases are out next week before
the commit and back-patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachments:
v4-0001-pgbench-Ensure-previous-progress-message-is-fully.patchtext/plain; charset=UTF-8; name=v4-0001-pgbench-Ensure-previous-progress-message-is-fully.patchDownload
From b55547a5db704a24dd6d2f8d01918f420626705c Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Fri, 8 Nov 2024 00:34:55 +0900
Subject: [PATCH v4] pgbench: Ensure previous progress message is fully cleared
when updating.
During pgbench's table initialization, progress updates could display
leftover characters from the previous message if the new message
was shorter. This commit resolves the issue by appending spaces to
the current message to fully overwrite any remaining characters from
the previous line.
Back-patch to all the supported versions.
Author: Yushi Ogiwara, Tatsuo Ishii, Fujii Masao
Reviewed-by: Tatsuo Ishii, Fujii Masao
Discussion: https://postgr.es/m/9a9b8b95b6a709877ae48ad5b0c59bb9@oss.nttdata.com
---
src/bin/pgbench/pgbench.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ba247f3f85..85e7b68baa 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 prev_chars = 0;
PGresult *res;
PQExpBufferData sql;
char copy_statement[256];
@@ -5004,10 +5005,10 @@ 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);
}
/* let's not call the timing for each row, but only each 100 rows */
else if (use_quiet && (j % 100 == 0))
@@ -5018,19 +5019,29 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
/* have we reached the next interval (or end)? */
if ((j == total) || (elapsed_sec >= log_interval * LOG_STEP_SECONDS))
{
- 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);
/* skip to the next interval */
log_interval = (int) ceil(elapsed_sec / LOG_STEP_SECONDS);
}
}
+
+ /*
+ * If the previous progress message is longer than the current one,
+ * add spaces to the current line to fully overwrite any remaining
+ * characters from the previous message.
+ */
+ if (prev_chars > chars)
+ fprintf(stderr, "%*c", prev_chars - chars, ' ');
+ fputc(eol, stderr);
+ prev_chars = chars;
}
if (chars != 0 && eol != '\n')
- fprintf(stderr, "%*c\r", chars - 1, ' '); /* Clear the current line */
+ fprintf(stderr, "%*c\r", chars, ' '); /* Clear the current line */
if (PQputline(con, "\\.\n"))
pg_fatal("very last PQputline failed");
--
2.47.0
On 2024/11/08 11:47, Tatsuo Ishii wrote:
I think you need to adjust
fprintf(stderr, "%*c\r", chars - 1, ' '); /* Clear the current line */
to:
fprintf(stderr, "%*c\r", chars, ' '); /* Clear the current line */
since now chars does not consider the EOL char. By clearing chars - 1,
the closing parenthesis will be left on the screen.You're right! I've updated the patch and attached v4.
V4 patch looks good to me.
About the back-patch: initially, there were concerns it might be too
invasive,
as you told upthread. However, after our discussion, I think the
latest version
is straightforward enough, and I'm okay with back-patching
it. Thoughts?
I agree. Now the patch is small enough to back-patch.
Since the minor release freeze is already in effect, if we commit this
patch,
we'll need to wait until the next minor releases are out next week
before
the commit and back-patch.
Got it.
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Hi Fujii-san,
On 2024/11/08 11:47, Tatsuo Ishii wrote:
I think you need to adjust
fprintf(stderr, "%*c\r", chars - 1, ' '); /* Clear the current line */
to:
fprintf(stderr, "%*c\r", chars, ' '); /* Clear the current line */
since now chars does not consider the EOL char. By clearing chars - 1,
the closing parenthesis will be left on the screen.You're right! I've updated the patch and attached v4.
About the back-patch: initially, there were concerns it might be too
invasive,
as you told upthread. However, after our discussion, I think the
latest version
is straightforward enough, and I'm okay with back-patching
it. Thoughts?Since the minor release freeze is already in effect, if we commit this
patch,
we'll need to wait until the next minor releases are out next week
before
the commit and back-patch.
Now that two minor releases are out, are you going to commit and
back-patch this?
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
On 2024/11/25 8:31, Tatsuo Ishii wrote:
Now that two minor releases are out, are you going to commit and
back-patch this?
Yes, I will.
But, the patch didn't apply cleanly to some back branches, so I've created
and attached updated patches for them. Could you review these?
If they look good, I'll proceed with pushing them.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachments:
v5-0001-pgbench-Ensure-previous-progress-message-is-fully-pg13-14.patchtext/plain; charset=UTF-8; name=v5-0001-pgbench-Ensure-previous-progress-message-is-fully-pg13-14.patchDownload
From 4e542d9a070943c634c8f64f5619545c90d1c0e4 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Tue, 26 Nov 2024 01:01:14 +0900
Subject: [PATCH v4] pgbench: Ensure previous progress message is fully cleared
when updating.
During pgbench's table initialization, progress updates could display
leftover characters from the previous message if the new message
was shorter. This commit resolves the issue by appending spaces to
the current message to fully overwrite any remaining characters from
the previous line.
Back-patch to all the supported versions.
Author: Yushi Ogiwara, Tatsuo Ishii, Fujii Masao
Reviewed-by: Tatsuo Ishii, Fujii Masao
Discussion: https://postgr.es/m/9a9b8b95b6a709877ae48ad5b0c59bb9@oss.nttdata.com
---
src/bin/pgbench/pgbench.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index c1d577a959..c7f8431841 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4222,6 +4222,8 @@ initGenerateDataClientSide(PGconn *con)
PGresult *res;
int i;
int64 k;
+ int chars = 0;
+ int prev_chars = 0;
/* used to track elapsed time and estimate of the remaining time */
pg_time_usec_t start;
@@ -4304,10 +4306,10 @@ initGenerateDataClientSide(PGconn *con)
double elapsed_sec = PG_TIME_GET_DOUBLE(pg_time_now() - start);
double remaining_sec = ((double) scale * naccounts - j) * elapsed_sec / j;
- fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) done (elapsed %.2f s, remaining %.2f s)%c",
- j, (int64) naccounts * scale,
- (int) (((int64) j * 100) / (naccounts * (int64) scale)),
- elapsed_sec, remaining_sec, eol);
+ chars = fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) done (elapsed %.2f s, remaining %.2f s)",
+ j, (int64) naccounts * scale,
+ (int) (((int64) j * 100) / (naccounts * (int64) scale)),
+ elapsed_sec, remaining_sec);
}
/* let's not call the timing for each row, but only each 100 rows */
else if (use_quiet && (j % 100 == 0))
@@ -4318,14 +4320,24 @@ initGenerateDataClientSide(PGconn *con)
/* have we reached the next interval (or end)? */
if ((j == scale * naccounts) || (elapsed_sec >= log_interval * LOG_STEP_SECONDS))
{
- fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) done (elapsed %.2f s, remaining %.2f s)%c",
- j, (int64) naccounts * scale,
- (int) (((int64) j * 100) / (naccounts * (int64) scale)), elapsed_sec, remaining_sec, eol);
+ chars = fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) done (elapsed %.2f s, remaining %.2f s)",
+ j, (int64) naccounts * scale,
+ (int) (((int64) j * 100) / (naccounts * (int64) scale)), elapsed_sec, remaining_sec);
/* skip to the next interval */
log_interval = (int) ceil(elapsed_sec / LOG_STEP_SECONDS);
}
}
+
+ /*
+ * If the previous progress message is longer than the current one,
+ * add spaces to the current line to fully overwrite any remaining
+ * characters from the previous message.
+ */
+ if (prev_chars > chars)
+ fprintf(stderr, "%*c", prev_chars - chars, ' ');
+ fputc(eol, stderr);
+ prev_chars = chars;
}
if (eol != '\n')
--
2.47.0
v5-0001-pgbench-Ensure-previous-progress-message-is-fully-pg15-16.patchtext/plain; charset=UTF-8; name=v5-0001-pgbench-Ensure-previous-progress-message-is-fully-pg15-16.patchDownload
From 24531a50a286005f4409e0b856fbd8204d00ebd0 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Tue, 26 Nov 2024 00:47:27 +0900
Subject: [PATCH v5] pgbench: Ensure previous progress message is fully cleared
when updating.
During pgbench's table initialization, progress updates could display
leftover characters from the previous message if the new message
was shorter. This commit resolves the issue by appending spaces to
the current message to fully overwrite any remaining characters from
the previous line.
Back-patch to all the supported versions.
Author: Yushi Ogiwara, Tatsuo Ishii, Fujii Masao
Reviewed-by: Tatsuo Ishii, Fujii Masao
Discussion: https://postgr.es/m/9a9b8b95b6a709877ae48ad5b0c59bb9@oss.nttdata.com
---
src/bin/pgbench/pgbench.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index c1134eae5b..fd2c8f42c5 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4890,6 +4890,8 @@ initGenerateDataClientSide(PGconn *con)
PGresult *res;
int i;
int64 k;
+ int chars = 0;
+ int prev_chars = 0;
char *copy_statement;
/* used to track elapsed time and estimate of the remaining time */
@@ -4975,10 +4977,10 @@ initGenerateDataClientSide(PGconn *con)
double elapsed_sec = PG_TIME_GET_DOUBLE(pg_time_now() - start);
double remaining_sec = ((double) scale * naccounts - j) * elapsed_sec / j;
- fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) done (elapsed %.2f s, remaining %.2f s)%c",
- j, (int64) naccounts * scale,
- (int) (((int64) j * 100) / (naccounts * (int64) scale)),
- elapsed_sec, remaining_sec, eol);
+ chars = fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) done (elapsed %.2f s, remaining %.2f s)",
+ j, (int64) naccounts * scale,
+ (int) (((int64) j * 100) / (naccounts * (int64) scale)),
+ elapsed_sec, remaining_sec);
}
/* let's not call the timing for each row, but only each 100 rows */
else if (use_quiet && (j % 100 == 0))
@@ -4989,14 +4991,24 @@ initGenerateDataClientSide(PGconn *con)
/* have we reached the next interval (or end)? */
if ((j == scale * naccounts) || (elapsed_sec >= log_interval * LOG_STEP_SECONDS))
{
- fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) done (elapsed %.2f s, remaining %.2f s)%c",
- j, (int64) naccounts * scale,
- (int) (((int64) j * 100) / (naccounts * (int64) scale)), elapsed_sec, remaining_sec, eol);
+ chars = fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) done (elapsed %.2f s, remaining %.2f s)",
+ j, (int64) naccounts * scale,
+ (int) (((int64) j * 100) / (naccounts * (int64) scale)), elapsed_sec, remaining_sec);
/* skip to the next interval */
log_interval = (int) ceil(elapsed_sec / LOG_STEP_SECONDS);
}
}
+
+ /*
+ * If the previous progress message is longer than the current one,
+ * add spaces to the current line to fully overwrite any remaining
+ * characters from the previous message.
+ */
+ if (prev_chars > chars)
+ fprintf(stderr, "%*c", prev_chars - chars, ' ');
+ fputc(eol, stderr);
+ prev_chars = chars;
}
if (eol != '\n')
--
2.47.0
v5-0001-pgbench-Ensure-previous-progress-message-is-fully-pg17-master.patchtext/plain; charset=UTF-8; name=v5-0001-pgbench-Ensure-previous-progress-message-is-fully-pg17-master.patchDownload
From b55547a5db704a24dd6d2f8d01918f420626705c Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Fri, 8 Nov 2024 00:34:55 +0900
Subject: [PATCH v4] pgbench: Ensure previous progress message is fully cleared
when updating.
During pgbench's table initialization, progress updates could display
leftover characters from the previous message if the new message
was shorter. This commit resolves the issue by appending spaces to
the current message to fully overwrite any remaining characters from
the previous line.
Back-patch to all the supported versions.
Author: Yushi Ogiwara, Tatsuo Ishii, Fujii Masao
Reviewed-by: Tatsuo Ishii, Fujii Masao
Discussion: https://postgr.es/m/9a9b8b95b6a709877ae48ad5b0c59bb9@oss.nttdata.com
---
src/bin/pgbench/pgbench.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ba247f3f85..85e7b68baa 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 prev_chars = 0;
PGresult *res;
PQExpBufferData sql;
char copy_statement[256];
@@ -5004,10 +5005,10 @@ 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);
}
/* let's not call the timing for each row, but only each 100 rows */
else if (use_quiet && (j % 100 == 0))
@@ -5018,19 +5019,29 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
/* have we reached the next interval (or end)? */
if ((j == total) || (elapsed_sec >= log_interval * LOG_STEP_SECONDS))
{
- 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);
/* skip to the next interval */
log_interval = (int) ceil(elapsed_sec / LOG_STEP_SECONDS);
}
}
+
+ /*
+ * If the previous progress message is longer than the current one,
+ * add spaces to the current line to fully overwrite any remaining
+ * characters from the previous message.
+ */
+ if (prev_chars > chars)
+ fprintf(stderr, "%*c", prev_chars - chars, ' ');
+ fputc(eol, stderr);
+ prev_chars = chars;
}
if (chars != 0 && eol != '\n')
- fprintf(stderr, "%*c\r", chars - 1, ' '); /* Clear the current line */
+ fprintf(stderr, "%*c\r", chars, ' '); /* Clear the current line */
if (PQputline(con, "\\.\n"))
pg_fatal("very last PQputline failed");
--
2.47.0
Yes, I will.
But, the patch didn't apply cleanly to some back branches, so I've
created
and attached updated patches for them. Could you review these?
If they look good, I'll proceed with pushing them.
Sure. The patch for master to v15 look good to me. Unfortunately
v5-0001-pgbench-Ensure-previous-progress-message-is-fully-pg13-14.patch
applies to v14 but does not apply to v13.
$ git checkout REL_13_STABLE
Switched to branch 'REL_13_STABLE'
Your branch is up to date with 'origin/REL_13_STABLE'.
$ git apply ~/v5-0001-pgbench-Ensure-previous-progress-message-is-fully-pg13-14.patch
error: patch failed: src/bin/pgbench/pgbench.c:4222
error: src/bin/pgbench/pgbench.c: patch does not apply
So I created a patch for pg13 using patch command. Attached is the
patch for pg13.
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
pgbench_v13.patchtext/x-patch; charset=us-asciiDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 6ea89cabc9..6bccacaf86 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3840,6 +3840,8 @@ initGenerateDataClientSide(PGconn *con)
PGresult *res;
int i;
int64 k;
+ int chars = 0;
+ int prev_chars = 0;
/* used to track elapsed time and estimate of the remaining time */
instr_time start,
@@ -3926,10 +3928,10 @@ initGenerateDataClientSide(PGconn *con)
elapsed_sec = INSTR_TIME_GET_DOUBLE(diff);
remaining_sec = ((double) scale * naccounts - j) * elapsed_sec / j;
- fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) done (elapsed %.2f s, remaining %.2f s)%c",
- j, (int64) naccounts * scale,
- (int) (((int64) j * 100) / (naccounts * (int64) scale)),
- elapsed_sec, remaining_sec, eol);
+ chars = fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) done (elapsed %.2f s, remaining %.2f s)",
+ j, (int64) naccounts * scale,
+ (int) (((int64) j * 100) / (naccounts * (int64) scale)),
+ elapsed_sec, remaining_sec);
}
/* let's not call the timing for each row, but only each 100 rows */
else if (use_quiet && (j % 100 == 0))
@@ -3943,14 +3945,24 @@ initGenerateDataClientSide(PGconn *con)
/* have we reached the next interval (or end)? */
if ((j == scale * naccounts) || (elapsed_sec >= log_interval * LOG_STEP_SECONDS))
{
- fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) done (elapsed %.2f s, remaining %.2f s)%c",
- j, (int64) naccounts * scale,
- (int) (((int64) j * 100) / (naccounts * (int64) scale)), elapsed_sec, remaining_sec, eol);
+ chars = fprintf(stderr, INT64_FORMAT " of " INT64_FORMAT " tuples (%d%%) done (elapsed %.2f s, remaining %.2f s)",
+ j, (int64) naccounts * scale,
+ (int) (((int64) j * 100) / (naccounts * (int64) scale)), elapsed_sec, remaining_sec);
/* skip to the next interval */
log_interval = (int) ceil(elapsed_sec / LOG_STEP_SECONDS);
}
}
+
+ /*
+ * If the previous progress message is longer than the current one,
+ * add spaces to the current line to fully overwrite any remaining
+ * characters from the previous message.
+ */
+ if (prev_chars > chars)
+ fprintf(stderr, "%*c", prev_chars - chars, ' ');
+ fputc(eol, stderr);
+ prev_chars = chars;
}
if (eol != '\n')
On 2024/11/27 18:41, Tatsuo Ishii wrote:
Yes, I will.
But, the patch didn't apply cleanly to some back branches, so I've
created
and attached updated patches for them. Could you review these?
If they look good, I'll proceed with pushing them.Sure. The patch for master to v15 look good to me. Unfortunately
v5-0001-pgbench-Ensure-previous-progress-message-is-fully-pg13-14.patch
applies to v14 but does not apply to v13.$ git checkout REL_13_STABLE
Switched to branch 'REL_13_STABLE'
Your branch is up to date with 'origin/REL_13_STABLE'.
$ git apply ~/v5-0001-pgbench-Ensure-previous-progress-message-is-fully-pg13-14.patch
error: patch failed: src/bin/pgbench/pgbench.c:4222
error: src/bin/pgbench/pgbench.c: patch does not applySo I created a patch for pg13 using patch command. Attached is the
patch for pg13.
I was applying the patch to v13 by first applying it to v14 and then using
git cherry-pick for v13, which worked fine. Anyway, thank you for providing
the patch for v13! I've now pushed the patches.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Sure. The patch for master to v15 look good to me. Unfortunately
v5-0001-pgbench-Ensure-previous-progress-message-is-fully-pg13-14.patch
applies to v14 but does not apply to v13.
$ git checkout REL_13_STABLE
Switched to branch 'REL_13_STABLE'
Your branch is up to date with 'origin/REL_13_STABLE'.
$ git apply
~/v5-0001-pgbench-Ensure-previous-progress-message-is-fully-pg13-14.patch
error: patch failed: src/bin/pgbench/pgbench.c:4222
error: src/bin/pgbench/pgbench.c: patch does not apply
So I created a patch for pg13 using patch command. Attached is the
patch for pg13.I was applying the patch to v13 by first applying it to v14 and then
using
git cherry-pick for v13, which worked fine. Anyway, thank you for
providing
the patch for v13! I've now pushed the patches.
Yeah, maybe git apply is not smart enough.
Thank you for pushing the patches!
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
On Thu, Nov 28, 2024 at 07:45:09AM +0900, Tatsuo Ishii wrote:
Yeah, maybe git apply is not smart enough.
`git apply` is more picky than a simple `patch -p1` as it checks more
context around the patch and is more likely to reject some input.
When the former does not work correctly, I personally just use the
latter, dealing with file additions and removals manually.
--
Michael
Hi,
On 2024-11-26 01:32:10 +0900, Fujii Masao wrote:
On 2024/11/25 8:31, Tatsuo Ishii wrote:
Now that two minor releases are out, are you going to commit and
back-patch this?Yes, I will.
But, the patch didn't apply cleanly to some back branches, so I've created
and attached updated patches for them. Could you review these?
If they look good, I'll proceed with pushing them.
I just did pgbench -i 100 -q via ssh and noticed it was *way* slower than I
expected. Did it with debian's pgbench, no such issue.
It's due to this patch.
/srv/dev/build/m-opt/src/bin/pgbench/pgbench -i -s 10 -Idtg -h /tmp -q > /tmp/pgiu 2>&1
With HEAD:
pgbench -i -s 10 -Idtg -h /tmp -q 2>&1|wc
1000114 52 1000448
With af35fe501af reverted:
pgbench -i -s 10 -Idtg -h /tmp -q 2>&1|wc
6 52 340
Outputting that many lines to the terminal causes noticeable slowdowns even
locally and make the terminal use a *lot* more cpu cycles.
With HEAD:
perf stat -p $(pidof gnome-terminal-server) /srv/dev/build/m-opt/src/bin/pgbench/pgbench -i -s 100 -Idtg -h /tmp -q
dropping old tables...
creating tables...
generating data (client-side)...
done in 6.31 s (drop tables 0.02 s, create tables 0.00 s, client-side generate 6.28 s).
Performance counter stats for process id '3615':
3,726.37 msec task-clock # 0.590 CPUs utilized
692,360 context-switches # 185.800 K/sec
49 cpu-migrations # 13.150 /sec
2 page-faults # 0.537 /sec
13,340,182,520 instructions # 1.35 insn per cycle
# 0.14 stalled cycles per insn
9,893,907,904 cycles # 2.655 GHz
1,913,148,556 stalled-cycles-frontend # 19.34% frontend cycles idle
2,935,132,872 branches # 787.665 M/sec
17,293,477 branch-misses # 0.59% of all branches
6.315407944 seconds time elapsed
With af35fe501af reverted:
perf stat -p $(pidof gnome-terminal-server) /srv/dev/build/m-opt/src/bin/pgbench/pgbench -i -s 100 -Idtg -h /tmp -q
dropping old tables...
creating tables...
generating data (client-side)...
done in 5.77 s (drop tables 0.02 s, create tables 0.00 s, client-side generate 5.75 s).
Performance counter stats for process id '3615':
228.02 msec task-clock # 0.039 CPUs utilized
239 context-switches # 1.048 K/sec
16 cpu-migrations # 70.170 /sec
0 page-faults # 0.000 /sec
298,383,249 instructions # 0.35 insn per cycle
# 0.28 stalled cycles per insn
857,475,516 cycles # 3.761 GHz
82,918,558 stalled-cycles-frontend # 9.67% frontend cycles idle
48,317,810 branches # 211.903 M/sec
618,525 branch-misses # 1.28% of all branches
5.780547274 seconds time elapsed
IOW, pgbench -i -s 100 -q got ~0.8s slower and made the terminal use 15x more
cycles.
Given the upcoming set of minor releases, I think it may be best for this this
patch ought to be reverted for now.
Greetings,
Andres Freund
On Fri, Feb 07, 2025 at 12:28:16PM -0500, Andres Freund wrote:
I just did pgbench -i 100 -q via ssh and noticed it was *way* slower than I
expected. Did it with debian's pgbench, no such issue.It's due to this patch.
/srv/dev/build/m-opt/src/bin/pgbench/pgbench -i -s 10 -Idtg -h /tmp -q > /tmp/pgiu 2>&1
With HEAD:
pgbench -i -s 10 -Idtg -h /tmp -q 2>&1|wc
1000114 52 1000448With af35fe501af reverted:
pgbench -i -s 10 -Idtg -h /tmp -q 2>&1|wc
6 52 340Outputting that many lines to the terminal causes noticeable slowdowns even
locally and make the terminal use a *lot* more cpu cycles.
Presumably we should only fputc(eol, stderr) when we actually fprintf()
above this point.
Given the upcoming set of minor releases, I think it may be best for this this
patch ought to be reverted for now.
+1, since we're nearing the freeze and this doesn't seem like a
particularly urgent bug fix.
--
nathan
Andres Freund <andres@anarazel.de> writes:
I just did pgbench -i 100 -q via ssh and noticed it was *way* slower than I
expected. Did it with debian's pgbench, no such issue.
It's due to this patch.
Oh! The problem is that the hunk
+ /*
+ * If the previous progress message is longer than the current one,
+ * add spaces to the current line to fully overwrite any remaining
+ * characters from the previous message.
+ */
+ if (prev_chars > chars)
+ fprintf(stderr, "%*c", prev_chars - chars, ' ');
+ fputc(eol, stderr);
+ prev_chars = chars;is executed unconditionally for each data row, when we should only run
it when we printed something. It's kind of invisible if "eol" is \r,
but I can believe that it's driving the terminal nuts. Trying it
here, it also makes the thing practically unresponsive to control-C.
Given the upcoming set of minor releases, I think it may be best for this this
patch ought to be reverted for now.
Seems easy enough to fix. But it's now middle of the night Saturday
morning in Japan, so I doubt Masao-san or Ishii-san will see this
for awhile. And the release freeze is coming up fast.
Let me have a go at fixing it, and if it turns out to be harder
than I think, I'll revert it instead.
regards, tom lane
On Fri, Feb 07, 2025 at 12:58:38PM -0500, Tom Lane wrote:
Let me have a go at fixing it, and if it turns out to be harder
than I think, I'll revert it instead.
Oops, I was already taking a look at this. I figured it'd just be
something like the following, although maybe there's a more elegant way.
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 40592e62606..b73921c36e3 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4990,6 +4990,7 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
for (k = 0; k < total; k++)
{
int64 j = k + 1;
+ bool printed = false; init_row(&sql, k);
if (PQputline(con, sql.data))
@@ -5011,6 +5012,7 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
j, total,
(int) ((j * 100) / total),
table, elapsed_sec, remaining_sec);
+ printed = true;
}
/* let's not call the timing for each row, but only each 100 rows */
else if (use_quiet && (j % 100 == 0))
@@ -5025,6 +5027,7 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
j, total,
(int) ((j * 100) / total),
table, elapsed_sec, remaining_sec);
+ printed = true;
/* skip to the next interval */
log_interval = (int) ceil(elapsed_sec / LOG_STEP_SECONDS);
@@ -5038,7 +5041,8 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
*/
if (prev_chars > chars)
fprintf(stderr, "%*c", prev_chars - chars, ' ');
- fputc(eol, stderr);
+ if (printed)
+ fputc(eol, stderr);
prev_chars = chars;
}
--
nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
On Fri, Feb 07, 2025 at 12:58:38PM -0500, Tom Lane wrote:
Let me have a go at fixing it, and if it turns out to be harder
than I think, I'll revert it instead.
Oops, I was already taking a look at this. I figured it'd just be
something like the following, although maybe there's a more elegant way.
Well, the stuff with prev_chars really ought to be skipped as well.
(Yeah, it's probably a no-op, but readers shouldn't have to figure
that out.)
My thought was that duplicating the logic isn't so bad, as attached.
regards, tom lane
Attachments:
pgbench-fix.patchtext/x-diff; charset=us-ascii; name=pgbench-fix.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 40592e6260..f303bdeec8 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -5011,6 +5011,16 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
j, total,
(int) ((j * 100) / total),
table, elapsed_sec, remaining_sec);
+
+ /*
+ * If the previous progress message is longer than the current
+ * one, add spaces to the current line to fully overwrite any
+ * remaining characters from the previous message.
+ */
+ if (prev_chars > chars)
+ fprintf(stderr, "%*c", prev_chars - chars, ' ');
+ fputc(eol, stderr);
+ prev_chars = chars;
}
/* let's not call the timing for each row, but only each 100 rows */
else if (use_quiet && (j % 100 == 0))
@@ -5026,20 +5036,20 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
(int) ((j * 100) / total),
table, elapsed_sec, remaining_sec);
+ /*
+ * If the previous progress message is longer than the current
+ * one, add spaces to the current line to fully overwrite any
+ * remaining characters from the previous message.
+ */
+ if (prev_chars > chars)
+ fprintf(stderr, "%*c", prev_chars - chars, ' ');
+ fputc(eol, stderr);
+ prev_chars = chars;
+
/* skip to the next interval */
log_interval = (int) ceil(elapsed_sec / LOG_STEP_SECONDS);
}
}
-
- /*
- * If the previous progress message is longer than the current one,
- * add spaces to the current line to fully overwrite any remaining
- * characters from the previous message.
- */
- if (prev_chars > chars)
- fprintf(stderr, "%*c", prev_chars - chars, ' ');
- fputc(eol, stderr);
- prev_chars = chars;
}
if (chars != 0 && eol != '\n')
Hi,
On 2025-02-07 12:58:38 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
I just did pgbench -i 100 -q via ssh and noticed it was *way* slower than I
expected. Did it with debian's pgbench, no such issue.It's due to this patch.
Oh! The problem is that the hunk
+ /* + * If the previous progress message is longer than the current one, + * add spaces to the current line to fully overwrite any remaining + * characters from the previous message. + */ + if (prev_chars > chars) + fprintf(stderr, "%*c", prev_chars - chars, ' '); + fputc(eol, stderr); + prev_chars = chars;is executed unconditionally for each data row, when we should only run
it when we printed something.
Yea, that would do it.
Trying it here, it also makes the thing practically unresponsive to
control-C.
Interestingly I don't see that aspect...
Given the upcoming set of minor releases, I think it may be best for this this
patch ought to be reverted for now.Seems easy enough to fix. But it's now middle of the night Saturday
morning in Japan, so I doubt Masao-san or Ishii-san will see this
for awhile. And the release freeze is coming up fast.Let me have a go at fixing it, and if it turns out to be harder
than I think, I'll revert it instead.
Makes sense!
Greetings,
Andres Freund
On Fri, Feb 07, 2025 at 01:10:04PM -0500, Tom Lane wrote:
Nathan Bossart <nathandbossart@gmail.com> writes:
Oops, I was already taking a look at this. I figured it'd just be
something like the following, although maybe there's a more elegant way.Well, the stuff with prev_chars really ought to be skipped as well.
(Yeah, it's probably a no-op, but readers shouldn't have to figure
that out.)My thought was that duplicating the logic isn't so bad, as attached.
WFM!
--
nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
On Fri, Feb 07, 2025 at 01:10:04PM -0500, Tom Lane wrote:
My thought was that duplicating the logic isn't so bad, as attached.
WFM!
It does fix the problem here, so I'll make it so.
regards, tom lane
Nathan Bossart <nathandbossart@gmail.com> writes:
On Fri, Feb 07, 2025 at 01:10:04PM -0500, Tom Lane wrote:
My thought was that duplicating the logic isn't so bad, as attached.
WFM!
It does fix the problem here, so I'll make it so.
Sorry for the problem and thank you for the fix!
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp