Re: [HACKERS] pg_basebackup wish list

[Michael Paquier]

On Thu, Sep 15, 2016 at 9:26 AM, Peter Eisentraut
 wrote:
> On 9/13/16 7:24 PM, Michael Paquier wrote:
>> PostgresNode.pm had better use the new --noclean option in its calls,
>> the new default is not useful for debugging.
>
> We don't do it for initdb either.  Is that a problem?

Right. In case of failure it may prove to be useful for debugging if
we'd use it.
-- 
Michael


-- 
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 wish list

[Peter Eisentraut]

On 9/13/16 7:24 PM, Michael Paquier wrote:
> PostgresNode.pm had better use the new --noclean option in its calls,
> the new default is not useful for debugging.

We don't do it for initdb either.  Is that a problem?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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 wish list

[Michael Paquier]

On Wed, Sep 14, 2016 at 6:52 AM, Peter Eisentraut
 wrote:
> On 9/12/16 3:31 PM, Tom Lane wrote:
>> Hm, there was just a kerfuffle about spelling things like this
>> "--no-clean" etc.  I wasn't on board with removing existing spellings,
>> but surely all newly added switches should use the "no-" spelling?
>
> This is supposed to be parallel to initdb's option.  So if we rename or
> migrate the one in initdb, then we can just hard-rename this one.  But
> until that is decided, it seems better to keep it the same as initdb has
> right now.

PostgresNode.pm had better use the new --noclean option in its calls,
the new default is not useful for debugging. Please see attached.
-- 
Michael
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index fede1e6..0b8c8e6 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -476,7 +476,7 @@ sub backup
 
 	print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
 	TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-p', $port,
-		'-x');
+		'-x', '--noclean');
 	print "# Backup finished\n";
 }
 

-- 
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 wish list

[Peter Eisentraut]

On 9/12/16 3:31 PM, Tom Lane wrote:
> Hm, there was just a kerfuffle about spelling things like this
> "--no-clean" etc.  I wasn't on board with removing existing spellings,
> but surely all newly added switches should use the "no-" spelling?

This is supposed to be parallel to initdb's option.  So if we rename or
migrate the one in initdb, then we can just hard-rename this one.  But
until that is decided, it seems better to keep it the same as initdb has
right now.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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 wish list

[Tom Lane]

Peter Eisentraut  writes:
> On 8/19/16 1:04 AM, Masahiko Sawada wrote:
>> I agree with adding this as an option and removing directory by default.
>> And it looks good to me except for missing new line in usage output.
>> 
>> printf(_("  -l, --label=LABEL  set backup label\n"));
>> +   printf(_("  -n, --noclean  do not clean up after errors"));
>> printf(_("  -P, --progress show progress information\n"));

> Committed with that fix.  Thanks.

Hm, there was just a kerfuffle about spelling things like this
"--no-clean" etc.  I wasn't on board with removing existing spellings,
but surely all newly added switches should use the "no-" spelling?

regards, tom lane


-- 
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 wish list

[Peter Eisentraut]

On 8/19/16 1:04 AM, Masahiko Sawada wrote:
> I agree with adding this as an option and removing directory by default.
> And it looks good to me except for missing new line in usage output.
> 
> printf(_("  -l, --label=LABEL  set backup label\n"));
> +   printf(_("  -n, --noclean  do not clean up after errors"));
> printf(_("  -P, --progress show progress information\n"));

Committed with that fix.  Thanks.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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 wish list

[Simon Riggs]

On 19 August 2016 at 08:46, Michael Paquier  wrote:
> On Fri, Aug 19, 2016 at 2:04 PM, Masahiko Sawada  
> wrote:
>> I agree with adding this as an option and removing directory by default.
>> And it looks good to me except for missing new line in usage output.
>>
>> printf(_("  -l, --label=LABEL  set backup label\n"));
>> +   printf(_("  -n, --noclean  do not clean up after errors"));
>> printf(_("  -P, --progress show progress information\n"));
>>
>> Registered this patch to CF1.
>
> +1 for the idea. Looking at the patch it is taking a sane approach.

Apart from this one liner change we look good to go.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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 wish list

[Michael Paquier]

On Fri, Aug 19, 2016 at 2:04 PM, Masahiko Sawada  wrote:
> I agree with adding this as an option and removing directory by default.
> And it looks good to me except for missing new line in usage output.
>
> printf(_("  -l, --label=LABEL  set backup label\n"));
> +   printf(_("  -n, --noclean  do not clean up after errors"));
> printf(_("  -P, --progress show progress information\n"));
>
> Registered this patch to CF1.

+1 for the idea. Looking at the patch it is taking a sane approach.
-- 
Michael


-- 
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 wish list

[Masahiko Sawada]

On Fri, Aug 19, 2016 at 7:06 AM, Peter Eisentraut
 wrote:
> On 7/12/16 9:55 PM, Masahiko Sawada wrote:
>> And what I think is pg_baseback never remove the directory specified
>> by -D option even if execution is failed. initdb command behaves so.
>> I think it's helpful for backup operation.
>
> This has been bothering me as well.
>
> How about the attached patch as a start?
>

Thank you for the patch!

I agree with adding this as an option and removing directory by default.
And it looks good to me except for missing new line in usage output.

printf(_("  -l, --label=LABEL  set backup label\n"));
+   printf(_("  -n, --noclean  do not clean up after errors"));
printf(_("  -P, --progress show progress information\n"));

Registered this patch to CF1.

Regards,

--
Masahiko Sawada


-- 
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 wish list

[Peter Eisentraut]

On 7/12/16 9:55 PM, Masahiko Sawada wrote:
> And what I think is pg_baseback never remove the directory specified
> by -D option even if execution is failed. initdb command behaves so.
> I think it's helpful for backup operation.

This has been bothering me as well.

How about the attached patch as a start?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 910310b7eab88af8972906307a55e02e64618da7 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 18 Aug 2016 12:00:00 -0400
Subject: [PATCH] pg_basebackup: Clean created directories on failure

Like initdb, clean up created data and xlog directories, unless the new
-n/--noclean option is specified.

Tablespace directories are not cleaned up, but a message is written
about that.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 18 +
 src/bin/pg_basebackup/pg_basebackup.c| 98 ++--
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 10 ++-
 3 files changed, 119 insertions(+), 7 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 03615da..9f1eae1 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -399,6 +399,24 @@ Options
  
 
  
+  -n
+  --noclean
+  
+   
+By default, when pg_basebackup aborts with an
+error, it removes any directories it might have created before
+discovering that it cannot finish the job (for example, data directory
+and transaction log directory). This option inhibits tidying-up and is
+thus useful for debugging.
+   
+
+   
+Note that tablespace directories are not cleaned up either way.
+   
+  
+ 
+
+ 
   -P
   --progress
   
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index ed41db8..d13a9a3 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -57,6 +57,7 @@ static TablespaceList tablespace_dirs = {NULL, NULL};
 static char *xlog_dir = "";
 static char format = 'p';		/* p(lain)/t(ar) */
 static char *label = "pg_basebackup base backup";
+static bool noclean = false;
 static bool showprogress = false;
 static int	verbose = 0;
 static int	compresslevel = 0;
@@ -68,6 +69,13 @@ static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static pg_time_t last_progress_report = 0;
 static int32 maxrate = 0;		/* no limit by default */
 
+static bool success = false;
+static bool made_new_pgdata = false;
+static bool found_existing_pgdata = false;
+static bool made_new_xlogdir = false;
+static bool found_existing_xlogdir = false;
+static bool made_tablespace_dirs = false;
+static bool found_tablespace_dirs = false;
 
 /* Progress counters */
 static uint64 totalsize;
@@ -81,6 +89,7 @@ static int	bgpipe[2] = {-1, -1};
 
 /* Handle to child process */
 static pid_t bgchild = -1;
+static bool in_log_streamer = false;
 
 /* End position for xlog streaming, empty string if unknown yet */
 static XLogRecPtr xlogendptr;
@@ -97,7 +106,7 @@ static PQExpBuffer recoveryconfcontents = NULL;
 /* Function headers */
 static void usage(void);
 static void disconnect_and_exit(int code);
-static void verify_dir_is_empty_or_create(char *dirname);
+static void verify_dir_is_empty_or_create(char *dirname, bool *created, bool *found);
 static void progress_report(int tablespacenum, const char *filename, bool force);
 
 static void ReceiveTarFile(PGconn *conn, PGresult *res, int rownum);
@@ -114,6 +123,69 @@ static void tablespace_list_append(const char *arg);
 
 
 static void
+cleanup_directories_atexit(void)
+{
+	if (success || in_log_streamer)
+		return;
+
+	if (!noclean)
+	{
+		if (made_new_pgdata)
+		{
+			fprintf(stderr, _("%s: removing data directory \"%s\"\n"),
+	progname, basedir);
+			if (!rmtree(basedir, true))
+fprintf(stderr, _("%s: failed to remove data directory\n"),
+		progname);
+		}
+		else if (found_existing_pgdata)
+		{
+			fprintf(stderr,
+	_("%s: removing contents of data directory \"%s\"\n"),
+	progname, basedir);
+			if (!rmtree(basedir, false))
+fprintf(stderr, _("%s: failed to remove contents of data directory\n"),
+		progname);
+		}
+
+		if (made_new_xlogdir)
+		{
+			fprintf(stderr, _("%s: removing transaction log directory \"%s\"\n"),
+	progname, xlog_dir);
+			if (!rmtree(xlog_dir, true))
+fprintf(stderr, _("%s: failed to remove transaction log directory\n"),
+		progname);
+		}
+		else if (found_existing_xlogdir)
+		{
+			fprintf(stderr,
+	_("%s: removing contents of transaction log directory \"%s\"\n"),
+	progname, xlog_dir);
+			if (!rmtree(xlog_dir, false))
+fprintf(stderr, _("%s: failed to remove contents of transaction log directory\n"),
+		progname);
+		}
+	}
+	else
+	{
+		if (made_new_pgdata || found_existing_pgdata)
+			

Re: [HACKERS] pg_basebackup wish list

[Jeff Janes]

On Thu, Jul 28, 2016 at 4:44 AM, Amit Kapila  wrote:
> On Tue, Jul 26, 2016 at 11:58 AM, Fujii Masao  wrote:
>> On Wed, Jul 13, 2016 at 3:06 AM, Jeff Janes  wrote:
>>> On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
>>>  wrote:
 On 7/12/16 12:53 PM, Jeff Janes wrote:
> The --help message for pg_basebackup says:
>
> -Z, --compress=0-9 compress tar output with given compression level
>
> But -Z0 is then rejected as 'invalid compression level "0"'.  The real
> docs do say 1-9, only the --help message has this bug.  Trivial patch
> attached.

 pg_dump --help and man page say it supports 0..9.  Maybe we should make
 that more consistent.
>>>
>>> pg_dump actually does support -Z0, though.  Well, sort of.  It outputs
>>> plain text.  Rather than plain text wrapped in some kind of dummy gzip
>>> header, which is what I had naively expected.
>>>
>>> Is that what -Z0 in pg_basebackup should do as well, just output
>>> uncompressed tar data, and not add the ".gz" to the "base.tar" file
>>> name?
>>
>> Yes, I think. What about the attached patch?
>>
>
> What if user tries to use -Z 0 with format as tar, won't it generate
> base.tar without any compression?  I am not sure if that is what Jeff
> intends to say in his proposal.

My initial proposal was just to change the "usage" message to match reality.

I think the current proposal is to make -Z0 be identical to having no
-Z specified at all, in other words produce a .tar file, not a .tar.gz
file.

I had thought we could make a .gz file which didn't actually use
compression, just packaged up the data behind a gzip header, but after
looking at it I don't think libz actually supports that.  Plus, it
would be pretty silly to have uncompressed data that would then have
to be "uncompressed" merely to unwrap it.  It could be useful for
testing where you don't want to write for special cases in your shell
script (which is where I discovered this, I wanted to test all values
between 0 and 9 and see which was fastest given my combination of CPU,
network, data and disks), but not useful for practical use.

I am happy with the code as currently committed.

Cheers,

Jeff


-- 
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 wish list

[Fujii Masao]

On Fri, Jul 29, 2016 at 11:01 PM, Amit Kapila  wrote:
> On Thu, Jul 28, 2016 at 7:34 PM, Fujii Masao  wrote:
>> On Thu, Jul 28, 2016 at 10:16 PM, Amit Kapila  
>> wrote:
>>
>>> I think there is some value in providing
>>> .tar for -Z 0,
>>
>> I was thinking that "-Ft -Z0" is something like an alias of "-Ft".
>> That is, the backup is taken in uncompressed tar format.
>>
>>> however in that case how should we define usage of -F p
>>> -Z 0?  Shall we say with plain format -Z 0 gets ignored or throw error
>>> or do something else?  If first, then I think it is better to mention
>>> the same in docs.
>>
>> ISTM that it's better to ignore that case, like pg_dump -Ft -Z0
>> doesn't throw an error.
>>
>
> Okay, then you can go ahead with your patch.

Thanks for the comment! I pushed the patch.

Regards,

-- 
Fujii Masao


-- 
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 wish list

[Amit Kapila]

On Thu, Jul 28, 2016 at 7:34 PM, Fujii Masao  wrote:
> On Thu, Jul 28, 2016 at 10:16 PM, Amit Kapila  wrote:
>
>> I think there is some value in providing
>> .tar for -Z 0,
>
> I was thinking that "-Ft -Z0" is something like an alias of "-Ft".
> That is, the backup is taken in uncompressed tar format.
>
>> however in that case how should we define usage of -F p
>> -Z 0?  Shall we say with plain format -Z 0 gets ignored or throw error
>> or do something else?  If first, then I think it is better to mention
>> the same in docs.
>
> ISTM that it's better to ignore that case, like pg_dump -Ft -Z0
> doesn't throw an error.
>

Okay, then you can go ahead with your patch.



--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
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 wish list

[Fujii Masao]

On Thu, Jul 28, 2016 at 10:16 PM, Amit Kapila  wrote:
> On Thu, Jul 28, 2016 at 5:37 PM, Fujii Masao  wrote:
>>
>> Maybe I failed to parse his proposal. It's helpful if you elaborate it.
>>
>
> As per mail [1], it seems the proposal is not to use .tar for -Z 0.

I was thinking that the proposal is "output uncompressed tar data,
and not add the ".gz" to the "base.tar" file name" part. So, if -Z 0 is
specified with tar format, .gz should not be added as a file extension.

> Now here actually we are on the fence, one can argue that if user
> doesn't want compression, he or she can use -F p (plain format).
> OTOH, without compression getting the backup as a single .tar file
> makes it simple to manage.

Right now we are providing both methods, plain and tar formats
(without compression, i.e., neither -z nor -Z options are specified).

> I think there is some value in providing
> .tar for -Z 0,

I was thinking that "-Ft -Z0" is something like an alias of "-Ft".
That is, the backup is taken in uncompressed tar format.

> however in that case how should we define usage of -F p
> -Z 0?  Shall we say with plain format -Z 0 gets ignored or throw error
> or do something else?  If first, then I think it is better to mention
> the same in docs.

ISTM that it's better to ignore that case, like pg_dump -Ft -Z0
doesn't throw an error.

Regards,

-- 
Fujii Masao


-- 
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 wish list

[Amit Kapila]

On Thu, Jul 28, 2016 at 5:37 PM, Fujii Masao  wrote:
>
> Maybe I failed to parse his proposal. It's helpful if you elaborate it.
>

As per mail [1], it seems the proposal is not to use .tar for -Z 0.
Now here actually we are on the fence, one can argue that if user
doesn't want compression, he or she can use -F p (plain format).
OTOH, without compression getting the backup as a single .tar file
makes it simple to manage.  I think there is some value in providing
.tar for -Z 0, however in that case how should we define usage of -F p
-Z 0?  Shall we say with plain format -Z 0 gets ignored or throw error
or do something else?  If first, then I think it is better to mention
the same in docs.

[1] - 
https://www.postgresql.org/message-id/CAMkU%3D1zzj0et2x9fCqxMGJ6XP-FtMSUwtNQGwF01698FRWQ6uA%40mail.gmail.com
-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
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 wish list

[Fujii Masao]

On Thu, Jul 28, 2016 at 8:44 PM, Amit Kapila  wrote:
> On Tue, Jul 26, 2016 at 11:58 AM, Fujii Masao  wrote:
>> On Wed, Jul 13, 2016 at 3:06 AM, Jeff Janes  wrote:
>>> On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
>>>  wrote:
 On 7/12/16 12:53 PM, Jeff Janes wrote:
> The --help message for pg_basebackup says:
>
> -Z, --compress=0-9 compress tar output with given compression level
>
> But -Z0 is then rejected as 'invalid compression level "0"'.  The real
> docs do say 1-9, only the --help message has this bug.  Trivial patch
> attached.

 pg_dump --help and man page say it supports 0..9.  Maybe we should make
 that more consistent.
>>>
>>> pg_dump actually does support -Z0, though.  Well, sort of.  It outputs
>>> plain text.  Rather than plain text wrapped in some kind of dummy gzip
>>> header, which is what I had naively expected.
>>>
>>> Is that what -Z0 in pg_basebackup should do as well, just output
>>> uncompressed tar data, and not add the ".gz" to the "base.tar" file
>>> name?
>>
>> Yes, I think. What about the attached patch?
>>
>
> What if user tries to use -Z 0 with format as tar, won't it generate
> base.tar without any compression?

Yes, with -Z 0 -F t options, the patched version of pg_basebackup generate
base.tar without compression.

> I am not sure if that is what Jeff
> intends to say in his proposal.

Maybe I failed to parse his proposal. It's helpful if you elaborate it.

Regards,

-- 
Fujii Masao


-- 
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 wish list

[Amit Kapila]

On Tue, Jul 26, 2016 at 11:58 AM, Fujii Masao  wrote:
> On Wed, Jul 13, 2016 at 3:06 AM, Jeff Janes  wrote:
>> On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
>>  wrote:
>>> On 7/12/16 12:53 PM, Jeff Janes wrote:
 The --help message for pg_basebackup says:

 -Z, --compress=0-9 compress tar output with given compression level

 But -Z0 is then rejected as 'invalid compression level "0"'.  The real
 docs do say 1-9, only the --help message has this bug.  Trivial patch
 attached.
>>>
>>> pg_dump --help and man page say it supports 0..9.  Maybe we should make
>>> that more consistent.
>>
>> pg_dump actually does support -Z0, though.  Well, sort of.  It outputs
>> plain text.  Rather than plain text wrapped in some kind of dummy gzip
>> header, which is what I had naively expected.
>>
>> Is that what -Z0 in pg_basebackup should do as well, just output
>> uncompressed tar data, and not add the ".gz" to the "base.tar" file
>> name?
>
> Yes, I think. What about the attached patch?
>

What if user tries to use -Z 0 with format as tar, won't it generate
base.tar without any compression?  I am not sure if that is what Jeff
intends to say in his proposal.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
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 wish list

[Fujii Masao]

On Tue, Jul 26, 2016 at 3:28 PM, Fujii Masao  wrote:
> On Wed, Jul 13, 2016 at 3:06 AM, Jeff Janes  wrote:
>> On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
>>  wrote:
>>> On 7/12/16 12:53 PM, Jeff Janes wrote:
 The --help message for pg_basebackup says:

 -Z, --compress=0-9 compress tar output with given compression level

 But -Z0 is then rejected as 'invalid compression level "0"'.  The real
 docs do say 1-9, only the --help message has this bug.  Trivial patch
 attached.
>>>
>>> pg_dump --help and man page say it supports 0..9.  Maybe we should make
>>> that more consistent.
>>
>> pg_dump actually does support -Z0, though.  Well, sort of.  It outputs
>> plain text.  Rather than plain text wrapped in some kind of dummy gzip
>> header, which is what I had naively expected.
>>
>> Is that what -Z0 in pg_basebackup should do as well, just output
>> uncompressed tar data, and not add the ".gz" to the "base.tar" file
>> name?
>
> Yes, I think. What about the attached patch?


Barring any objection, I will commit this patch.

Regards,

-- 
Fujii Masao


-- 
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 wish list

[Fujii Masao]

On Wed, Jul 13, 2016 at 3:06 AM, Jeff Janes  wrote:
> On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
>  wrote:
>> On 7/12/16 12:53 PM, Jeff Janes wrote:
>>> The --help message for pg_basebackup says:
>>>
>>> -Z, --compress=0-9 compress tar output with given compression level
>>>
>>> But -Z0 is then rejected as 'invalid compression level "0"'.  The real
>>> docs do say 1-9, only the --help message has this bug.  Trivial patch
>>> attached.
>>
>> pg_dump --help and man page say it supports 0..9.  Maybe we should make
>> that more consistent.
>
> pg_dump actually does support -Z0, though.  Well, sort of.  It outputs
> plain text.  Rather than plain text wrapped in some kind of dummy gzip
> header, which is what I had naively expected.
>
> Is that what -Z0 in pg_basebackup should do as well, just output
> uncompressed tar data, and not add the ".gz" to the "base.tar" file
> name?

Yes, I think. What about the attached patch?

Regards,

-- 
Fujii Masao
*** a/doc/src/sgml/ref/pg_basebackup.sgml
--- b/doc/src/sgml/ref/pg_basebackup.sgml
***
*** 364,370  PostgreSQL documentation

 
  Enables gzip compression of tar file output, and specifies the
! compression level (1 through 9, 9 being best
  compression). Compression is only available when using the tar
  format.
 
--- 364,370 

 
  Enables gzip compression of tar file output, and specifies the
! compression level (0 through 9, 0 being no compression and 9 being best
  compression). Compression is only available when using the tar
  format.
 
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
***
*** 2073,2079  main(int argc, char **argv)
  break;
  			case 'Z':
  compresslevel = atoi(optarg);
! if (compresslevel <= 0 || compresslevel > 9)
  {
  	fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
  			progname, optarg);
--- 2073,2079 
  break;
  			case 'Z':
  compresslevel = atoi(optarg);
! if (compresslevel < 0 || compresslevel > 9)
  {
  	fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
  			progname, optarg);

-- 
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 wish list

[Amit Kapila]

On Tue, Jul 12, 2016 at 10:23 PM, Jeff Janes  wrote:
> I've been having some adventures with pg_basebackup lately, and had
> some suggestions based on those.
>
> The --help message for pg_basebackup says:
>
> -Z, --compress=0-9 compress tar output with given compression level
>
> But -Z0 is then rejected as 'invalid compression level "0"'.  The real
> docs do say 1-9, only the --help message has this bug.  Trivial patch
> attached.
>
> These ones I have not written code for yet:
>
> The progress reporting for pg_basebackup is pretty terse:
>
>  858117/7060099 kB (12%), 0/1 tablespace
>
> I think we should at least add a count-up timer showing the seconds it
> has been running.  I can always use my own stopwatch, but that is not
> very friendly and easy to forget to start.
>

Another possibility is to enhance -P option as -P sec, such that it
will display progress after ever 'sec' seconds.  Something like we
have for pgbench.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
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 wish list

[Masahiko Sawada]

On Wed, Jul 13, 2016 at 1:53 AM, Jeff Janes  wrote:
> I've been having some adventures with pg_basebackup lately, and had
> some suggestions based on those.

And what I think is pg_baseback never remove the directory specified
by -D option even if execution is failed. initdb command behaves so.
I think it's helpful for backup operation.

Regards,

Masahiko Sawada


-- 
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 wish list

[Kenneth Marshall]

On Tue, Jul 12, 2016 at 11:06:39AM -0700, Jeff Janes wrote:
> On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
>  wrote:
> > On 7/12/16 12:53 PM, Jeff Janes wrote:
> >> The --help message for pg_basebackup says:
> >>
> >> -Z, --compress=0-9 compress tar output with given compression level
> >>
> >> But -Z0 is then rejected as 'invalid compression level "0"'.  The real
> >> docs do say 1-9, only the --help message has this bug.  Trivial patch
> >> attached.
> >
> > pg_dump --help and man page say it supports 0..9.  Maybe we should make
> > that more consistent.
> 
> pg_dump actually does support -Z0, though.  Well, sort of.  It outputs
> plain text.  Rather than plain text wrapped in some kind of dummy gzip
> header, which is what I had naively expected.
> 
> Is that what -Z0 in pg_basebackup should do as well, just output
> uncompressed tar data, and not add the ".gz" to the "base.tar" file
> name?
> 
> Cheers,
> 
> Jeff
> 

Hi,

Yes, please support the no compression option. It can be useful in
situations where the bottleneck is the compression itself (quite
easily done with zlib based options, another plug for a higher
performance option).

Regards,
Ken


-- 
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 wish list

[Jeff Janes]

On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
 wrote:
> On 7/12/16 12:53 PM, Jeff Janes wrote:
>> The --help message for pg_basebackup says:
>>
>> -Z, --compress=0-9 compress tar output with given compression level
>>
>> But -Z0 is then rejected as 'invalid compression level "0"'.  The real
>> docs do say 1-9, only the --help message has this bug.  Trivial patch
>> attached.
>
> pg_dump --help and man page say it supports 0..9.  Maybe we should make
> that more consistent.

pg_dump actually does support -Z0, though.  Well, sort of.  It outputs
plain text.  Rather than plain text wrapped in some kind of dummy gzip
header, which is what I had naively expected.

Is that what -Z0 in pg_basebackup should do as well, just output
uncompressed tar data, and not add the ".gz" to the "base.tar" file
name?

Cheers,

Jeff


-- 
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 wish list

[Peter Eisentraut]

On 7/12/16 12:53 PM, Jeff Janes wrote:
> The --help message for pg_basebackup says:
> 
> -Z, --compress=0-9 compress tar output with given compression level
> 
> But -Z0 is then rejected as 'invalid compression level "0"'.  The real
> docs do say 1-9, only the --help message has this bug.  Trivial patch
> attached.

pg_dump --help and man page say it supports 0..9.  Maybe we should make
that more consistent.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

[HACKERS] pg_basebackup wish list

[Jeff Janes]

I've been having some adventures with pg_basebackup lately, and had
some suggestions based on those.

The --help message for pg_basebackup says:

-Z, --compress=0-9 compress tar output with given compression level

But -Z0 is then rejected as 'invalid compression level "0"'.  The real
docs do say 1-9, only the --help message has this bug.  Trivial patch
attached.

These ones I have not written code for yet:

The progress reporting for pg_basebackup is pretty terse:

 858117/7060099 kB (12%), 0/1 tablespace

I think we should at least add a count-up timer showing the seconds it
has been running.  I can always use my own stopwatch, but that is not
very friendly and easy to forget to start.

And maybe change the reporting units from kB to MB when the pre-scan
says the total size exceeds some threshold?  At the same limit
pg_size_pretty does?

If I use the verbose flag, then the progress message includes the name
of the file being written to by the client. However, in -Ft mode, this
is always "something/base.tar" (unless you are using tablespaces),
which is not terribly useful. Should it instead report the name of the
file being read on the server end?

When using pg_basebackup from the wrong version, the error message it
reports is pretty unhelpful:

pg_basebackup: could not initiate base backup: ERROR:  syntax error

Could we have a newer version of pg_basebackup capture that error and
inject a HINT, or is there a better solution for getting a better
error message?

Cheers,

Jeff


pg_basebackup_compress.patch
Description: Binary data

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