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
Attachments:
0001-Fix-printing-last-progress-report-line-in-client-pro.patchtext/x-patch; charset=UTF-8; name=0001-Fix-printing-last-progress-report-line-in-client-pro.patchDownload+4-3
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