pgbench -i progress output on terminal
Hi,
I wonder why we don't use the same style for $subject as pg_basebackup
--progress, that is, use a carriage return instead of a newline after
each line reporting the number of tuples copied?
Attached patch for that.
Thanks,
Amit
Attachments:
compactify-pgbench-init-progress-output.patchapplication/octet-stream; name=compactify-pgbench-init-progress-output.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4a7ac1f821..d0f2cadaaf 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3906,7 +3906,7 @@ 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)\n",
+ 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);
@@ -3923,7 +3923,7 @@ 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)\n",
+ 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);
@@ -3932,7 +3932,13 @@ initGenerateDataClientSide(PGconn *con)
}
}
+ /* Stay on the same line if reporting to a terminal */
+ fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
}
+
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+
if (PQputline(con, "\\.\n"))
{
fprintf(stderr, "very last PQputline failed\n");
On Thu, Nov 28, 2019 at 10:41:14AM +0900, Amit Langote wrote:
I wonder why we don't use the same style for $subject as pg_basebackup
--progress, that is, use a carriage return instead of a newline after
each line reporting the number of tuples copied?Attached patch for that.
I have not checked your patch in details, but +1 for the change.
--
Michael
Hello Amit,
I wonder why we don't use the same style for $subject as pg_basebackup
--progress, that is, use a carriage return instead of a newline after
each line reporting the number of tuples copied?
Why not.
Attached patch for that.
I'll look into it. Could you add it to the CF app?
--
Fabien.
Hi Fabien,
On Thu, Nov 28, 2019 at 4:35 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Hello Amit,
I wonder why we don't use the same style for $subject as pg_basebackup
--progress, that is, use a carriage return instead of a newline after
each line reporting the number of tuples copied?Why not.
Attached patch for that.
I'll look into it. Could you add it to the CF app?
Great, done.
https://commitfest.postgresql.org/26/2363/
Thanks,
Amit
I wonder why we don't use the same style for $subject as pg_basebackup
--progress, that is, use a carriage return instead of a newline after
each line reporting the number of tuples copied?
Patch applies cleanly, compiles, and works for me.
My 0.02€:
fprintf -> fputs or fputc to avoid a format parsing, or maybe use %c in
the formats.
As the format is not constant, ISTM that vfprintf should be called, not
fprintf (even if in practice fprintf does call vfprintf internally).
I'm not sure what the compilers does with isatty(fileno(stderr)), maybe
the eol could be precomputed:
char eol = isatty(...) ? '\r' : '\n';
and reused afterwards in the loop:
fprintf(stderr, ".... %c", ..., eol);
that would remove the added in-loop printing.
--
Fabien.
Hi Fabien,
On Fri, Nov 29, 2019 at 10:13 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
I wonder why we don't use the same style for $subject as pg_basebackup
--progress, that is, use a carriage return instead of a newline after
each line reporting the number of tuples copied?Patch applies cleanly, compiles, and works for me.
Thanks a lot for the quick review.
My 0.02€:
fprintf -> fputs or fputc to avoid a format parsing, or maybe use %c in
the formats.As the format is not constant, ISTM that vfprintf should be called, not
fprintf (even if in practice fprintf does call vfprintf internally).I'm not sure what the compilers does with isatty(fileno(stderr)), maybe
the eol could be precomputed:char eol = isatty(...) ? '\r' : '\n';
and reused afterwards in the loop:
fprintf(stderr, ".... %c", ..., eol);
that would remove the added in-loop printing.
I have updated the patch based on these observations. Attached v2.
Thanks,
Amit
Attachments:
compactify-pgbench-init-progress-output_v2.patchapplication/octet-stream; name=compactify-pgbench-init-progress-output_v2.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4a7ac1f821..081b647d31 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3833,6 +3833,8 @@ initGenerateDataClientSide(PGconn *con)
double elapsed_sec,
remaining_sec;
int log_interval = 1;
+ /* Stay on the same line if reporting to a terminal */
+ char eolchar = isatty(fileno(stderr)) ? '\r' : '\n';
fprintf(stderr, "generating data (client-side)...\n");
@@ -3906,10 +3908,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)\n",
+ 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);
+ elapsed_sec, remaining_sec, eolchar);
}
/* let's not call the timing for each row, but only each 100 rows */
else if (use_quiet && (j % 100 == 0))
@@ -3923,16 +3925,19 @@ 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)\n",
+ 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);
+ (int) (((int64) j * 100) / (naccounts * (int64) scale)), elapsed_sec, remaining_sec, eolchar);
/* skip to the next interval */
log_interval = (int) ceil(elapsed_sec / LOG_STEP_SECONDS);
}
}
-
}
+
+ if (eolchar != '\n')
+ fprintf(stderr, "\n");
+
if (PQputline(con, "\\.\n"))
{
fprintf(stderr, "very last PQputline failed\n");
Hello Amit,
I have updated the patch based on these observations. Attached v2.
Patch v2 applies & compiles cleanly, works for me.
I'm not partial to Hungarian notation conventions, which is not widely
used elsewhere in pg. I'd suggest eolchar -> eol or line_end or whatever,
but others may have different opinion. Maybe having a char variable is a
rare enough occurence which warrants advertising it.
Maybe use fputc instead of fprintf in the closing output?
I'm unsure about what happens on MacOS and Windows terminal, but if it
works for other commands progress options, it is probably all right.
--
Fabien.
Hi Fabien,
Thanks for taking a look again.
On Sat, Nov 30, 2019 at 4:28 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
I have updated the patch based on these observations. Attached v2.
Patch v2 applies & compiles cleanly, works for me.
I'm not partial to Hungarian notation conventions, which is not widely
used elsewhere in pg. I'd suggest eolchar -> eol or line_end or whatever,
but others may have different opinion. Maybe having a char variable is a
rare enough occurence which warrants advertising it.
On second thought, I'm fine with just eol.
Maybe use fputc instead of fprintf in the closing output?
OK, done.
I'm unsure about what happens on MacOS and Windows terminal, but if it
works for other commands progress options, it is probably all right.
I wrote the v1 patch on CentOS Linux, and now on MacOS. It would be
great if someone can volunteer to test on Windows terminal.
Attached v3.
Thanks,
Amit
Attachments:
compactify-pgbench-init-progress-output_v3.patchapplication/octet-stream; name=compactify-pgbench-init-progress-output_v3.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4a7ac1f821..e2ce88ff3f 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3833,6 +3833,8 @@ initGenerateDataClientSide(PGconn *con)
double elapsed_sec,
remaining_sec;
int log_interval = 1;
+ /* Stay on the same line if reporting to a terminal */
+ char eol = isatty(fileno(stderr)) ? '\r' : '\n';
fprintf(stderr, "generating data (client-side)...\n");
@@ -3906,10 +3908,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)\n",
+ 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);
+ elapsed_sec, remaining_sec, eol);
}
/* let's not call the timing for each row, but only each 100 rows */
else if (use_quiet && (j % 100 == 0))
@@ -3923,16 +3925,19 @@ 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)\n",
+ 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);
+ (int) (((int64) j * 100) / (naccounts * (int64) scale)), elapsed_sec, remaining_sec, eol);
/* skip to the next interval */
log_interval = (int) ceil(elapsed_sec / LOG_STEP_SECONDS);
}
}
-
}
+
+ if (eol != '\n')
+ fputc('\n', stderr);
+
if (PQputline(con, "\\.\n"))
{
fprintf(stderr, "very last PQputline failed\n");
I wrote the v1 patch on CentOS Linux, and now on MacOS. It would be
great if someone can volunteer to test on Windows terminal.
I do not have that.
Attached v3.
Patch applies, compiles, works for me. No further comments.
I switched the patch as ready.
--
Fabien.
Hi Fabien,
On Sun, Dec 1, 2019 at 4:33 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Patch applies, compiles, works for me. No further comments.
I switched the patch as ready.
Thanks a lot.
Regards,
Amit
On Mon, Dec 02, 2019 at 02:30:47PM +0900, Amit Langote wrote:
On Sun, Dec 1, 2019 at 4:33 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Patch applies, compiles, works for me. No further comments.
I switched the patch as ready.
Thanks a lot.
An issue with the patch as proposed is that its style is different
than what pg_rewind and pg_basebackup do in the same cases, but who
cares :)
By the way, the first patch sent on this thread had a bug when
redirecting the output of stderr to a log file because it was printing
a newline for each loop done on naccounts, but you just want to print
a log every 100 rows or 100k rows depending on if the quiet mode is
used or not, so the log file grew in size with mostly empty lines. v3
does that correctly of course as you add the last character of one log
line each time the log entry is printed.
Another question I have is why doing only that for the data
initialization phase? Wouldn't it make sense to be consistent with
the other tools having --progress and do the same dance in pgbench's
printProgressReport()?
NB: Note as well that pgindent complains for one thing, a newline
before the call to isatty.
--
Michael
Another question I have is why doing only that for the data
initialization phase? Wouldn't it make sense to be consistent with the
other tools having --progress and do the same dance in pgbench's
printProgressReport()?
I thought of it but did not suggest it.
When running a bench I like seeing the last few seconds status to see the
dynamic evolution at a glance, and overwriting the previous line would
hide that.
--
Fabien.
Thanks for the review.
On Mon, Dec 2, 2019 at 3:28 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Dec 02, 2019 at 02:30:47PM +0900, Amit Langote wrote:
On Sun, Dec 1, 2019 at 4:33 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Patch applies, compiles, works for me. No further comments.
I switched the patch as ready.
Thanks a lot.
An issue with the patch as proposed is that its style is different
than what pg_rewind and pg_basebackup do in the same cases, but who
cares :)
How about adding a function, say print_progress_to_stderr(const char
*fmt,...), exposed to the front-end utilities and use it from
everywhere? Needless to say that it will contain the check for whether
stderr points to terminal or a file and print accordingly.
By the way, the first patch sent on this thread had a bug when
redirecting the output of stderr to a log file because it was printing
a newline for each loop done on naccounts, but you just want to print
a log every 100 rows or 100k rows depending on if the quiet mode is
used or not, so the log file grew in size with mostly empty lines.
Naive programming :(
Another question I have is why doing only that for the data
initialization phase? Wouldn't it make sense to be consistent with
the other tools having --progress and do the same dance in pgbench's
printProgressReport()?
Considering Fabien's comment on this, we will have to check which
other instances are printing information that is not very useful to
look at line-by-line.
NB: Note as well that pgindent complains for one thing, a newline
before the call to isatty.
Fixed.
Attached v4.
Thanks,
Amit
Attachments:
compactify-pgbench-init-progress-output_v4.patchapplication/octet-stream; name=compactify-pgbench-init-progress-output_v4.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 7f7d0a45c3..9788692b2e 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3835,6 +3835,9 @@ initGenerateDataClientSide(PGconn *con)
remaining_sec;
int log_interval = 1;
+ /* Stay on the same line if reporting to a terminal */
+ char eol = isatty(fileno(stderr)) ? '\r' : '\n';
+
fprintf(stderr, "generating data (client-side)...\n");
/*
@@ -3910,10 +3913,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)\n",
+ 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);
+ elapsed_sec, remaining_sec, eol);
}
/* let's not call the timing for each row, but only each 100 rows */
else if (use_quiet && (j % 100 == 0))
@@ -3927,16 +3930,19 @@ 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)\n",
+ 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);
+ (int) (((int64) j * 100) / (naccounts * (int64) scale)), elapsed_sec, remaining_sec, eol);
/* skip to the next interval */
log_interval = (int) ceil(elapsed_sec / LOG_STEP_SECONDS);
}
}
-
}
+
+ if (eol != '\n')
+ fputc('\n', stderr);
+
if (PQputline(con, "\\.\n"))
{
fprintf(stderr, "very last PQputline failed\n");
Attached v4.
Patch applies cleanly, compiles, works for me. Put it back to ready.
--
Fabien.
On Tue, Dec 03, 2019 at 10:30:35AM +0900, Amit Langote wrote:
How about adding a function, say print_progress_to_stderr(const char
*fmt,...), exposed to the front-end utilities and use it from
everywhere? Needless to say that it will contain the check for whether
stderr points to terminal or a file and print accordingly.
I have considered this point, but that does not seem worth the
complication as each tool has its own idea of the log output, its own
idea of the log output timing and its own idea of when it is necessary
to print the last newline when finishing to the output with '\r'.
Considering Fabien's comment on this, we will have to check which
other instances are printing information that is not very useful to
look at line-by-line.
Thanks, applied the part for the initialization to HEAD. I got to
think about Fabien's point and it is true that for pgbench's
--progress not keeping things on the same line for a terminal has
advantages because the data printed is not cumulative: that's a
summary of the previous state printed which can be compared.
Note: the patch works on Windows, no problem.
--
Michael
On Wed, Dec 4, 2019 at 11:35 AM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Dec 03, 2019 at 10:30:35AM +0900, Amit Langote wrote:
How about adding a function, say print_progress_to_stderr(const char
*fmt,...), exposed to the front-end utilities and use it from
everywhere? Needless to say that it will contain the check for whether
stderr points to terminal or a file and print accordingly.I have considered this point, but that does not seem worth the
complication as each tool has its own idea of the log output, its own
idea of the log output timing and its own idea of when it is necessary
to print the last newline when finishing to the output with '\r'.
Okay, seems more trouble than worth to design around all that.
Considering Fabien's comment on this, we will have to check which
other instances are printing information that is not very useful to
look at line-by-line.Thanks, applied the part for the initialization to HEAD. I got to
think about Fabien's point and it is true that for pgbench's
--progress not keeping things on the same line for a terminal has
advantages because the data printed is not cumulative: that's a
summary of the previous state printed which can be compared.Note: the patch works on Windows, no problem.
Thanks for checking and committing the patch.
Regards,
Amit