Thread: Newline after --progress report

While hacking on pg_rewind, this in pg_rewind's main() function caught 
my eye:

     progress_report(true);
     printf("\n");

It is peculiar, because progress_report() uses fprintf(stderr, ...) for 
all its printing, and in fact the only other use of printf() in 
pg_rewind is in printing the "pg_rewind --help" text.

I think the idea here was to move to the next line, after 
progress_report() has updated the progress line for the last time. It 
probably also should not be printed, when "--progress" is not used.

Attached is a patch to fix this, as well as a similar issue in 
pg_checksums. pg_basebackup and pgbench also print progres reports like 
this, but they seem correct to me.

- Heikki

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> While hacking on pg_rewind, this in pg_rewind's main() function caught 
> my eye:

Good catch.

> Attached is a patch to fix this, as well as a similar issue in 
> pg_checksums. pg_basebackup and pgbench also print progres reports like 
> this, but they seem correct to me.

I wonder whether it'd be better to push the responsibility for this
into progress_report(), by adding an additional parameter "bool last"
or the like.  Then the callers would not need such an unseemly amount
of knowledge about what progress_report() is doing.

            regards, tom lane

On 14/08/2020 16:51, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>> Attached is a patch to fix this, as well as a similar issue in
>> pg_checksums. pg_basebackup and pgbench also print progres reports like
>> this, but they seem correct to me.
> 
> I wonder whether it'd be better to push the responsibility for this
> into progress_report(), by adding an additional parameter "bool last"
> or the like.  Then the callers would not need such an unseemly amount
> of knowledge about what progress_report() is doing.

Good point. Pushed a patch along those lines.

- Heikki

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> Good point. Pushed a patch along those lines.

Uh ... you patched v12 but not v13?

Also, I'd recommend that you NOT do this:

+   fprintf(stderr, (!finished && isatty(fileno(stderr))) ? "\r" : "\n");

as it breaks printf format verification in many/most compilers.

            regards, tom lane

On 17/08/2020 16:59, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>> Good point. Pushed a patch along those lines.
> 
> Uh ... you patched v12 but not v13?

Darn, I forgot it exists.

> Also, I'd recommend that you NOT do this:
> 
> +   fprintf(stderr, (!finished && isatty(fileno(stderr))) ? "\r" : "\n");
> 
> as it breaks printf format verification in many/most compilers.

Ok. I pushed the same commit to v12 as to other branches now, to keep 
them in sync. I'll go fix that as a separate commit. Thanks!

- Heikki