pgbench -i progress output on terminal

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

pgbench -i progress output on terminal

Amit Langote
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 (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Michael Paquier-2
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Fabien COELHO-3
In reply to this post by Amit Langote

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.


Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Amit Langote
Hi Fabien,

On Thu, Nov 28, 2019 at 4:35 PM Fabien COELHO <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Fabien COELHO-3
In reply to this post by Amit Langote

> 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.
Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Amit Langote
Hi Fabien,

On Fri, Nov 29, 2019 at 10:13 PM Fabien COELHO <[hidden email]> 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 (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Fabien COELHO-3

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.


Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Amit Langote
Hi Fabien,

Thanks for taking a look again.

On Sat, Nov 30, 2019 at 4:28 PM Fabien COELHO <[hidden email]> 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 (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Fabien COELHO-3

> 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.


Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Amit Langote
Hi Fabien,

On Sun, Dec 1, 2019 at 4:33 AM Fabien COELHO <[hidden email]> wrote:
> Patch applies, compiles, works for me. No further comments.
>
> I switched the patch as ready.

Thanks a lot.

Regards,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Michael Paquier-2
On Mon, Dec 02, 2019 at 02:30:47PM +0900, Amit Langote wrote:
> On Sun, Dec 1, 2019 at 4:33 AM Fabien COELHO <[hidden email]> 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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Fabien COELHO-3

> 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.


Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Amit Langote
In reply to this post by Michael Paquier-2
Thanks for the review.

On Mon, Dec 2, 2019 at 3:28 PM Michael Paquier <[hidden email]> 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 <[hidden email]> 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 (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Fabien COELHO-3

> Attached v4.

Patch applies cleanly, compiles, works for me. Put it back to ready.

--
Fabien.


Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Michael Paquier-2
In reply to this post by Amit Langote
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench -i progress output on terminal

Amit Langote
On Wed, Dec 4, 2019 at 11:35 AM Michael Paquier <[hidden email]> 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