Make pg_basebackup -x stream the default
Per some discussions with a number of different people at pgconfeu, here is
a patch that changes the default mode of pg_basebackup to be streaming the
wal, as this is what most users would want -- and those that don't want it
have to make other changes as well. Doing the "most safe" thing by default
is a good idea.
The new option "-x none" is provided to turn this off and get the previous
behavior back.
I will add this to the next (January) commitfest.
//Magnus
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachments:
basebackup_stream_default.patchtext/x-patch; charset=US-ASCII; name=basebackup_stream_default.patchDownload
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 1f15a17..0db0ffb 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -302,10 +302,10 @@ PostgreSQL documentation
<para>
Includes the required transaction log files (WAL files) in the
backup. This will include all transaction logs generated during
- the backup. If this option is specified, it is possible to start
- a postmaster directly in the extracted directory without the need
- to consult the log archive, thus making this a completely standalone
- backup.
+ the backup. Unless the option <literal>none</literal> is specified,
+ it is possible to start a postmaster directly in the extracted
+ directory without the need to consult the log archive, thus
+ making this a completely standalone backup.
</para>
<para>
The following methods for collecting the transaction logs are
@@ -313,6 +313,16 @@ PostgreSQL documentation
<variablelist>
<varlistentry>
+ <term><literal>n</literal></term>
+ <term><literal>none</literal></term>
+ <listitem>
+ <para>
+ Don't include transaction log in the backup.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>f</literal></term>
<term><literal>fetch</literal></term>
<listitem>
@@ -349,6 +359,9 @@ PostgreSQL documentation
named <filename>pg_wal.tar</filename> (if the server is a version
earlier than 10, the file will be named <filename>pg_xlog.tar</filename>).
</para>
+ <para>
+ This value is the default.
+ </para>
</listitem>
</varlistentry>
</variablelist>
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index e2875df..7e294d8 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -71,8 +71,9 @@ static bool noclean = false;
static bool showprogress = false;
static int verbose = 0;
static int compresslevel = 0;
-static bool includewal = false;
-static bool streamwal = false;
+static bool includewal = true;
+static bool streamwal = true;
+static bool set_walmode = false;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
@@ -326,7 +327,7 @@ usage(void)
printf(_(" -T, --tablespace-mapping=OLDDIR=NEWDIR\n"
" relocate tablespace in OLDDIR to NEWDIR\n"));
printf(_(" -x, --xlog include required WAL files in backup (fetch mode)\n"));
- printf(_(" -X, --xlog-method=fetch|stream\n"
+ printf(_(" -X, --xlog-method=none|fetch|stream\n"
" 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"));
@@ -1700,7 +1701,11 @@ BaseBackup(void)
*/
if (streamwal && !CheckServerVersionForStreaming(conn))
{
- /* Error message already written in CheckServerVersionForStreaming() */
+ /*
+ * Error message already written in CheckServerVersionForStreaming(),
+ * but add a hint about using -X none.
+ */
+ fprintf(stderr, _("HINT: use -X none or -X fetch to disable log streaming\n"));
disconnect_and_exit(1);
}
@@ -2112,7 +2117,7 @@ main(int argc, char **argv)
tablespace_list_append(optarg);
break;
case 'x':
- if (includewal)
+ if (set_walmode)
{
fprintf(stderr,
_("%s: cannot specify both --xlog and --xlog-method\n"),
@@ -2120,11 +2125,12 @@ main(int argc, char **argv)
exit(1);
}
+ set_walmode = true;
includewal = true;
streamwal = false;
break;
case 'X':
- if (includewal)
+ if (set_walmode)
{
fprintf(stderr,
_("%s: cannot specify both --xlog and --xlog-method\n"),
@@ -2132,13 +2138,30 @@ main(int argc, char **argv)
exit(1);
}
- includewal = true;
- if (strcmp(optarg, "f") == 0 ||
+ /*
+ * Indicate that this has been configured, so we can complain
+ * if it's configured more than once.
+ */
+ set_walmode = true;
+
+ if (strcmp(optarg, "n") == 0 ||
+ strcmp(optarg, "none") == 0)
+ {
+ includewal = false;
+ streamwal = false;
+ }
+ else if (strcmp(optarg, "f") == 0 ||
strcmp(optarg, "fetch") == 0)
+ {
+ includewal = true;
streamwal = false;
+ }
else if (strcmp(optarg, "s") == 0 ||
strcmp(optarg, "stream") == 0)
+ {
+ includewal = true;
streamwal = true;
+ }
else
{
fprintf(stderr,
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 7811093..4c6670c 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 Cwd;
use Config;
use PostgresNode;
use TestLib;
-use Test::More tests => 69;
+use Test::More tests => 71;
program_help_ok('pg_basebackup');
program_version_ok('pg_basebackup');
@@ -63,7 +63,7 @@ foreach my $filename (qw(backup_label tablespace_map postgresql.auto.conf.tmp))
close FILE;
}
-$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backup" ],
+$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backup", '-X', 'none' ],
'pg_basebackup runs');
ok(-f "$tempdir/backup/PG_VERSION", 'backup was created');
@@ -225,6 +225,11 @@ like(
qr/^primary_conninfo = '.*port=$port.*'\n/m,
'recovery.conf sets primary_conninfo');
+$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backupxd" ],
+ 'pg_basebackup runs in default xlog mode');
+ok(grep(/^[0-9A-F]{24}$/, slurp_dir("$tempdir/backupxd/pg_wal")),
+ 'WAL files copied');
+
$node->command_ok(
[ 'pg_basebackup', '-D', "$tempdir/backupxf", '-X', 'fetch' ],
'pg_basebackup -X fetch runs');
Summary
=======
Thank you for submission! I think it needs a bit more work to be even
better.
Please deal with '-x' argument and with wording in documentation.
I'll set status to 'waiting on author' now.
Submission review
==============
Patch is in correct format.
Patch applies cleanly on HEAD.
Tests pass. There seems to be sufficient amount of tests to cover a change.
Usability review
============
Patch sounds like a good idea and does what it supposed to do. /me in DBA
hat will be happy to have it.
However, it makes '-x' parameter a bit confusing/surprising: specifying it
will be equivalent to '-X fetch' which is surprising regression from the
new default.
I think we need to either change -x to be equivalent to '-X streaming' or
just get rid of it altogether.
Feature test
===========
I've tested the change manually by doing pg_basebackup with -X none, -X
streaming and -X fetch and their shorthand variants, each with
max_wal_senders sent to 1 and 2.
I've got all the expected results/failures (e.g. -X streaming fails when
max_wal_senders set to 1).
Regression tests pass.
Coding review
===========
One comment about docs:
Includes the required transaction log files (WAL files) in the
backup. This will include all transaction logs generated during
- the backup. If this option is specified, it is possible to start
- a postmaster directly in the extracted directory without the need
- to consult the log archive, thus making this a completely
standalone
- backup.
+ the backup. Unless the option <literal>none</literal> is specified,
+ it is possible to start a postmaster directly in the extracted
+ directory without the need to consult the log archive, thus
+ making this a completely standalone backup.
</para>
<para>
I suggest "method <literal>none</literal>" instead of "option
<literal>none</literal>". I found the word "option" confusing in that
sentence.
--
Vladimir Rusinov
Storage SRE, Google Ireland
Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland
Registered in Dublin, Ireland
Registration Number: 368047
On Tue, Nov 8, 2016 at 5:45 PM, Magnus Hagander <magnus@hagander.net> wrote:
Show quoted text
Per some discussions with a number of different people at pgconfeu, here
is a patch that changes the default mode of pg_basebackup to be streaming
the wal, as this is what most users would want -- and those that don't want
it have to make other changes as well. Doing the "most safe" thing by
default is a good idea.The new option "-x none" is provided to turn this off and get the previous
behavior back.I will add this to the next (January) commitfest.
//Magnus
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
On 11/8/16 12:45 PM, Magnus Hagander wrote:
Per some discussions with a number of different people at pgconfeu, here
is a patch that changes the default mode of pg_basebackup to be
streaming the wal, as this is what most users would want -- and those
that don't want it have to make other changes as well. Doing the "most
safe" thing by default is a good idea.The new option "-x none" is provided to turn this off and get the
previous behavior back.
I would have expected that the "stream" method would become the default.
Just a short while ago it was proposed to remove the "fetch" method
altogether, and it was only kept because of some niche use cases. I
don't think "fetch" is the most safe method, because it requires
wal_keep_segments to be configured.
--
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, Dec 15, 2016 at 7:23 PM, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:
On 11/8/16 12:45 PM, Magnus Hagander wrote:
Per some discussions with a number of different people at pgconfeu, here
is a patch that changes the default mode of pg_basebackup to be
streaming the wal, as this is what most users would want -- and those
that don't want it have to make other changes as well. Doing the "most
safe" thing by default is a good idea.The new option "-x none" is provided to turn this off and get the
previous behavior back.I would have expected that the "stream" method would become the default.
Just a short while ago it was proposed to remove the "fetch" method
altogether, and it was only kept because of some niche use cases. I
don't think "fetch" is the most safe method, because it requires
wal_keep_segments to be configured.
Eh. Yes. That's exactly what this patch does, is it not?
I'd say the main reason fetch was kept around was to support tar file mode,
which previously did not support streaming. I don't really see any other
usecase where it was better than stream.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On 12/15/2016 10:23 AM, Peter Eisentraut wrote:
On 11/8/16 12:45 PM, Magnus Hagander wrote:
Per some discussions with a number of different people at pgconfeu, here
is a patch that changes the default mode of pg_basebackup to be
streaming the wal, as this is what most users would want -- and those
that don't want it have to make other changes as well. Doing the "most
safe" thing by default is a good idea.The new option "-x none" is provided to turn this off and get the
previous behavior back.I would have expected that the "stream" method would become the default.
Just a short while ago it was proposed to remove the "fetch" method
altogether, and it was only kept because of some niche use cases. I
don't think "fetch" is the most safe method, because it requires
wal_keep_segments to be configured.
IMO, if you are using fetch just using archive_command. Let's rip it out
of pg_basebackup or at least deprecate it.
jD
--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, opinions are my own.
--
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, Dec 15, 2016 at 7:36 PM, Joshua D. Drake <jd@commandprompt.com>
wrote:
On 12/15/2016 10:23 AM, Peter Eisentraut wrote:
On 11/8/16 12:45 PM, Magnus Hagander wrote:
Per some discussions with a number of different people at pgconfeu, here
is a patch that changes the default mode of pg_basebackup to be
streaming the wal, as this is what most users would want -- and those
that don't want it have to make other changes as well. Doing the "most
safe" thing by default is a good idea.The new option "-x none" is provided to turn this off and get the
previous behavior back.I would have expected that the "stream" method would become the default.
Just a short while ago it was proposed to remove the "fetch" method
altogether, and it was only kept because of some niche use cases. I
don't think "fetch" is the most safe method, because it requires
wal_keep_segments to be configured.IMO, if you are using fetch just using archive_command. Let's rip it out
of pg_basebackup or at least deprecate it.
Fetch runs fine without ssh keys. Especially important on platforms without
native ssh. It's also pull rather than push. And it fetches just the xlog
you need, and not *all* of it, for one-off backups. I think it definitely
still has it's uses, but should of course not be the default.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On 12/15/16 1:27 PM, Magnus Hagander wrote:
On Thu, Dec 15, 2016 at 7:23 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com
<mailto:peter.eisentraut@2ndquadrant.com>> wrote:On 11/8/16 12:45 PM, Magnus Hagander wrote:
Per some discussions with a number of different people at pgconfeu, here
is a patch that changes the default mode of pg_basebackup to be
streaming the wal, as this is what most users would want -- and those
that don't want it have to make other changes as well. Doing the "most
safe" thing by default is a good idea.The new option "-x none" is provided to turn this off and get the
previous behavior back.I would have expected that the "stream" method would become the default.
Just a short while ago it was proposed to remove the "fetch" method
altogether, and it was only kept because of some niche use cases. I
don't think "fetch" is the most safe method, because it requires
wal_keep_segments to be configured.Eh. Yes. That's exactly what this patch does, is it not?
Yes, I misread the patch. Works for me, then.
--
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, Dec 15, 2016 at 12:37 AM, Vladimir Rusinov <vrusinov@google.com>
wrote:
Usability review
============
Patch sounds like a good idea and does what it supposed to do. /me in DBA
hat will be happy to have it.However, it makes '-x' parameter a bit confusing/surprising: specifying it
will be equivalent to '-X fetch' which is surprising regression from the
new default.
This seems like a good idea, really.
Given that we already break a number of other things around backups and
replication in this release, it seems like a good time.
I definitely think removing it is what we should do -- let's not redefine
it to mean streaming, let's just get rid of -x altogether, and have people
use -X streaming|fetch|none.
What do others feel about this?
One comment about docs:
Includes the required transaction log files (WAL files) in the
backup. This will include all transaction logs generated during
- the backup. If this option is specified, it is possible to start
- a postmaster directly in the extracted directory without the need
- to consult the log archive, thus making this a completely
standalone- backup.
+ the backup. Unless the option <literal>none</literal> is
specified,+ it is possible to start a postmaster directly in the extracted
+ directory without the need to consult the log archive, thus
+ making this a completely standalone backup.
</para>
<para>
I suggest "method <literal>none</literal>" instead of "option
<literal>none</literal>". I found the word "option" confusing in that
sentence.
Sounds reasonable, will fix.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Fri, Dec 16, 2016 at 11:36 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Thu, Dec 15, 2016 at 12:37 AM, Vladimir Rusinov <vrusinov@google.com>
wrote:Usability review
============
Patch sounds like a good idea and does what it supposed to do. /me in DBA
hat will be happy to have it.However, it makes '-x' parameter a bit confusing/surprising: specifying it
will be equivalent to '-X fetch' which is surprising regression from the new
default.This seems like a good idea, really.
Given that we already break a number of other things around backups and
replication in this release, it seems like a good time.I definitely think removing it is what we should do -- let's not redefine it
to mean streaming, let's just get rid of -x altogether, and have people use
-X streaming|fetch|none.What do others feel about this?
+1 to drop -x option. That's less confusing.
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 Fri, Dec 16, 2016 at 6:35 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Fri, Dec 16, 2016 at 11:36 PM, Magnus Hagander <magnus@hagander.net>
wrote:On Thu, Dec 15, 2016 at 12:37 AM, Vladimir Rusinov <vrusinov@google.com>
wrote:Usability review
============
Patch sounds like a good idea and does what it supposed to do. /me in
DBA
hat will be happy to have it.
However, it makes '-x' parameter a bit confusing/surprising: specifying
it
will be equivalent to '-X fetch' which is surprising regression from
the new
default.
This seems like a good idea, really.
Given that we already break a number of other things around backups and
replication in this release, it seems like a good time.I definitely think removing it is what we should do -- let's not
redefine it
to mean streaming, let's just get rid of -x altogether, and have people
use
-X streaming|fetch|none.
What do others feel about this?
+1 to drop -x option. That's less confusing.
Attached is an updated patch that does this. As a bonus it simplifies the
code a bit. I also fixed an error message that I missed updating in the
previous patch.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachments:
basebackup_stream_default.patchtext/x-patch; charset=US-ASCII; name=basebackup_stream_default.patchDownload
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 1f15a17..b6381ea 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -88,7 +88,7 @@ PostgreSQL documentation
There is no guarantee that all WAL files required for the backup are archived
at the end of backup. If you are planning to use the backup for an archive
recovery and want to ensure that all required files are available at that moment,
- you need to include them into the backup by using the <literal>-x</> option.
+ you need to include them into the backup by using the <literal>-X</> option.
</para>
</listitem>
<listitem>
@@ -285,27 +285,16 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
- <term><option>-x</option></term>
- <term><option>--xlog</option></term>
- <listitem>
- <para>
- Using this option is equivalent of using <literal>-X</literal> with
- method <literal>fetch</literal>.
- </para>
- </listitem>
- </varlistentry>
-
- <varlistentry>
<term><option>-X <replaceable class="parameter">method</replaceable></option></term>
<term><option>--xlog-method=<replaceable class="parameter">method</replaceable></option></term>
<listitem>
<para>
Includes the required transaction log files (WAL files) in the
backup. This will include all transaction logs generated during
- the backup. If this option is specified, it is possible to start
- a postmaster directly in the extracted directory without the need
- to consult the log archive, thus making this a completely standalone
- backup.
+ the backup. Unless the method <literal>none</literal> is specified,
+ it is possible to start a postmaster directly in the extracted
+ directory without the need to consult the log archive, thus
+ making this a completely standalone backup.
</para>
<para>
The following methods for collecting the transaction logs are
@@ -313,6 +302,16 @@ PostgreSQL documentation
<variablelist>
<varlistentry>
+ <term><literal>n</literal></term>
+ <term><literal>none</literal></term>
+ <listitem>
+ <para>
+ Don't include transaction log in the backup.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>f</literal></term>
<term><literal>fetch</literal></term>
<listitem>
@@ -349,6 +348,9 @@ PostgreSQL documentation
named <filename>pg_wal.tar</filename> (if the server is a version
earlier than 10, the file will be named <filename>pg_xlog.tar</filename>).
</para>
+ <para>
+ This value is the default.
+ </para>
</listitem>
</varlistentry>
</variablelist>
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index f7ba9a9..459f36d 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -71,8 +71,8 @@ static bool noclean = false;
static bool showprogress = false;
static int verbose = 0;
static int compresslevel = 0;
-static bool includewal = false;
-static bool streamwal = false;
+static bool includewal = true;
+static bool streamwal = true;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
@@ -325,8 +325,7 @@ usage(void)
printf(_(" -S, --slot=SLOTNAME replication slot to use\n"));
printf(_(" -T, --tablespace-mapping=OLDDIR=NEWDIR\n"
" relocate tablespace in OLDDIR to NEWDIR\n"));
- printf(_(" -x, --xlog include required WAL files in backup (fetch mode)\n"));
- printf(_(" -X, --xlog-method=fetch|stream\n"
+ printf(_(" -X, --xlog-method=none|fetch|stream\n"
" 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"));
@@ -1700,7 +1699,11 @@ BaseBackup(void)
*/
if (streamwal && !CheckServerVersionForStreaming(conn))
{
- /* Error message already written in CheckServerVersionForStreaming() */
+ /*
+ * Error message already written in CheckServerVersionForStreaming(),
+ * but add a hint about using -X none.
+ */
+ fprintf(stderr, _("HINT: use -X none or -X fetch to disable log streaming\n"));
disconnect_and_exit(1);
}
@@ -2035,7 +2038,6 @@ main(int argc, char **argv)
{"write-recovery-conf", no_argument, NULL, 'R'},
{"slot", required_argument, NULL, 'S'},
{"tablespace-mapping", required_argument, NULL, 'T'},
- {"xlog", no_argument, NULL, 'x'},
{"xlog-method", required_argument, NULL, 'X'},
{"gzip", no_argument, NULL, 'z'},
{"compress", required_argument, NULL, 'Z'},
@@ -2111,38 +2113,29 @@ main(int argc, char **argv)
case 'T':
tablespace_list_append(optarg);
break;
- case 'x':
- if (includewal)
- {
- fprintf(stderr,
- _("%s: cannot specify both --xlog and --xlog-method\n"),
- progname);
- exit(1);
- }
-
- includewal = true;
- streamwal = false;
- break;
case 'X':
- if (includewal)
+ if (strcmp(optarg, "n") == 0 ||
+ strcmp(optarg, "none") == 0)
{
- fprintf(stderr,
- _("%s: cannot specify both --xlog and --xlog-method\n"),
- progname);
- exit(1);
+ includewal = false;
+ streamwal = false;
}
-
- includewal = true;
- if (strcmp(optarg, "f") == 0 ||
+ else if (strcmp(optarg, "f") == 0 ||
strcmp(optarg, "fetch") == 0)
+ {
+ includewal = true;
streamwal = false;
+ }
else if (strcmp(optarg, "s") == 0 ||
strcmp(optarg, "stream") == 0)
+ {
+ includewal = true;
streamwal = true;
+ }
else
{
fprintf(stderr,
- _("%s: invalid xlog-method option \"%s\", must be \"fetch\" or \"stream\"\n"),
+ _("%s: invalid xlog-method option \"%s\", must be \"fetch\", \"stream\" or \"none\"\n"),
progname, optarg);
exit(1);
}
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 7811093..4c6670c 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 Cwd;
use Config;
use PostgresNode;
use TestLib;
-use Test::More tests => 69;
+use Test::More tests => 71;
program_help_ok('pg_basebackup');
program_version_ok('pg_basebackup');
@@ -63,7 +63,7 @@ foreach my $filename (qw(backup_label tablespace_map postgresql.auto.conf.tmp))
close FILE;
}
-$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backup" ],
+$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backup", '-X', 'none' ],
'pg_basebackup runs');
ok(-f "$tempdir/backup/PG_VERSION", 'backup was created');
@@ -225,6 +225,11 @@ like(
qr/^primary_conninfo = '.*port=$port.*'\n/m,
'recovery.conf sets primary_conninfo');
+$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backupxd" ],
+ 'pg_basebackup runs in default xlog mode');
+ok(grep(/^[0-9A-F]{24}$/, slurp_dir("$tempdir/backupxd/pg_wal")),
+ 'WAL files copied');
+
$node->command_ok(
[ 'pg_basebackup', '-D', "$tempdir/backupxf", '-X', 'fetch' ],
'pg_basebackup -X fetch runs');
On Sat, Dec 17, 2016 at 2:37 PM, Magnus Hagander <magnus@hagander.net>
wrote:
Attached is an updated patch that does this. As a bonus it simplifies the
code a bit. I also fixed an error message that I missed updating in the
previous patch.
looks good to me.
Still applies cleanly at head with all tests pass.
I have no further comments, although since I'm not experienced Postgres
reviewer, commiter may want to take another look before merging.
--
Vladimir Rusinov
Storage SRE, Google Ireland
Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland
Registered in Dublin, Ireland
Registration Number: 368047
Attachments:
On Mon, Dec 19, 2016 at 7:51 PM, Vladimir Rusinov <vrusinov@google.com> wrote:
On Sat, Dec 17, 2016 at 2:37 PM, Magnus Hagander <magnus@hagander.net>
wrote:Attached is an updated patch that does this. As a bonus it simplifies the
code a bit. I also fixed an error message that I missed updating in the
previous patch.looks good to me.
Yep, basically looks good to me. Here are some small comments.
while ((c = getopt_long(argc, argv, "D:F:r:RT:xX:l:nNzZ:d:c:h:p:U:s:S:wWvP",
long_options, &option_index)) != -1)
'x' should be removed from the above code.
To create a backup of a single-tablespace local database and compress this with bzip2:
$ pg_basebackup -D - -Ft | bzip2 > backup.tar.bz2
The above example in the docs needs to be modified. For example,
add "-X fetch" into the above command so that pg_basebackup
should not exit with an error.
The server must also be configured with max_wal_senders set high
enough to leave at least one session available for the backup.
I think that it's better to explain explicitly here that max_wal_senders
should be higher than one by default.
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, Dec 20, 2016 at 10:38 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Dec 19, 2016 at 7:51 PM, Vladimir Rusinov <vrusinov@google.com> wrote:
The server must also be configured with max_wal_senders set high
enough to leave at least one session available for the backup.I think that it's better to explain explicitly here that max_wal_senders
should be higher than one by default.
Recovery tests are broken by this patch, the backup() method in
PostgresNode.pm uses pg_basebackup -x:
sub backup
{
my ($self, $backup_name) = @_;
my $backup_path = $self->backup_dir . '/' . $backup_name;
my $port = $self->port;
my $name = $self->name;
print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-p', $port,
'-x', '--no-sync');
print "# Backup finished\n";
}
--
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 Tue, Dec 20, 2016 at 2:38 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Dec 19, 2016 at 7:51 PM, Vladimir Rusinov <vrusinov@google.com>
wrote:On Sat, Dec 17, 2016 at 2:37 PM, Magnus Hagander <magnus@hagander.net>
wrote:Attached is an updated patch that does this. As a bonus it simplifies
the
code a bit. I also fixed an error message that I missed updating in the
previous patch.looks good to me.
Yep, basically looks good to me. Here are some small comments.
while ((c = getopt_long(argc, argv, "D:F:r:RT:xX:l:nNzZ:d:c:h:p:U:
s:S:wWvP",
long_options, &option_index)) != -1)
'x' should be removed from the above code.
Could've sworn I did that, but clearly not. Thanks.
To create a backup of a single-tablespace local database and compress
this with bzip2:
$ pg_basebackup -D - -Ft | bzip2 > backup.tar.bz2
The above example in the docs needs to be modified. For example,
add "-X fetch" into the above command so that pg_basebackup
should not exit with an error.
Ah yes, good point as well.
The server must also be configured with max_wal_senders set high
enough to leave at least one session available for the backup.I think that it's better to explain explicitly here that max_wal_senders
should be higher than one by default.
Added:
The server must also be configured
with <xref linkend="guc-max-wal-senders"> set high enough to leave at
least
one session available for the backup and one for WAL streaming (if used).
Will post new patch shortly, once I've addressed Michaels comments as well.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Tue, Dec 20, 2016 at 11:53 PM, Michael Paquier <michael.paquier@gmail.com
wrote:
On Tue, Dec 20, 2016 at 10:38 PM, Fujii Masao <masao.fujii@gmail.com>
wrote:On Mon, Dec 19, 2016 at 7:51 PM, Vladimir Rusinov <vrusinov@google.com>
wrote:
The server must also be configured with max_wal_senders set high
enough to leave at least one session available for the backup.I think that it's better to explain explicitly here that max_wal_senders
should be higher than one by default.Recovery tests are broken by this patch, the backup() method in
PostgresNode.pm uses pg_basebackup -x:
sub backup
{
my ($self, $backup_name) = @_;
my $backup_path = $self->backup_dir . '/' . $backup_name;
my $port = $self->port;
my $name = $self->name;print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-p',
$port,
'-x', '--no-sync');
print "# Backup finished\n";
}
Oh bleh. That's what I get for just running the tests for pg_basebackup
itself.
I removed it completely in this patch, making it use streaming. Or is there
a particular reason we want it to use fetch, that I'm not aware of?
Attached is a new patch with this fix, and those suggested by Fujii as well.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachments:
basebackup_stream_default_3.patchtext/x-patch; charset=US-ASCII; name=basebackup_stream_default_3.patchDownload
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 1f15a17..b22b410 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -56,7 +56,7 @@ PostgreSQL documentation
and <filename>pg_hba.conf</filename> must explicitly permit the replication
connection. The server must also be configured
with <xref linkend="guc-max-wal-senders"> set high enough to leave at least
- one session available for the backup.
+ one session available for the backup and one for WAL streaming (if used).
</para>
<para>
@@ -88,7 +88,7 @@ PostgreSQL documentation
There is no guarantee that all WAL files required for the backup are archived
at the end of backup. If you are planning to use the backup for an archive
recovery and want to ensure that all required files are available at that moment,
- you need to include them into the backup by using the <literal>-x</> option.
+ you need to include them into the backup by using the <literal>-X</> option.
</para>
</listitem>
<listitem>
@@ -285,27 +285,16 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
- <term><option>-x</option></term>
- <term><option>--xlog</option></term>
- <listitem>
- <para>
- Using this option is equivalent of using <literal>-X</literal> with
- method <literal>fetch</literal>.
- </para>
- </listitem>
- </varlistentry>
-
- <varlistentry>
<term><option>-X <replaceable class="parameter">method</replaceable></option></term>
<term><option>--xlog-method=<replaceable class="parameter">method</replaceable></option></term>
<listitem>
<para>
Includes the required transaction log files (WAL files) in the
backup. This will include all transaction logs generated during
- the backup. If this option is specified, it is possible to start
- a postmaster directly in the extracted directory without the need
- to consult the log archive, thus making this a completely standalone
- backup.
+ the backup. Unless the method <literal>none</literal> is specified,
+ it is possible to start a postmaster directly in the extracted
+ directory without the need to consult the log archive, thus
+ making this a completely standalone backup.
</para>
<para>
The following methods for collecting the transaction logs are
@@ -313,6 +302,16 @@ PostgreSQL documentation
<variablelist>
<varlistentry>
+ <term><literal>n</literal></term>
+ <term><literal>none</literal></term>
+ <listitem>
+ <para>
+ Don't include transaction log in the backup.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>f</literal></term>
<term><literal>fetch</literal></term>
<listitem>
@@ -349,6 +348,9 @@ PostgreSQL documentation
named <filename>pg_wal.tar</filename> (if the server is a version
earlier than 10, the file will be named <filename>pg_xlog.tar</filename>).
</para>
+ <para>
+ This value is the default.
+ </para>
</listitem>
</varlistentry>
</variablelist>
@@ -699,7 +701,7 @@ PostgreSQL documentation
To create a backup of a single-tablespace local database and compress
this with <productname>bzip2</productname>:
<screen>
-<prompt>$</prompt> <userinput>pg_basebackup -D - -Ft | bzip2 > backup.tar.bz2</userinput>
+<prompt>$</prompt> <userinput>pg_basebackup -D - -Ft -X fetch | bzip2 > backup.tar.bz2</userinput>
</screen>
(This command will fail if there are multiple tablespaces in the
database.)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 79b899a..3f79f7b 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -71,8 +71,8 @@ static bool noclean = false;
static bool showprogress = false;
static int verbose = 0;
static int compresslevel = 0;
-static bool includewal = false;
-static bool streamwal = false;
+static bool includewal = true;
+static bool streamwal = true;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
@@ -325,8 +325,7 @@ usage(void)
printf(_(" -S, --slot=SLOTNAME replication slot to use\n"));
printf(_(" -T, --tablespace-mapping=OLDDIR=NEWDIR\n"
" relocate tablespace in OLDDIR to NEWDIR\n"));
- printf(_(" -x, --xlog include required WAL files in backup (fetch mode)\n"));
- printf(_(" -X, --xlog-method=fetch|stream\n"
+ printf(_(" -X, --xlog-method=none|fetch|stream\n"
" 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"));
@@ -1700,7 +1699,11 @@ BaseBackup(void)
*/
if (streamwal && !CheckServerVersionForStreaming(conn))
{
- /* Error message already written in CheckServerVersionForStreaming() */
+ /*
+ * Error message already written in CheckServerVersionForStreaming(),
+ * but add a hint about using -X none.
+ */
+ fprintf(stderr, _("HINT: use -X none or -X fetch to disable log streaming\n"));
disconnect_and_exit(1);
}
@@ -2035,7 +2038,6 @@ main(int argc, char **argv)
{"write-recovery-conf", no_argument, NULL, 'R'},
{"slot", required_argument, NULL, 'S'},
{"tablespace-mapping", required_argument, NULL, 'T'},
- {"xlog", no_argument, NULL, 'x'},
{"xlog-method", required_argument, NULL, 'X'},
{"gzip", no_argument, NULL, 'z'},
{"compress", required_argument, NULL, 'Z'},
@@ -2078,7 +2080,7 @@ main(int argc, char **argv)
atexit(cleanup_directories_atexit);
- while ((c = getopt_long(argc, argv, "D:F:r:RT:xX:l:nNzZ:d:c:h:p:U:s:S:wWvP",
+ while ((c = getopt_long(argc, argv, "D:F:r:RT:X:l:nNzZ:d:c:h:p:U:s:S:wWvP",
long_options, &option_index)) != -1)
{
switch (c)
@@ -2111,38 +2113,29 @@ main(int argc, char **argv)
case 'T':
tablespace_list_append(optarg);
break;
- case 'x':
- if (includewal)
- {
- fprintf(stderr,
- _("%s: cannot specify both --xlog and --xlog-method\n"),
- progname);
- exit(1);
- }
-
- includewal = true;
- streamwal = false;
- break;
case 'X':
- if (includewal)
+ if (strcmp(optarg, "n") == 0 ||
+ strcmp(optarg, "none") == 0)
{
- fprintf(stderr,
- _("%s: cannot specify both --xlog and --xlog-method\n"),
- progname);
- exit(1);
+ includewal = false;
+ streamwal = false;
}
-
- includewal = true;
- if (strcmp(optarg, "f") == 0 ||
+ else if (strcmp(optarg, "f") == 0 ||
strcmp(optarg, "fetch") == 0)
+ {
+ includewal = true;
streamwal = false;
+ }
else if (strcmp(optarg, "s") == 0 ||
strcmp(optarg, "stream") == 0)
+ {
+ includewal = true;
streamwal = true;
+ }
else
{
fprintf(stderr,
- _("%s: invalid xlog-method option \"%s\", must be \"fetch\" or \"stream\"\n"),
+ _("%s: invalid xlog-method option \"%s\", must be \"fetch\", \"stream\" or \"none\"\n"),
progname, optarg);
exit(1);
}
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 7811093..4c6670c 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 Cwd;
use Config;
use PostgresNode;
use TestLib;
-use Test::More tests => 69;
+use Test::More tests => 71;
program_help_ok('pg_basebackup');
program_version_ok('pg_basebackup');
@@ -63,7 +63,7 @@ foreach my $filename (qw(backup_label tablespace_map postgresql.auto.conf.tmp))
close FILE;
}
-$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backup" ],
+$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backup", '-X', 'none' ],
'pg_basebackup runs');
ok(-f "$tempdir/backup/PG_VERSION", 'backup was created');
@@ -225,6 +225,11 @@ like(
qr/^primary_conninfo = '.*port=$port.*'\n/m,
'recovery.conf sets primary_conninfo');
+$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backupxd" ],
+ 'pg_basebackup runs in default xlog mode');
+ok(grep(/^[0-9A-F]{24}$/, slurp_dir("$tempdir/backupxd/pg_wal")),
+ 'WAL files copied');
+
$node->command_ok(
[ 'pg_basebackup', '-D', "$tempdir/backupxf", '-X', 'fetch' ],
'pg_basebackup -X fetch runs');
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index c1b16ca..f3c38bc 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -484,7 +484,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', '--no-sync');
+ '--no-sync');
print "# Backup finished\n";
}
On Sat, Dec 31, 2016 at 9:24 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Tue, Dec 20, 2016 at 11:53 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:Recovery tests are broken by this patch, the backup() method in
PostgresNode.pm uses pg_basebackup -x:
sub backup
{
my ($self, $backup_name) = @_;
my $backup_path = $self->backup_dir . '/' . $backup_name;
my $port = $self->port;
my $name = $self->name;print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-p',
$port,
'-x', '--no-sync');
print "# Backup finished\n";
}Oh bleh. That's what I get for just running the tests for pg_basebackup
itself.I removed it completely in this patch, making it use streaming. Or is there
a particular reason we want it to use fetch, that I'm not aware of?
Not really. Both should be equivalent in the current tests. It may
actually be a good idea to add an argument in PostgresNode->backup to
define the WAL fetching method.
Attached is a new patch with this fix, and those suggested by Fujii as well.
- the backup. If this option is specified, it is possible to start
- a postmaster directly in the extracted directory without the need
- to consult the log archive, thus making this a completely standalone
- backup.
+ the backup. Unless the method <literal>none</literal> is specified,
+ it is possible to start a postmaster directly in the extracted
+ directory without the need to consult the log archive, thus
+ making this a completely standalone backup.
</para>
I find a bit weird to mention a method value in a paragraph before
listing them. Perhaps it should be a separate paragraph after the list
of methods available?-static bool includewal = false;
-static bool streamwal = false;
+static bool includewal = true;
+static bool streamwal = true;
Two booleans are used to define 3 states. It may be cleaner to use an
enum instead. The important point is that streaming WAL is enabled
only if "stream" method is used.Other than that the patch looks good to me. Tests pass.
--
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 31 December 2016 at 23:47, Michael Paquier <michael.paquier@gmail.com> wrote:
Other than that the patch looks good to me. Tests pass.
+1
--
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 Sun, Jan 1, 2017 at 12:47 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:
On Sat, Dec 31, 2016 at 9:24 PM, Magnus Hagander <magnus@hagander.net>
wrote:On Tue, Dec 20, 2016 at 11:53 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:Recovery tests are broken by this patch, the backup() method in
PostgresNode.pm uses pg_basebackup -x:
sub backup
{
my ($self, $backup_name) = @_;
my $backup_path = $self->backup_dir . '/' . $backup_name;
my $port = $self->port;
my $name = $self->name;print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-p',
$port,
'-x', '--no-sync');
print "# Backup finished\n";
}Oh bleh. That's what I get for just running the tests for pg_basebackup
itself.I removed it completely in this patch, making it use streaming. Or is
there
a particular reason we want it to use fetch, that I'm not aware of?
Not really. Both should be equivalent in the current tests. It may
actually be a good idea to add an argument in PostgresNode->backup to
define the WAL fetching method.Attached is a new patch with this fix, and those suggested by Fujii as
well.
- the backup. If this option is specified, it is possible to start - a postmaster directly in the extracted directory without the need - to consult the log archive, thus making this a completely standalone - backup. + the backup. Unless the method <literal>none</literal> is specified, + it is possible to start a postmaster directly in the extracted + directory without the need to consult the log archive, thus + making this a completely standalone backup. </para> I find a bit weird to mention a method value in a paragraph before listing them. Perhaps it should be a separate paragraph after the list of methods available?-static bool includewal = false; -static bool streamwal = false; +static bool includewal = true; +static bool streamwal = true; Two booleans are used to define 3 states. It may be cleaner to use an enum instead. The important point is that streaming WAL is enabled only if "stream" method is used.Other than that the patch looks good to me. Tests pass.
Meh, just as I was going to respond "committed" I noticed this second round
of review comments. Apologies, pushed without that.
I agree on the change with includewal/streamwal. That was already the case
in the existing code of course, but that doesn't mean it couldn't be made
better. I'll take a look at doing that as a separate patch.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Wed, Jan 4, 2017 at 10:43 AM, Magnus Hagander <magnus@hagander.net>
wrote:
On Sun, Jan 1, 2017 at 12:47 AM, Michael Paquier <
michael.paquier@gmail.com> wrote:On Sat, Dec 31, 2016 at 9:24 PM, Magnus Hagander <magnus@hagander.net>
wrote:On Tue, Dec 20, 2016 at 11:53 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:Recovery tests are broken by this patch, the backup() method in
PostgresNode.pm uses pg_basebackup -x:
sub backup
{
my ($self, $backup_name) = @_;
my $backup_path = $self->backup_dir . '/' . $backup_name;
my $port = $self->port;
my $name = $self->name;print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-p',
$port,
'-x', '--no-sync');
print "# Backup finished\n";
}Oh bleh. That's what I get for just running the tests for pg_basebackup
itself.I removed it completely in this patch, making it use streaming. Or is
there
a particular reason we want it to use fetch, that I'm not aware of?
Not really. Both should be equivalent in the current tests. It may
actually be a good idea to add an argument in PostgresNode->backup to
define the WAL fetching method.Attached is a new patch with this fix, and those suggested by Fujii as
well.
- the backup. If this option is specified, it is possible to start - a postmaster directly in the extracted directory without the need - to consult the log archive, thus making this a completely standalone - backup. + the backup. Unless the method <literal>none</literal> is specified, + it is possible to start a postmaster directly in the extracted + directory without the need to consult the log archive, thus + making this a completely standalone backup. </para> I find a bit weird to mention a method value in a paragraph before listing them. Perhaps it should be a separate paragraph after the list of methods available?-static bool includewal = false; -static bool streamwal = false; +static bool includewal = true; +static bool streamwal = true; Two booleans are used to define 3 states. It may be cleaner to use an enum instead. The important point is that streaming WAL is enabled only if "stream" method is used.Other than that the patch looks good to me. Tests pass.
Meh, just as I was going to respond "committed" I noticed this second
round of review comments. Apologies, pushed without that.I agree on the change with includewal/streamwal. That was already the case
in the existing code of course, but that doesn't mean it couldn't be made
better. I'll take a look at doing that as a separate patch.
Here's a patch that does this. Does this match what you were thinking?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachments:
pg_basebackup_enum.patchtext/x-patch; charset=US-ASCII; name=pg_basebackup_enum.patchDownload
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 3f83d87..8ebf24e 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -61,6 +61,16 @@ typedef struct TablespaceList
*/
#define MINIMUM_VERSION_FOR_PG_WAL 100000
+/*
+ * Different ways to include WAL
+ */
+typedef enum
+{
+ NO_WAL,
+ FETCH_WAL,
+ STREAM_WAL
+} IncludeWal;
+
/* Global options */
static char *basedir = NULL;
static TablespaceList tablespace_dirs = {NULL, NULL};
@@ -71,8 +81,7 @@ static bool noclean = false;
static bool showprogress = false;
static int verbose = 0;
static int compresslevel = 0;
-static bool includewal = true;
-static bool streamwal = true;
+static IncludeWal includewal = STREAM_WAL;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
@@ -1697,7 +1706,7 @@ BaseBackup(void)
* If WAL streaming was requested, also check that the server is new
* enough for that.
*/
- if (streamwal && !CheckServerVersionForStreaming(conn))
+ if (includewal == STREAM_WAL && !CheckServerVersionForStreaming(conn))
{
/*
* Error message already written in CheckServerVersionForStreaming(),
@@ -1731,9 +1740,9 @@ BaseBackup(void)
psprintf("BASE_BACKUP LABEL '%s' %s %s %s %s %s %s",
escaped_label,
showprogress ? "PROGRESS" : "",
- includewal && !streamwal ? "WAL" : "",
+ includewal == FETCH_WAL ? "WAL" : "",
fastcheckpoint ? "FAST" : "",
- includewal ? "NOWAIT" : "",
+ includewal == NO_WAL ? "" : "NOWAIT",
maxrate_clause ? maxrate_clause : "",
format == 't' ? "TABLESPACE_MAP" : "");
@@ -1776,7 +1785,7 @@ BaseBackup(void)
PQclear(res);
MemSet(xlogend, 0, sizeof(xlogend));
- if (verbose && includewal)
+ if (verbose && includewal != NO_WAL)
fprintf(stderr, _("transaction log start point: %s on timeline %u\n"),
xlogstart, starttli);
@@ -1833,7 +1842,7 @@ BaseBackup(void)
* If we're streaming WAL, start the streaming session before we start
* receiving the actual data chunks.
*/
- if (streamwal)
+ if (includewal == STREAM_WAL)
{
if (verbose)
fprintf(stderr, _("%s: starting background WAL receiver\n"),
@@ -1879,7 +1888,7 @@ BaseBackup(void)
disconnect_and_exit(1);
}
strlcpy(xlogend, PQgetvalue(res, 0, 0), sizeof(xlogend));
- if (verbose && includewal)
+ if (verbose && includewal != NO_WAL)
fprintf(stderr, "transaction log end point: %s\n", xlogend);
PQclear(res);
@@ -2117,20 +2126,17 @@ main(int argc, char **argv)
if (strcmp(optarg, "n") == 0 ||
strcmp(optarg, "none") == 0)
{
- includewal = false;
- streamwal = false;
+ includewal = NO_WAL;
}
else if (strcmp(optarg, "f") == 0 ||
strcmp(optarg, "fetch") == 0)
{
- includewal = true;
- streamwal = false;
+ includewal = FETCH_WAL;
}
else if (strcmp(optarg, "s") == 0 ||
strcmp(optarg, "stream") == 0)
{
- includewal = true;
- streamwal = true;
+ includewal = STREAM_WAL;
}
else
{
@@ -2261,7 +2267,7 @@ main(int argc, char **argv)
exit(1);
}
- if (format == 't' && streamwal && strcmp(basedir, "-") == 0)
+ if (format == 't' && includewal == STREAM_WAL && strcmp(basedir, "-") == 0)
{
fprintf(stderr,
_("%s: cannot stream transaction logs in tar mode to stdout\n"),
@@ -2271,7 +2277,7 @@ main(int argc, char **argv)
exit(1);
}
- if (replication_slot && !streamwal)
+ if (replication_slot && includewal != STREAM_WAL)
{
fprintf(stderr,
_("%s: replication slots can only be used with WAL streaming\n"),
On Sat, Jan 7, 2017 at 12:04 AM, Magnus Hagander <magnus@hagander.net> wrote:
On Wed, Jan 4, 2017 at 10:43 AM, Magnus Hagander <magnus@hagander.net>
wrote:Meh, just as I was going to respond "committed" I noticed this second
round of review comments. Apologies, pushed without that.I agree on the change with includewal/streamwal. That was already the case
in the existing code of course, but that doesn't mean it couldn't be made
better. I'll take a look at doing that as a separate patch.Here's a patch that does this. Does this match what you were thinking?
Yes, that's it. I have looked at the patch in details and that looks
correct to me.
--
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 Sat, Jan 7, 2017 at 1:32 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:
On Sat, Jan 7, 2017 at 12:04 AM, Magnus Hagander <magnus@hagander.net>
wrote:On Wed, Jan 4, 2017 at 10:43 AM, Magnus Hagander <magnus@hagander.net>
wrote:Meh, just as I was going to respond "committed" I noticed this second
round of review comments. Apologies, pushed without that.I agree on the change with includewal/streamwal. That was already the
case
in the existing code of course, but that doesn't mean it couldn't be
made
better. I'll take a look at doing that as a separate patch.
Here's a patch that does this. Does this match what you were thinking?
Yes, that's it. I have looked at the patch in details and that looks
correct to me.
OK. Pushed. I agree it made it more readable, if nothing else.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Tue, Jan 10, 2017 at 12:05 AM, Magnus Hagander <magnus@hagander.net> wrote:
OK. Pushed. I agree it made it more readable, if nothing else.
Thanks.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers