Re: pgbench -i progress output on terminal
On Wed, Dec 4, 2019 at 11:35 AM Michael Paquier 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
Re: pgbench -i progress output on terminal
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 signature.asc Description: PGP signature
Re: pgbench -i progress output on terminal
Attached v4. Patch applies cleanly, compiles, works for me. Put it back to ready. -- Fabien.
Re: pgbench -i progress output on terminal
Thanks for the review. On Mon, Dec 2, 2019 at 3:28 PM Michael Paquier 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 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 compactify-pgbench-init-progress-output_v4.patch Description: Binary data
Re: pgbench -i progress output on terminal
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.
Re: pgbench -i progress output on terminal
On Mon, Dec 02, 2019 at 02:30:47PM +0900, Amit Langote wrote: > On Sun, Dec 1, 2019 at 4:33 AM Fabien COELHO 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 signature.asc Description: PGP signature
Re: pgbench -i progress output on terminal
Hi Fabien, On Sun, Dec 1, 2019 at 4:33 AM Fabien COELHO wrote: > Patch applies, compiles, works for me. No further comments. > > I switched the patch as ready. Thanks a lot. Regards, Amit
Re: pgbench -i progress output on terminal
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.
Re: pgbench -i progress output on terminal
Hi Fabien, Thanks for taking a look again. On Sat, Nov 30, 2019 at 4:28 PM Fabien COELHO 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 compactify-pgbench-init-progress-output_v3.patch Description: Binary data
Re: pgbench -i progress output on terminal
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.
Re: pgbench -i progress output on terminal
Hi Fabien, On Fri, Nov 29, 2019 at 10:13 PM Fabien COELHO 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 compactify-pgbench-init-progress-output_v2.patch Description: Binary data
Re: pgbench -i progress output on terminal
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.
Re: pgbench -i progress output on terminal
Hi Fabien, On Thu, Nov 28, 2019 at 4:35 PM Fabien COELHO 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
Re: pgbench -i progress output on terminal
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.
Re: pgbench -i progress output on terminal
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 signature.asc Description: PGP signature
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 compactify-pgbench-init-progress-output.patch Description: Binary data
