pg_basebackup wish list
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
Attachments:
pg_basebackup_compress.patchapplication/octet-stream; name=pg_basebackup_compress.patchDownload
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
new file mode 100644
index ec69682..cda5656
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
*************** usage(void)
*** 247,253 ****
" include required WAL files with specified method\n"));
printf(_(" --xlogdir=XLOGDIR location for the transaction log directory\n"));
printf(_(" -z, --gzip compress tar output\n"));
! printf(_(" -Z, --compress=0-9 compress tar output with given compression level\n"));
printf(_("\nGeneral options:\n"));
printf(_(" -c, --checkpoint=fast|spread\n"
" set fast or spread checkpointing\n"));
--- 247,253 ----
" include required WAL files with specified method\n"));
printf(_(" --xlogdir=XLOGDIR location for the transaction log directory\n"));
printf(_(" -z, --gzip compress tar output\n"));
! printf(_(" -Z, --compress=1-9 compress tar output with given compression level\n"));
printf(_("\nGeneral options:\n"));
printf(_(" -c, --checkpoint=fast|spread\n"
" set fast or spread checkpointing\n"));
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
On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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
On Tue, Jul 12, 2016 at 11:06:39AM -0700, Jeff Janes wrote:
On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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
On Wed, Jul 13, 2016 at 1:53 AM, Jeff Janes <jeff.janes@gmail.com> 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
On Tue, Jul 12, 2016 at 10:23 PM, Jeff Janes <jeff.janes@gmail.com> 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
On Wed, Jul 13, 2016 at 3:06 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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
Attachments:
basebackup_compression_level0.patchtext/x-patch; charset=US-ASCII; name=basebackup_compression_level0.patchDownload
*** a/doc/src/sgml/ref/pg_basebackup.sgml
--- b/doc/src/sgml/ref/pg_basebackup.sgml
***************
*** 364,370 **** PostgreSQL documentation
<listitem>
<para>
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.
</para>
--- 364,370 ----
<listitem>
<para>
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.
</para>
*** 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);
On Tue, Jul 26, 2016 at 3:28 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Jul 13, 2016 at 3:06 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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
On Tue, Jul 26, 2016 at 11:58 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Jul 13, 2016 at 3:06 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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
On Thu, Jul 28, 2016 at 8:44 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Jul 26, 2016 at 11:58 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Jul 13, 2016 at 3:06 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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
Import Notes
Resolved by subject fallback
On Thu, Jul 28, 2016 at 5:37 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Maybe I failed to parse his proposal. It's helpful if you elaborate it.
As per mail [1]/messages/by-id/CAMkU=1zzj0et2x9fCqxMGJ6XP-FtMSUwtNQGwF01698FRWQ6uA@mail.gmail.com -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com, 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]: /messages/by-id/CAMkU=1zzj0et2x9fCqxMGJ6XP-FtMSUwtNQGwF01698FRWQ6uA@mail.gmail.com -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.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
On Thu, Jul 28, 2016 at 10:16 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jul 28, 2016 at 5:37 PM, Fujii Masao <masao.fujii@gmail.com> 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
On Thu, Jul 28, 2016 at 7:34 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Jul 28, 2016 at 10:16 PM, Amit Kapila <amit.kapila16@gmail.com> 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
On Fri, Jul 29, 2016 at 11:01 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jul 28, 2016 at 7:34 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Jul 28, 2016 at 10:16 PM, Amit Kapila <amit.kapila16@gmail.com> 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
On Thu, Jul 28, 2016 at 4:44 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Jul 26, 2016 at 11:58 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Jul 13, 2016 at 3:06 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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
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
Attachments:
0001-pg_basebackup-Clean-created-directories-on-failure.patchtext/x-patch; name=0001-pg_basebackup-Clean-created-directories-on-failure.patchDownload
From 910310b7eab88af8972906307a55e02e64618da7 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
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 @@ <title>Options</title>
</varlistentry>
<varlistentry>
+ <term><option>-n</option></term>
+ <term><option>--noclean</option></term>
+ <listitem>
+ <para>
+ By default, when <command>pg_basebackup</command> 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.
+ </para>
+
+ <para>
+ Note that tablespace directories are not cleaned up either way.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-P</option></term>
<term><option>--progress</option></term>
<listitem>
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)
+ fprintf(stderr,
+ _("%s: data directory \"%s\" not removed at user's request\n"),
+ progname, basedir);
+
+ if (made_new_xlogdir || found_existing_xlogdir)
+ fprintf(stderr,
+ _("%s: transaction log directory \"%s\" not removed at user's request\n"),
+ progname, xlog_dir);
+ }
+
+ if (made_tablespace_dirs || found_tablespace_dirs)
+ fprintf(stderr,
+ _("%s: changes to tablespace directories will not be undone"),
+ progname);
+}
+
+static void
disconnect_and_exit(int code)
{
if (conn != NULL)
@@ -252,6 +324,7 @@ usage(void)
printf(_(" -c, --checkpoint=fast|spread\n"
" set fast or spread checkpointing\n"));
printf(_(" -l, --label=LABEL set backup label\n"));
+ printf(_(" -n, --noclean do not clean up after errors"));
printf(_(" -P, --progress show progress information\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -V, --version output version information, then exit\n"));
@@ -374,6 +447,8 @@ LogStreamerMain(logstreamer_param *param)
{
StreamCtl stream;
+ in_log_streamer = true;
+
MemSet(&stream, 0, sizeof(stream));
stream.startpos = param->startptr;
stream.timeline = param->timeline;
@@ -500,7 +575,7 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
* be give and the process ended.
*/
static void
-verify_dir_is_empty_or_create(char *dirname)
+verify_dir_is_empty_or_create(char *dirname, bool *created, bool *found)
{
switch (pg_check_dir(dirname))
{
@@ -516,12 +591,16 @@ verify_dir_is_empty_or_create(char *dirname)
progname, dirname, strerror(errno));
disconnect_and_exit(1);
}
+ if (created)
+ *created = true;
return;
case 1:
/*
* Exists, empty
*/
+ if (found)
+ *found = true;
return;
case 2:
case 3:
@@ -1746,7 +1825,7 @@ BaseBackup(void)
{
char *path = (char *) get_tablespace_mapping(PQgetvalue(res, i, 1));
- verify_dir_is_empty_or_create(path);
+ verify_dir_is_empty_or_create(path, &made_tablespace_dirs, &found_tablespace_dirs);
}
}
@@ -1955,6 +2034,7 @@ main(int argc, char **argv)
{"gzip", no_argument, NULL, 'z'},
{"compress", required_argument, NULL, 'Z'},
{"label", required_argument, NULL, 'l'},
+ {"noclean", no_argument, NULL, 'n'},
{"dbname", required_argument, NULL, 'd'},
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
@@ -1989,7 +2069,9 @@ main(int argc, char **argv)
}
}
- while ((c = getopt_long(argc, argv, "D:F:r:RT:xX:l:zZ:d:c:h:p:U:s:S:wWvP",
+ atexit(cleanup_directories_atexit);
+
+ while ((c = getopt_long(argc, argv, "D:F:r:RT:xX:l:nzZ:d:c:h:p:U:s:S:wWvP",
long_options, &option_index)) != -1)
{
switch (c)
@@ -2064,6 +2146,9 @@ main(int argc, char **argv)
case 'l':
label = pg_strdup(optarg);
break;
+ case 'n':
+ noclean = true;
+ break;
case 'z':
#ifdef HAVE_LIBZ
compresslevel = Z_DEFAULT_COMPRESSION;
@@ -2233,14 +2318,14 @@ main(int argc, char **argv)
* unless we are writing to stdout.
*/
if (format == 'p' || strcmp(basedir, "-") != 0)
- verify_dir_is_empty_or_create(basedir);
+ verify_dir_is_empty_or_create(basedir, &made_new_pgdata, &found_existing_pgdata);
/* Create transaction log symlink, if required */
if (strcmp(xlog_dir, "") != 0)
{
char *linkloc;
- verify_dir_is_empty_or_create(xlog_dir);
+ verify_dir_is_empty_or_create(xlog_dir, &made_new_xlogdir, &found_existing_xlogdir);
/* form name of the place where the symlink must go */
linkloc = psprintf("%s/pg_xlog", basedir);
@@ -2261,5 +2346,6 @@ main(int argc, char **argv)
BaseBackup();
+ success = true;
return 0;
}
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 6c33936..fd9857d 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -4,7 +4,7 @@
use Config;
use PostgresNode;
use TestLib;
-use Test::More tests => 51;
+use Test::More tests => 54;
program_help_ok('pg_basebackup');
program_version_ok('pg_basebackup');
@@ -40,6 +40,14 @@
[ 'pg_basebackup', '-D', "$tempdir/backup" ],
'pg_basebackup fails because of WAL configuration');
+ok(! -d "$tempdir/backup", 'backup directory was cleaned up');
+
+$node->command_fails(
+ [ 'pg_basebackup', '-D', "$tempdir/backup", '-n' ],
+ 'failing run with noclean option');
+
+ok(-d "$tempdir/backup", 'backup directory was created and left behind');
+
open CONF, ">>$pgdata/postgresql.conf";
print CONF "max_replication_slots = 10\n";
print CONF "max_wal_senders = 10\n";
--
2.9.3
On Fri, Aug 19, 2016 at 7:06 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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
On Fri, Aug 19, 2016 at 2:04 PM, Masahiko Sawada <sawada.mshk@gmail.com> 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
On 19 August 2016 at 08:46, Michael Paquier <michael.paquier@gmail.com> wrote:
On Fri, Aug 19, 2016 at 2:04 PM, Masahiko Sawada <sawada.mshk@gmail.com> 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 Riggs 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
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
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
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
On Wed, Sep 14, 2016 at 6:52 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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
Attachments:
tap-noclean.patchtext/x-diff; charset=US-ASCII; name=tap-noclean.patchDownload
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";
}
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
On Thu, Sep 15, 2016 at 9:26 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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