Thread: 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

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

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

> 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

> 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