On Wed, Jan 4, 2017 at 10:43 AM, Magnus Hagander <mag...@hagander.net>
wrote:
>
>
> On Sun, Jan 1, 2017 at 12:47 AM, Michael Paquier <
> michael.paqu...@gmail.com> wrote:
>
>> On Sat, Dec 31, 2016 at 9:24 PM, Magnus Hagander <mag...@hagander.net>
>> wrote:
>> > On Tue, Dec 20, 2016 at 11:53 PM, Michael Paquier
>> > <michael.paqu...@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/
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"),
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
