On Fri, Dec 16, 2016 at 6:35 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
> On Fri, Dec 16, 2016 at 11:36 PM, Magnus Hagander <mag...@hagander.net>
> wrote:
> > On Thu, Dec 15, 2016 at 12:37 AM, Vladimir Rusinov <vrusi...@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/
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');
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
