Re: [HACKERS] pg_basebackup --progress output for batch execution

[Martín Marqués]

Hi,

Thanks for having a look at this patch.

2017-11-09 20:55 GMT-03:00 Jeff Janes :
> On Fri, Sep 29, 2017 at 4:00 PM, Martin Marques
>  wrote:
>>
>> Hi,
>>
>> Some time ago I had to work on a system where I was cloning a standby
>> using pg_basebackup, that didn't have screen or tmux. For that reason I
>> redirected the output to a file and ran it with nohup.
>>
>> I normally (always actually ;) ) run pg_basebackup with --progress and
>> --verbose so I can follow how much has been done. When done on a tty you
>> get a nice progress bar with the percentage that has been cloned.
>>
>> The problem came with the execution and redirection of the output, as
>> the --progress option will write a *very* long line!
>>
>> Back then I thought of writing a patch (actually someone suggested I do
>> so) to add a --batch-mode option which would change the behavior
>> pg_basebackup has when printing the output messages.
>
>
>
> While separate lines in the output file is better than one very long line,
> it still doesn't seem so useful.  If you aren't watching it in real time,
> then you really need to have a timestamp on each line so that you can
> interpret the output.  The lines are about one second apart, but I don't
> know robust that timing is under all conditions.

I kind of disagree with your view here.

If the cloning process takes many hours to complete (in my case, it
was around 12 hours IIRC) you might want to peak at the log every now
and then with tail.

I do agree on adding a timestamp prefix to each line, as it's not
clear from the code how often progress_report() is called.

> I think I agree with Arthur that I'd rather have the decision made by
> inspecting whether output is going to a tty, rather than by adding another
> command line option.  But maybe that is not detected robustly enough across
> all platforms and circumstances?

In this case, Arthur's idea is good, but would make the patch less
generic/flexible for the end user.

That's why I tried to reproduce what top does when executed with -b
(Batch mode operation). There, it's the end user who decides how the
output is formatted (well, saying it decides on formatting a bit of an
overstatement, but you get it ;) )

An example where using isatty() might fail is if you run pg_basebackup
from a tty but redirect the output to a file, I believe that in that
case isatty() will return true, but it's very likely that the user
might want batch mode output.

But maybe we should also add Arthurs idea anyway (when not in batch
mode), as it seems pretty lame to output progress in one line if you
are not in a tty.

Thoughts?

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_basebackup --progress output for batch execution

[Arthur Zakirov]

On Thu, Nov 09, 2017 at 03:55:36PM -0800, Jeff Janes wrote:
> 
> I think I agree with Arthur that I'd rather have the decision made by
> inspecting whether output is going to a tty, rather than by adding another
> command line option.  But maybe that is not detected robustly enough across
> all platforms and circumstances?
> 

isatty() is used within Postgres code already (for example, pg_upgrade/util.c).
However, it seems that on Windows isatty() is deprecated and it is recommended 
to use _isatty(). Moreover, on Windows it can give false positive result [1], 
if I'm not mistaken:

> The _isatty function determines whether fd is associated with a character 
> device (a terminal, console, printer, or serial port).

1 - https://msdn.microsoft.com/en-us/library/f4s0ddew(v=vs.140).aspx

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_basebackup --progress output for batch execution

[Jeff Janes]

On Fri, Sep 29, 2017 at 4:00 PM, Martin Marques <
martin.marq...@2ndquadrant.com> wrote:

> Hi,
>
> Some time ago I had to work on a system where I was cloning a standby
> using pg_basebackup, that didn't have screen or tmux. For that reason I
> redirected the output to a file and ran it with nohup.
>
> I normally (always actually ;) ) run pg_basebackup with --progress and
> --verbose so I can follow how much has been done. When done on a tty you
> get a nice progress bar with the percentage that has been cloned.
>
> The problem came with the execution and redirection of the output, as
> the --progress option will write a *very* long line!
>
> Back then I thought of writing a patch (actually someone suggested I do
> so) to add a --batch-mode option which would change the behavior
> pg_basebackup has when printing the output messages.
>


While separate lines in the output file is better than one very long line,
it still doesn't seem so useful.  If you aren't watching it in real time,
then you really need to have a timestamp on each line so that you can
interpret the output.  The lines are about one second apart, but I don't
know robust that timing is under all conditions.

I think I agree with Arthur that I'd rather have the decision made by
inspecting whether output is going to a tty, rather than by adding another
command line option.  But maybe that is not detected robustly enough across
all platforms and circumstances?

Cheers,

Jeff

Re: [HACKERS] pg_basebackup --progress output for batch execution

[Arthur Zakirov]

Hello,

On Sun, Oct 01, 2017 at 04:49:17PM -0300, Martin Marques wrote:
> Updated patch with documentation of the new option.
> 

I have checked the patch.
The patch is applied and compiled correctly without any errors. Tests passed.
The documentation doesn't have errors too.


I have a little suggestion. Maybe insert new line without any additional 
parameters? We can check that stderr is not terminal using isatty().

The code could become:

if (!isatty(fileno(stderr)))
fprintf(stderr, "\n");
else
fprintf(stderr, "\r");

Also it could be good to not insert new line after progress:

if (showprogress)
{
progress_report(PQntuples(res), NULL, true);
/* if (!batchmode) */
/* or */
if (isatty(fileno(stderr)))
fprintf(stderr, "\n");  /* Need to move to next line */
}

Otherwise we have an extra empty line:

pg_basebackup: initiating base backup, waiting for checkpoint to complete
pg_basebackup: checkpoint completed
pg_basebackup: write-ahead log start point: 0/1C28 on timeline 1
pg_basebackup: starting background WAL receiver
pg_basebackup: created temporary replication slot "pg_basebackup_19635"
 0/183345 kB (0%), 0/1 tablespace (data_repl/backup_label )
183355/183355 kB (100%), 0/1 tablespace (data_repl/global/pg_control)
183355/183355 kB (100%), 1/1 tablespace

pg_basebackup: write-ahead log end point: 0/1CF8
pg_basebackup: waiting for background process to finish streaming ...
pg_basebackup: base backup completed

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_basebackup --progress output for batch execution

[Martin Marques]

Updated patch with documentation of the new option.


-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From ede201ed96d41d799dc3c83dfab1cdcc03e5ced4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=ADn=20Marqu=C3=A9s?=
 
Date: Sun, 1 Oct 2017 16:39:41 -0300
Subject: [PATCH] Adding an option to pg_basebackup to output messages as if it
 were running in batch-mode, as opossed to running in a tty.

This is usefull when using --progress and redirecting the output to
a file for later inspection with tail.

New option --batch-mode with the short option -b added.
---
 doc/src/sgml/ref/pg_basebackup.sgml   | 16 
 src/bin/pg_basebackup/pg_basebackup.c | 19 +--
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index f790c56..db5160f 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -461,6 +461,22 @@ PostgreSQL documentation
  
 
  
+  -b
+  --batch-mode
+  
+   
+Runs pg_basebackup in batch mode. This is useful if
+the output is to be pipped so the other end of the pipe reads each line.
+   
+   
+Using this option with --progress will result in
+printing each progress output with a newline at the end, instead of a
+carrige return.
+   
+  
+ 
+
+ 
   -S slotname
   --slot=slotname
   
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index dac7299..cf97ea3 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -82,6 +82,7 @@ static char format = 'p';		/* p(lain)/t(ar) */
 static char *label = "pg_basebackup base backup";
 static bool noclean = false;
 static bool showprogress = false;
+static bool batchmode = false;
 static int	verbose = 0;
 static int	compresslevel = 0;
 static IncludeWal includewal = STREAM_WAL;
@@ -355,6 +356,7 @@ usage(void)
 	printf(_("  -P, --progress show progress information\n"));
 	printf(_("  -S, --slot=SLOTNAMEreplication slot to use\n"));
 	printf(_("  --no-slot  prevent creation of temporary replication slot\n"));
+	printf(_("  -b, --batch-mode   run in batch mode\n"));
 	printf(_("  -v, --verbose  output verbose messages\n"));
 	printf(_("  -V, --version  output version information, then exit\n"));
 	printf(_("  -?, --help show this help, then exit\n"));
@@ -806,7 +808,10 @@ progress_report(int tablespacenum, const char *filename, bool force)
 totaldone_str, totalsize_str, percent,
 tablespacenum, tablespacecount);
 
-	fprintf(stderr, "\r");
+	if (batchmode)
+		fprintf(stderr, "\n");
+	else
+		fprintf(stderr, "\r");
 }
 
 static int32
@@ -1786,7 +1791,13 @@ BaseBackup(void)
 progname);
 
 	if (showprogress && !verbose)
+	{
 		fprintf(stderr, "waiting for checkpoint\r");
+		if (batchmode)
+			fprintf(stderr, "\n");
+		else
+			fprintf(stderr, "\r");
+	}
 
 	basebkp =
 		psprintf("BASE_BACKUP LABEL '%s' %s %s %s %s %s %s",
@@ -2118,6 +2129,7 @@ main(int argc, char **argv)
 		{"status-interval", required_argument, NULL, 's'},
 		{"verbose", no_argument, NULL, 'v'},
 		{"progress", no_argument, NULL, 'P'},
+		{"batch-mode", no_argument, NULL, 'b'},
 		{"waldir", required_argument, NULL, 1},
 		{"no-slot", no_argument, NULL, 2},
 		{NULL, 0, NULL, 0}
@@ -2146,7 +2158,7 @@ main(int argc, char **argv)
 
 	atexit(cleanup_directories_atexit);
 
-	while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWvP",
+	while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWvPb",
 			long_options, _index)) != -1)
 	{
 		switch (c)
@@ -2288,6 +2300,9 @@ main(int argc, char **argv)
 			case 'P':
 showprogress = true;
 break;
+			case 'b':
+batchmode = true;
+break;
 			default:
 
 /*
-- 
2.9.5


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pg_basebackup --progress output for batch execution

[Martin Marques]

Hi,

Some time ago I had to work on a system where I was cloning a standby
using pg_basebackup, that didn't have screen or tmux. For that reason I
redirected the output to a file and ran it with nohup.

I normally (always actually ;) ) run pg_basebackup with --progress and
--verbose so I can follow how much has been done. When done on a tty you
get a nice progress bar with the percentage that has been cloned.

The problem came with the execution and redirection of the output, as
the --progress option will write a *very* long line!

Back then I thought of writing a patch (actually someone suggested I do
so) to add a --batch-mode option which would change the behavior
pg_basebackup has when printing the output messages.

Attach is the patch. I'll be submitting it to the CF.

P.D.: I'm aware that there's a documentation patch missing. :)

Kind regards,

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From f6e54c0d062d62daf70c0870f96032eb0c102e66 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=ADn=20Marqu=C3=A9s?=
 
Date: Fri, 29 Sep 2017 19:21:44 -0300
Subject: [PATCH] Adding an option to pg_basebackup to output messages as if it
 were running in batch-mode, as opossed to running in a tty.

This is usefull when using --progress and redirecting the output to
a file for later inspection with tail.

New option --batch-mode with the short option -b added.
---
 src/bin/pg_basebackup/pg_basebackup.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index dac7299..cf97ea3 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -82,6 +82,7 @@ static char format = 'p';		/* p(lain)/t(ar) */
 static char *label = "pg_basebackup base backup";
 static bool noclean = false;
 static bool showprogress = false;
+static bool batchmode = false;
 static int	verbose = 0;
 static int	compresslevel = 0;
 static IncludeWal includewal = STREAM_WAL;
@@ -355,6 +356,7 @@ usage(void)
 	printf(_("  -P, --progress show progress information\n"));
 	printf(_("  -S, --slot=SLOTNAMEreplication slot to use\n"));
 	printf(_("  --no-slot  prevent creation of temporary replication slot\n"));
+	printf(_("  -b, --batch-mode   run in batch mode\n"));
 	printf(_("  -v, --verbose  output verbose messages\n"));
 	printf(_("  -V, --version  output version information, then exit\n"));
 	printf(_("  -?, --help show this help, then exit\n"));
@@ -806,7 +808,10 @@ progress_report(int tablespacenum, const char *filename, bool force)
 totaldone_str, totalsize_str, percent,
 tablespacenum, tablespacecount);
 
-	fprintf(stderr, "\r");
+	if (batchmode)
+		fprintf(stderr, "\n");
+	else
+		fprintf(stderr, "\r");
 }
 
 static int32
@@ -1786,7 +1791,13 @@ BaseBackup(void)
 progname);
 
 	if (showprogress && !verbose)
+	{
 		fprintf(stderr, "waiting for checkpoint\r");
+		if (batchmode)
+			fprintf(stderr, "\n");
+		else
+			fprintf(stderr, "\r");
+	}
 
 	basebkp =
 		psprintf("BASE_BACKUP LABEL '%s' %s %s %s %s %s %s",
@@ -2118,6 +2129,7 @@ main(int argc, char **argv)
 		{"status-interval", required_argument, NULL, 's'},
 		{"verbose", no_argument, NULL, 'v'},
 		{"progress", no_argument, NULL, 'P'},
+		{"batch-mode", no_argument, NULL, 'b'},
 		{"waldir", required_argument, NULL, 1},
 		{"no-slot", no_argument, NULL, 2},
 		{NULL, 0, NULL, 0}
@@ -2146,7 +2158,7 @@ main(int argc, char **argv)
 
 	atexit(cleanup_directories_atexit);
 
-	while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWvP",
+	while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWvPb",
 			long_options, _index)) != -1)
 	{
 		switch (c)
@@ -2288,6 +2300,9 @@ main(int argc, char **argv)
 			case 'P':
 showprogress = true;
 break;
+			case 'b':
+batchmode = true;
+break;
 			default:
 
 /*
-- 
2.9.5


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers