pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedNewline after --progress report
5+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2020-08-18 734478200a aecefffc3f bff41b2b89 Avoid non-constant format string argument to fprintf().
2020-08-17 d7ec8337f9 9f44d71b06 5ca1798f32 Fix printing last progress report line in client programs.
* Newline after --progress report
@ 2020-08-14 07:57 Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 5+ messages in thread
From: Heikki Linnakangas @ 2020-08-14 07:57 UTC (permalink / raw)
To: pgsql-hackers
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.patch (text/x-patch, 1.8K) download
From b4a64c3aca7257336c3ca1bf4bee8675e148de10 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Wed, 12 Aug 2020 23:27:01 +0300
Subject: [PATCH 1/1] Fix printing last progress report line in client
programs.
A number of client programs have a "--progress" option that when printing
to a TTY, updates the current line by printing a '\r' and overwriting it.
After the last line, an extra '\n' needs to be printed to move the cursor
to the next line. pg_basebackup and pgbench got this right, but pg_rewind
and pg_checksums were slightly wrong. pg_rewind printed the newline to
stdout instead of stderr, and pg_checksum printed the newline even when
not printing to a TTY. Fix them.
---
src/bin/pg_checksums/pg_checksums.c | 3 ++-
src/bin/pg_rewind/pg_rewind.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1daa5aed0e0..9ae884897ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -626,7 +626,8 @@ main(int argc, char *argv[])
if (showprogress)
{
progress_report(true);
- fprintf(stderr, "\n"); /* Need to move to next line */
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n"); /* Need to move to next line */
}
printf(_("Checksum operation completed\n"));
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 0015d3b461a..c000c12fa58 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -422,7 +422,8 @@ main(int argc, char **argv)
executeFileMap();
progress_report(true);
- printf("\n");
+ if (showprogress && isatty(fileno(stderr)))
+ fprintf(stderr, "\n"); /* Need to move to next line */
if (showprogress)
pg_log_info("creating backup label and updating control file");
--
2.20.1
* Re: Newline after --progress report
@ 2020-08-14 13:51 Tom Lane <tgl@sss.pgh.pa.us>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2020-08-14 13:51 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Newline after --progress report
@ 2020-08-17 07:14 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 5+ messages in thread
From: Heikki Linnakangas @ 2020-08-17 07:14 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Newline after --progress report
@ 2020-08-17 13:59 Tom Lane <tgl@sss.pgh.pa.us>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2020-08-17 13:59 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Newline after --progress report
@ 2020-08-17 14:20 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 5+ messages in thread
From: Heikki Linnakangas @ 2020-08-17 14:20 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2020-08-17 14:20 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-08-14 07:57 Newline after --progress report Heikki Linnakangas <hlinnaka@iki.fi>
2020-08-14 13:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-08-17 07:14 ` Heikki Linnakangas <hlinnaka@iki.fi>
2020-08-17 13:59 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-08-17 14:20 ` Heikki Linnakangas <hlinnaka@iki.fi>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox