pg_basebackups and slots
I've started work on a patch to make pg_basebackup use the temporary slots
feature that has been committed (thanks Petr!!). The reason for this is to
avoid the cases where a burst of traffic on the master during the backup
can cause the receive log part of the basebackup to fall far enough behind
that it fails.
I have a few considerations at this point, about interaction with existing
options.
Right now, we have -S/--slot which specifies a slot name. If you want to
use that, you have to create the slot ahead of time, and it will be a
permanent slot (of course). This is primarily documented as a feature to
use for replication (to make sure xlog is kept around until the standby is
started up), but it does also solve the same problem. But to use it for
base backups today you have to manually create the slot, then base backup,
then drop the slot, which is error prone.
My thought is that if -S/--slot is not specified, but -X stream is, then we
use a temporary slot always. This obviously requires the server to be
configured with enough slots (I still think we should change the default
here, but that's a different topic), but I think that's acceptable. Then we
should add a "--no-slot" to make it revert to previous behaviour.
Does that seem reasonable? Or would people prefer it to default to off?
However, it also made me think that the interface for setting up a replica
with a *permanent* slot would be much nicer if we could just have
pg_basebackup create the slot, instead of having to create it manually.
Basically, I'm envisioning adding an IF_NOT_EXISTS argument to
CREATE_REPLICATION_SLOT, so that pg_basebackup can pass up the slot name it
wants to use and have it created if necessary. Do people think think this
is worth doing? (It would also make pg_receivexlog and pg_receivelogical
easier to use, I think, and it would obviously be implemented there as
well).
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Thu, Dec 15, 2016 at 10:04 AM, Magnus Hagander <magnus@hagander.net>
wrote:
I've started work on a patch to make pg_basebackup use the temporary slots
feature that has been committed (thanks Petr!!). The reason for this is to
avoid the cases where a burst of traffic on the master during the backup
can cause the receive log part of the basebackup to fall far enough behind
that it fails.I have a few considerations at this point, about interaction with existing
options.Right now, we have -S/--slot which specifies a slot name. If you want to
use that, you have to create the slot ahead of time, and it will be a
permanent slot (of course). This is primarily documented as a feature to
use for replication (to make sure xlog is kept around until the standby is
started up), but it does also solve the same problem. But to use it for
base backups today you have to manually create the slot, then base backup,
then drop the slot, which is error prone.My thought is that if -S/--slot is not specified, but -X stream is, then
we use a temporary slot always. This obviously requires the server to be
configured with enough slots (I still think we should change the default
here, but that's a different topic), but I think that's acceptable. Then we
should add a "--no-slot" to make it revert to previous behaviour.Does that seem reasonable? Or would people prefer it to default to off?
However, it also made me think that the interface for setting up a replica
with a *permanent* slot would be much nicer if we could just have
pg_basebackup create the slot, instead of having to create it manually.Basically, I'm envisioning adding an IF_NOT_EXISTS argument to
CREATE_REPLICATION_SLOT, so that pg_basebackup can pass up the slot name it
wants to use and have it created if necessary. Do people think think this
is worth doing? (It would also make pg_receivexlog and pg_receivelogical
easier to use, I think, and it would obviously be implemented there as
well).
Pah, nevermind this last question. I was looking a completely broken
branch -- we already have the if-not-exists parameter for
pg_receivexlog/pg_receivelogical. I shouldn't write emails before
thinking...
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Thu, Dec 15, 2016 at 10:26 AM, Magnus Hagander <magnus@hagander.net>
wrote:
On Thu, Dec 15, 2016 at 10:04 AM, Magnus Hagander <magnus@hagander.net>
wrote:I've started work on a patch to make pg_basebackup use the temporary
slots feature that has been committed (thanks Petr!!). The reason for this
is to avoid the cases where a burst of traffic on the master during the
backup can cause the receive log part of the basebackup to fall far enough
behind that it fails.I have a few considerations at this point, about interaction with
existing options.Right now, we have -S/--slot which specifies a slot name. If you want to
use that, you have to create the slot ahead of time, and it will be a
permanent slot (of course). This is primarily documented as a feature to
use for replication (to make sure xlog is kept around until the standby is
started up), but it does also solve the same problem. But to use it for
base backups today you have to manually create the slot, then base backup,
then drop the slot, which is error prone.My thought is that if -S/--slot is not specified, but -X stream is, then
we use a temporary slot always. This obviously requires the server to be
configured with enough slots (I still think we should change the default
here, but that's a different topic), but I think that's acceptable. Then we
should add a "--no-slot" to make it revert to previous behaviour.Does that seem reasonable? Or would people prefer it to default to off?
So here's a patch that does this, for discussion. It implements the
following behavior for -X:
* When used with <10.0 servers, behave just like before.
* When -S <name> is specified, behave just like before (use an existing
replication slot, fail if it does not exist)
* When used on 10.0 with no -S, create and use a temporary replication slot
while running, with name pg_basebackup_<pid>.
* When used with 10.0 with no -S but --no-slot specified, run without a
slot like before.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachments:
pg_basebackup_temp_slot.patchtext/x-patch; charset=US-ASCII; name=pg_basebackup_temp_slot.patchDownload
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 1f15a17..f912ed0 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -246,6 +246,20 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>--no-slot</option></term>
+ <listitem>
+ <para>
+ This option prevents the creation of a temporary replication slot
+ during the backup even if it's supported by the server.
+ </para>
+ <para>
+ Temporary replication slots are created by default if no slot name
+ is given with the <literal>-S</literal> when using log streaming.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-T <replaceable class="parameter">olddir</replaceable>=<replaceable class="parameter">newdir</replaceable></option></term>
<term><option>--tablespace-mapping=<replaceable class="parameter">olddir</replaceable>=<replaceable class="parameter">newdir</replaceable></option></term>
<listitem>
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index e2875df..85bc7ef 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -61,6 +61,11 @@ typedef struct TablespaceList
*/
#define MINIMUM_VERSION_FOR_PG_WAL 100000
+/*
+ * Temporary replication slots are supported from version 10.
+ */
+#define MINIMUM_VERSION_FOR_TEMP_SLOTS 100000
+
/* Global options */
static char *basedir = NULL;
static TablespaceList tablespace_dirs = {NULL, NULL};
@@ -79,6 +84,8 @@ static bool do_sync = true;
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 char *replication_slot = NULL;
+static bool temp_replication_slot = true;
static bool success = false;
static bool made_new_pgdata = false;
@@ -323,6 +330,7 @@ usage(void)
printf(_(" -R, --write-recovery-conf\n"
" write recovery.conf after backup\n"));
printf(_(" -S, --slot=SLOTNAME replication slot to use\n"));
+ printf(_(" --no-slot prevent creation of temporary replication slot\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"));
@@ -452,6 +460,7 @@ typedef struct
char xlog[MAXPGPATH]; /* directory or tarfile depending on mode */
char *sysidentifier;
int timeline;
+ bool temp_slot;
} logstreamer_param;
static int
@@ -471,6 +480,16 @@ LogStreamerMain(logstreamer_param *param)
stream.do_sync = do_sync;
stream.mark_done = true;
stream.partial_suffix = NULL;
+ stream.replication_slot = replication_slot;
+ stream.temp_slot = param->temp_slot;
+ if (stream.temp_slot && !stream.replication_slot)
+ {
+ /* Generate a name for the temporary slot */
+ char name[32];
+
+ snprintf(name, sizeof(name), "pg_basebackup_%d", getpid());
+ stream.replication_slot = pstrdup(name);
+ }
if (format == 'p')
stream.walmethod = CreateWalDirectoryMethod(param->xlog, do_sync);
@@ -557,6 +576,11 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
PQserverVersion(conn) < MINIMUM_VERSION_FOR_PG_WAL ?
"pg_xlog" : "pg_wal");
+ /* Temporary replication slots are only supported in 10 and newer */
+ if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_TEMP_SLOTS)
+ param->temp_slot = false;
+ else
+ param->temp_slot = temp_replication_slot;
if (format == 'p')
{
@@ -2052,11 +2076,13 @@ main(int argc, char **argv)
{"verbose", no_argument, NULL, 'v'},
{"progress", no_argument, NULL, 'P'},
{"xlogdir", required_argument, NULL, 1},
+ {"no-slot", no_argument, NULL, 2},
{NULL, 0, NULL, 0}
};
int c;
int option_index;
+ bool no_slot = false;
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
@@ -2106,7 +2132,16 @@ main(int argc, char **argv)
writerecoveryconf = true;
break;
case 'S':
+
+ /*
+ * When specifying replication slot name, use a permanent
+ * slot.
+ */
replication_slot = pg_strdup(optarg);
+ temp_replication_slot = false;
+ break;
+ case 2:
+ no_slot = true;
break;
case 'T':
tablespace_list_append(optarg);
@@ -2268,7 +2303,7 @@ main(int argc, char **argv)
exit(1);
}
- if (replication_slot && !streamwal)
+ if ((replication_slot || no_slot) && !streamwal)
{
fprintf(stderr,
_("%s: replication slots can only be used with WAL streaming\n"),
@@ -2278,6 +2313,20 @@ main(int argc, char **argv)
exit(1);
}
+ if (no_slot)
+ {
+ if (replication_slot)
+ {
+ fprintf(stderr,
+ _("%s: no-slot cannot be used with slot name\n"),
+ progname);
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ progname);
+ exit(1);
+ }
+ temp_replication_slot = false;
+ }
+
if (strcmp(xlog_dir, "") != 0)
{
if (format != 'p')
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index 99445e6..e8389f6 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -41,6 +41,7 @@ static bool do_create_slot = false;
static bool slot_exists_ok = false;
static bool do_drop_slot = false;
static bool synchronous = false;
+static char *replication_slot = NULL;
static void usage(void);
@@ -340,6 +341,8 @@ StreamLog(void)
stream.mark_done = false;
stream.walmethod = CreateWalDirectoryMethod(basedir, stream.do_sync);
stream.partial_suffix = ".partial";
+ stream.replication_slot = replication_slot;
+ stream.temp_slot = false;
ReceiveXlogStream(conn, &stream);
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index cb5f989..be4d78a 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -44,6 +44,7 @@ static bool do_create_slot = false;
static bool slot_exists_ok = false;
static bool do_start_slot = false;
static bool do_drop_slot = false;
+static char *replication_slot = NULL;
/* filled pairwise with option, value. value may be NULL */
static char **options;
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 4382e5d..7d04882 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -455,10 +455,10 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
* synchronous_standby_names, but we've protected them against it so
* far, so let's continue to do so unless specifically requested.
*/
- if (replication_slot != NULL)
+ if (stream->replication_slot != NULL)
{
reportFlushPosition = true;
- sprintf(slotcmd, "SLOT \"%s\" ", replication_slot);
+ sprintf(slotcmd, "SLOT \"%s\" ", stream->replication_slot);
}
else
{
@@ -509,6 +509,24 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
}
/*
+ * Create temporary replication slot if one is needed
+ */
+ if (stream->temp_slot)
+ {
+ snprintf(query, sizeof(query),
+ "CREATE_REPLICATION_SLOT \"%s\" TEMPORARY PHYSICAL RESERVE_WAL",
+ stream->replication_slot);
+ res = PQexec(conn, query);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ fprintf(stderr, _("%s: could not create temporary replication slot \"%s\": %s\n"),
+ progname, stream->replication_slot, PQerrorMessage(conn));
+ PQclear(res);
+ return false;
+ }
+ }
+
+ /*
* initialize flush position to starting point, it's the caller's
* responsibility that that's sane.
*/
diff --git a/src/bin/pg_basebackup/receivelog.h b/src/bin/pg_basebackup/receivelog.h
index b5913ea..01776d3 100644
--- a/src/bin/pg_basebackup/receivelog.h
+++ b/src/bin/pg_basebackup/receivelog.h
@@ -37,13 +37,15 @@ typedef struct StreamCtl
* often */
bool synchronous; /* Flush immediately WAL data on write */
bool mark_done; /* Mark segment as done in generated archive */
- bool do_sync; /* Flush to disk to ensure consistent state
- * of data */
+ bool do_sync; /* Flush to disk to ensure consistent state of
+ * data */
stream_stop_callback stream_stop; /* Stop streaming when returns true */
WalWriteMethod *walmethod; /* How to write the WAL */
char *partial_suffix; /* Suffix appended to partially received files */
+ char *replication_slot; /* Replication slot to use, or NULL */
+ bool temp_slot; /* Create temporary replication slot */
} StreamCtl;
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 595eaff..7f2e078 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -38,7 +38,6 @@ char *connection_string = NULL;
char *dbhost = NULL;
char *dbuser = NULL;
char *dbport = NULL;
-char *replication_slot = NULL;
char *dbname = NULL;
int dbgetpassword = 0; /* 0=auto, -1=never, 1=always */
static bool have_password = false;
diff --git a/src/bin/pg_basebackup/streamutil.h b/src/bin/pg_basebackup/streamutil.h
index d2d5a6d..33d6afb 100644
--- a/src/bin/pg_basebackup/streamutil.h
+++ b/src/bin/pg_basebackup/streamutil.h
@@ -23,7 +23,6 @@ extern char *dbuser;
extern char *dbport;
extern char *dbname;
extern int dbgetpassword;
-extern char *replication_slot;
/* Connection kept global so we can disconnect easily */
extern PGconn *conn;
but -X stream is, then we use a temporary slot always.
This seems even more useful with -X fetch to me, as with fetch we are sure
we
are not immediately receiving the WAL. So, I'd propose to use it whenever -X
is specified, regardless of which method is specified.
(I still think we should change the default here, but that's a different
topic)
+1
Does that seem reasonable? Or would people prefer it to default to off?
Yes. Users are specifically requesting wal using -X, so making sure that
actually happens by default would work.
On Thu, Dec 15, 2016 at 7:28 PM, Magnus Hagander <magnus@hagander.net> wrote:
So here's a patch that does this, for discussion. It implements the
following behavior for -X:* When used with <10.0 servers, behave just like before.
* When -S <name> is specified, behave just like before (use an existing
replication slot, fail if it does not exist)
* When used on 10.0 with no -S, create and use a temporary replication slot
while running, with name pg_basebackup_<pid>.
* When used with 10.0 with no -S but --no-slot specified, run without a slot
like before.
There are users using -X stream without a slot now because they don't want to
cause WAL retention in pg_xlog and are ready for retries in taking the base
backup... I am wondering if it is a good idea to change the default behavior
and not introduce a new option like --temporary-slot, or
--slot-mode=temp|persistent|none with none being the default instead. There
are users caring about pg_xlog not filling up if pg_basebackup hangs on write
for example. And it may be a good idea to be able to use --slot-mode=temp
with -S/--slot actually to allow users to monitor it. If no slot name is given
with --slot generating one looks fine to me.
Regarding the patch, it would be good to add a set of TAP tests to cover the
new features. I am not seeing anything badly wrong with it at first sight by
the way.
--
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 Dec 16, 2016 07:27, "Michael Paquier" <michael.paquier@gmail.com> wrote:
On Thu, Dec 15, 2016 at 7:28 PM, Magnus Hagander <magnus@hagander.net>
wrote:
So here's a patch that does this, for discussion. It implements the
following behavior for -X:* When used with <10.0 servers, behave just like before.
* When -S <name> is specified, behave just like before (use an existing
replication slot, fail if it does not exist)
* When used on 10.0 with no -S, create and use a temporary replication
slot
while running, with name pg_basebackup_<pid>.
* When used with 10.0 with no -S but --no-slot specified, run without a
slot
like before.
There are users using -X stream without a slot now because they don't want
to
cause WAL retention in pg_xlog and are ready for retries in taking the base
backup... I am wondering if it is a good idea to change the default behavior
and not introduce a new option like --temporary-slot, or
--slot-mode=temp|persistent|none with none being the default instead. There
are users caring about pg_xlog not filling up if pg_basebackup hangs on
write
for example. And it may be a good idea to be able to use --slot-mode=temp
with -S/--slot actually to allow users to monitor it. If no slot name is
given
with --slot generating one looks fine to me.
I really think the default should be "what most people want", and not
"whatever is compatible with a mode that was necessary because we lacked
infrastructure".
Yes there are some people who are fine with their backups failing. I'm
willing to say there are *many* more who benefit from an automatic slot.
Regarding the patch, it would be good to add a set of TAP tests to cover the
new features. I am not seeing anything badly wrong with it at first sight by
the way
I don't really know how to write a good test for it. I mean, we could run
it with the parameter, but how to we make it actually verify the slot? Make
a really big database to make it guaranteed to be slow enough that we can
notice it in a different session?
/Magnus
At 2016-12-16 07:32:24 +0100, magnus@hagander.net wrote:
I really think the default should be "what most people want", and not
"whatever is compatible with a mode that was necessary because we
lacked infrastructure".
Very much agreed.
-- Abhijit
--
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 3:32 PM, Magnus Hagander <magnus@hagander.net> wrote:
I don't really know how to write a good test for it. I mean, we could run it
with the parameter, but how to we make it actually verify the slot? Make a
really big database to make it guaranteed to be slow enough that we can
notice it in a different session?
No, not that. I was meaning tests to trigger the error code paths with
the compatibility switches you are adding.
--
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 16/12/16 07:32, Magnus Hagander wrote:
On Dec 16, 2016 07:27, "Michael Paquier" <michael.paquier@gmail.com
<mailto:michael.paquier@gmail.com>> wrote:On Thu, Dec 15, 2016 at 7:28 PM, Magnus Hagander
<magnus@hagander.net <mailto:magnus@hagander.net>> wrote:So here's a patch that does this, for discussion. It implements the
following behavior for -X:* When used with <10.0 servers, behave just like before.
* When -S <name> is specified, behave just like before (use anexisting
replication slot, fail if it does not exist)
* When used on 10.0 with no -S, create and use a temporaryreplication slot
while running, with name pg_basebackup_<pid>.
* When used with 10.0 with no -S but --no-slot specified, runwithout a slot
like before.
There are users using -X stream without a slot now because they
don't want to
cause WAL retention in pg_xlog and are ready for retries in taking
the base
backup... I am wondering if it is a good idea to change the default
behavior
and not introduce a new option like --temporary-slot, or
--slot-mode=temp|persistent|none with none being the default
instead. There
are users caring about pg_xlog not filling up if pg_basebackup hangs
on write
for example. And it may be a good idea to be able to use
--slot-mode=temp
with -S/--slot actually to allow users to monitor it. If no slot
name is given
with --slot generating one looks fine to me.I really think the default should be "what most people want", and not
"whatever is compatible with a mode that was necessary because we lacked
infrastructure".
+1 (btw glad you made the patch)
Yes there are some people who are fine with their backups failing. I'm
willing to say there are *many* more who benefit from an automatic slot.
I think the issue with slots taking over disk space should be fixed by
making it possible to cut off slots when necessary (ie some GUC that
would say how much behind slot can be at maximum, with something like 0
being unlimited like it's now) instead of trying to work around that in
every client.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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 12/15/16 4:04 AM, Magnus Hagander wrote:
This obviously requires the server to be configured with enough slots (I
still think we should change the default here, but that's a different
topic), but I think that's acceptable.
We could implicitly reserve one replication slot per walsender. And
then max_replication_slots (possibly renamed) would just be additional
slots beyond that.
--
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 Fri, Dec 16, 2016 at 7:40 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:
On Fri, Dec 16, 2016 at 3:32 PM, Magnus Hagander <magnus@hagander.net>
wrote:I don't really know how to write a good test for it. I mean, we could
run it
with the parameter, but how to we make it actually verify the slot? Make
a
really big database to make it guaranteed to be slow enough that we can
notice it in a different session?No, not that. I was meaning tests to trigger the error code paths with
the compatibility switches you are adding.
There is only one switch - --no-slot. I guess you mean rune one backup with
that one? Sure, that's easy enough to do. I'm not sure how much it really
tests, but let's do it :)
You mean something like this, right?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachments:
pg_basebackup_temp_slot2.patchtext/x-patch; charset=US-ASCII; name=pg_basebackup_temp_slot2.patchDownload
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 1f15a17..f912ed0 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -246,6 +246,20 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>--no-slot</option></term>
+ <listitem>
+ <para>
+ This option prevents the creation of a temporary replication slot
+ during the backup even if it's supported by the server.
+ </para>
+ <para>
+ Temporary replication slots are created by default if no slot name
+ is given with the <literal>-S</literal> when using log streaming.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-T <replaceable class="parameter">olddir</replaceable>=<replaceable class="parameter">newdir</replaceable></option></term>
<term><option>--tablespace-mapping=<replaceable class="parameter">olddir</replaceable>=<replaceable class="parameter">newdir</replaceable></option></term>
<listitem>
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index f7ba9a9..fc171c1 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -61,6 +61,11 @@ typedef struct TablespaceList
*/
#define MINIMUM_VERSION_FOR_PG_WAL 100000
+/*
+ * Temporary replication slots are supported from version 10.
+ */
+#define MINIMUM_VERSION_FOR_TEMP_SLOTS 100000
+
/* Global options */
static char *basedir = NULL;
static TablespaceList tablespace_dirs = {NULL, NULL};
@@ -79,6 +84,8 @@ static bool do_sync = true;
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 char *replication_slot = NULL;
+static bool temp_replication_slot = true;
static bool success = false;
static bool made_new_pgdata = false;
@@ -323,6 +330,7 @@ usage(void)
printf(_(" -R, --write-recovery-conf\n"
" write recovery.conf after backup\n"));
printf(_(" -S, --slot=SLOTNAME replication slot to use\n"));
+ printf(_(" --no-slot prevent creation of temporary replication slot\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"));
@@ -452,6 +460,7 @@ typedef struct
char xlog[MAXPGPATH]; /* directory or tarfile depending on mode */
char *sysidentifier;
int timeline;
+ bool temp_slot;
} logstreamer_param;
static int
@@ -471,6 +480,16 @@ LogStreamerMain(logstreamer_param *param)
stream.do_sync = do_sync;
stream.mark_done = true;
stream.partial_suffix = NULL;
+ stream.replication_slot = replication_slot;
+ stream.temp_slot = param->temp_slot;
+ if (stream.temp_slot && !stream.replication_slot)
+ {
+ /* Generate a name for the temporary slot */
+ char name[32];
+
+ snprintf(name, sizeof(name), "pg_basebackup_%d", getpid());
+ stream.replication_slot = pstrdup(name);
+ }
if (format == 'p')
stream.walmethod = CreateWalDirectoryMethod(param->xlog, do_sync);
@@ -557,6 +576,11 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
PQserverVersion(conn) < MINIMUM_VERSION_FOR_PG_WAL ?
"pg_xlog" : "pg_wal");
+ /* Temporary replication slots are only supported in 10 and newer */
+ if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_TEMP_SLOTS)
+ param->temp_slot = false;
+ else
+ param->temp_slot = temp_replication_slot;
if (format == 'p')
{
@@ -2052,11 +2076,13 @@ main(int argc, char **argv)
{"verbose", no_argument, NULL, 'v'},
{"progress", no_argument, NULL, 'P'},
{"xlogdir", required_argument, NULL, 1},
+ {"no-slot", no_argument, NULL, 2},
{NULL, 0, NULL, 0}
};
int c;
int option_index;
+ bool no_slot = false;
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
@@ -2106,7 +2132,16 @@ main(int argc, char **argv)
writerecoveryconf = true;
break;
case 'S':
+
+ /*
+ * When specifying replication slot name, use a permanent
+ * slot.
+ */
replication_slot = pg_strdup(optarg);
+ temp_replication_slot = false;
+ break;
+ case 2:
+ no_slot = true;
break;
case 'T':
tablespace_list_append(optarg);
@@ -2268,7 +2303,7 @@ main(int argc, char **argv)
exit(1);
}
- if (replication_slot && !streamwal)
+ if ((replication_slot || no_slot) && !streamwal)
{
fprintf(stderr,
_("%s: replication slots can only be used with WAL streaming\n"),
@@ -2278,6 +2313,20 @@ main(int argc, char **argv)
exit(1);
}
+ if (no_slot)
+ {
+ if (replication_slot)
+ {
+ fprintf(stderr,
+ _("%s: --no-slot cannot be used with slot name\n"),
+ progname);
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ progname);
+ exit(1);
+ }
+ temp_replication_slot = false;
+ }
+
if (strcmp(xlog_dir, "") != 0)
{
if (format != 'p')
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index 99445e6..e8389f6 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -41,6 +41,7 @@ static bool do_create_slot = false;
static bool slot_exists_ok = false;
static bool do_drop_slot = false;
static bool synchronous = false;
+static char *replication_slot = NULL;
static void usage(void);
@@ -340,6 +341,8 @@ StreamLog(void)
stream.mark_done = false;
stream.walmethod = CreateWalDirectoryMethod(basedir, stream.do_sync);
stream.partial_suffix = ".partial";
+ stream.replication_slot = replication_slot;
+ stream.temp_slot = false;
ReceiveXlogStream(conn, &stream);
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index cb5f989..be4d78a 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -44,6 +44,7 @@ static bool do_create_slot = false;
static bool slot_exists_ok = false;
static bool do_start_slot = false;
static bool do_drop_slot = false;
+static char *replication_slot = NULL;
/* filled pairwise with option, value. value may be NULL */
static char **options;
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 4382e5d..7d04882 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -455,10 +455,10 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
* synchronous_standby_names, but we've protected them against it so
* far, so let's continue to do so unless specifically requested.
*/
- if (replication_slot != NULL)
+ if (stream->replication_slot != NULL)
{
reportFlushPosition = true;
- sprintf(slotcmd, "SLOT \"%s\" ", replication_slot);
+ sprintf(slotcmd, "SLOT \"%s\" ", stream->replication_slot);
}
else
{
@@ -509,6 +509,24 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
}
/*
+ * Create temporary replication slot if one is needed
+ */
+ if (stream->temp_slot)
+ {
+ snprintf(query, sizeof(query),
+ "CREATE_REPLICATION_SLOT \"%s\" TEMPORARY PHYSICAL RESERVE_WAL",
+ stream->replication_slot);
+ res = PQexec(conn, query);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ fprintf(stderr, _("%s: could not create temporary replication slot \"%s\": %s\n"),
+ progname, stream->replication_slot, PQerrorMessage(conn));
+ PQclear(res);
+ return false;
+ }
+ }
+
+ /*
* initialize flush position to starting point, it's the caller's
* responsibility that that's sane.
*/
diff --git a/src/bin/pg_basebackup/receivelog.h b/src/bin/pg_basebackup/receivelog.h
index b5913ea..01776d3 100644
--- a/src/bin/pg_basebackup/receivelog.h
+++ b/src/bin/pg_basebackup/receivelog.h
@@ -37,13 +37,15 @@ typedef struct StreamCtl
* often */
bool synchronous; /* Flush immediately WAL data on write */
bool mark_done; /* Mark segment as done in generated archive */
- bool do_sync; /* Flush to disk to ensure consistent state
- * of data */
+ bool do_sync; /* Flush to disk to ensure consistent state of
+ * data */
stream_stop_callback stream_stop; /* Stop streaming when returns true */
WalWriteMethod *walmethod; /* How to write the WAL */
char *partial_suffix; /* Suffix appended to partially received files */
+ char *replication_slot; /* Replication slot to use, or NULL */
+ bool temp_slot; /* Create temporary replication slot */
} StreamCtl;
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 595eaff..7f2e078 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -38,7 +38,6 @@ char *connection_string = NULL;
char *dbhost = NULL;
char *dbuser = NULL;
char *dbport = NULL;
-char *replication_slot = NULL;
char *dbname = NULL;
int dbgetpassword = 0; /* 0=auto, -1=never, 1=always */
static bool have_password = false;
diff --git a/src/bin/pg_basebackup/streamutil.h b/src/bin/pg_basebackup/streamutil.h
index d2d5a6d..33d6afb 100644
--- a/src/bin/pg_basebackup/streamutil.h
+++ b/src/bin/pg_basebackup/streamutil.h
@@ -23,7 +23,6 @@ extern char *dbuser;
extern char *dbport;
extern char *dbname;
extern int dbgetpassword;
-extern char *replication_slot;
/* Connection kept global so we can disconnect easily */
extern PGconn *conn;
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 7811093..8e786c1 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 => 70;
program_help_ok('pg_basebackup');
program_version_ok('pg_basebackup');
@@ -239,6 +239,9 @@ $node->command_ok(
[ 'pg_basebackup', '-D', "$tempdir/backupxst", '-X', 'stream', '-Ft' ],
'pg_basebackup -X stream runs in tar mode');
ok(-f "$tempdir/backupxst/pg_wal.tar", "tar file was created");
+$node->command_ok(
+ [ 'pg_basebackup', '-D', "$tempdir/backupnoslot", '-X', 'stream', '--no-slot' ],
+ 'pg_basebackup -X stream runs with --no-slot');
$node->command_fails(
[ 'pg_basebackup', '-D', "$tempdir/fail", '-S', 'slot1' ],
On Fri, Dec 16, 2016 at 12:41 PM, Petr Jelinek <petr.jelinek@2ndquadrant.com
wrote:
On 16/12/16 07:32, Magnus Hagander wrote:
On Dec 16, 2016 07:27, "Michael Paquier" <michael.paquier@gmail.com
<mailto:michael.paquier@gmail.com>> wrote:On Thu, Dec 15, 2016 at 7:28 PM, Magnus Hagander
<magnus@hagander.net <mailto:magnus@hagander.net>> wrote:So here's a patch that does this, for discussion. It implements the
following behavior for -X:* When used with <10.0 servers, behave just like before.
* When -S <name> is specified, behave just like before (use anexisting
replication slot, fail if it does not exist)
* When used on 10.0 with no -S, create and use a temporaryreplication slot
while running, with name pg_basebackup_<pid>.
* When used with 10.0 with no -S but --no-slot specified, runwithout a slot
like before.
There are users using -X stream without a slot now because they
don't want to
cause WAL retention in pg_xlog and are ready for retries in taking
the base
backup... I am wondering if it is a good idea to change the default
behavior
and not introduce a new option like --temporary-slot, or
--slot-mode=temp|persistent|none with none being the default
instead. There
are users caring about pg_xlog not filling up if pg_basebackup hangs
on write
for example. And it may be a good idea to be able to use
--slot-mode=temp
with -S/--slot actually to allow users to monitor it. If no slot
name is given
with --slot generating one looks fine to me.I really think the default should be "what most people want", and not
"whatever is compatible with a mode that was necessary because we lacked
infrastructure".+1 (btw glad you made the patch)
I'm glad you made the temp slots, so I could abandon that branch :)
Yes there are some people who are fine with their backups failing. I'm
willing to say there are *many* more who benefit from an automatic slot.I think the issue with slots taking over disk space should be fixed by
making it possible to cut off slots when necessary (ie some GUC that
would say how much behind slot can be at maximum, with something like 0
being unlimited like it's now) instead of trying to work around that in
every client.
Agreed, that seems like a better solution.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Sat, Dec 17, 2016 at 3:54 PM, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:
On 12/15/16 4:04 AM, Magnus Hagander wrote:
This obviously requires the server to be configured with enough slots (I
still think we should change the default here, but that's a different
topic), but I think that's acceptable.We could implicitly reserve one replication slot per walsender. And
then max_replication_slots (possibly renamed) would just be additional
slots beyond that.
That makes a lot of sense now that we have temporary replication slots, I
agree. And then max_replication_slots could be something like
persistent_replication_slots?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On 12/17/16 9:55 AM, Magnus Hagander wrote:
That makes a lot of sense now that we have temporary replication slots,
I agree. And then max_replication_slots could be something like
persistent_replication_slots?
I was thinking was more like that each walsender gets one fixed slot,
which can be temporary or persistent.
Or maybe default max_replication_slots to something like -1, which means
equal to max_wal_senders.
--
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
This patch looks reasonable to me.
Attached is a top-up patch with a few small fixups.
I suggest to wait for the resolution of the "Replication/backup
defaults" thread. I would not want to be in a situation where users who
have not been trained to use replication slots now have yet another
restart-only parameter to set before they can take a backup.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
pg_basebackup_temp_slot2-fixups.patchtext/x-patch; name=pg_basebackup_temp_slot2-fixups.patchDownload
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 5f0efe2239..5c2db2581c 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -240,6 +240,11 @@ <title>Options</title>
the server does not remove any necessary WAL data in the time between
the end of the base backup and the start of streaming replication.
</para>
+ <para>
+ If this option is not specified and the server supports temporary
+ replication slots (version 10 and later), then a temporary replication
+ slot is automatically used for WAL streaming.
+ </para>
</listitem>
</varlistentry>
@@ -252,7 +257,13 @@ <title>Options</title>
</para>
<para>
Temporary replication slots are created by default if no slot name
- is given with the <literal>-S</literal> when using log streaming.
+ is given with the option <option>-S</option> when using log streaming.
+ </para>
+ <para>
+ The main purpose of this option is to allow taking a base backup when
+ the server is out of free replication slots. Using replication slots
+ is almost always preferred, because it prevents needed WAL from being
+ removed by the server during the backup.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 111d113bbf..f443aee631 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -482,13 +482,7 @@ LogStreamerMain(logstreamer_param *param)
stream.replication_slot = replication_slot;
stream.temp_slot = param->temp_slot;
if (stream.temp_slot && !stream.replication_slot)
- {
- /* Generate a name for the temporary slot */
- char name[32];
-
- snprintf(name, sizeof(name), "pg_basebackup_%d", getpid());
- stream.replication_slot = pstrdup(name);
- }
+ stream.replication_slot = psprintf("pg_basebackup_%d", (int) getpid());
if (format == 'p')
stream.walmethod = CreateWalDirectoryMethod(param->xlog, do_sync);
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 72f3339c91..55612832a6 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -519,7 +519,7 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
res = PQexec(conn, query);
if (PQresultStatus(res) != PGRES_TUPLES_OK)
{
- fprintf(stderr, _("%s: could not create temporary replication slot \"%s\": %s\n"),
+ fprintf(stderr, _("%s: could not create temporary replication slot \"%s\": %s"),
progname, stream->replication_slot, PQerrorMessage(conn));
PQclear(res);
return false;
On Wed, Jan 4, 2017 at 3:32 PM, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:
This patch looks reasonable to me.
Attached is a top-up patch with a few small fixups.
I suggest to wait for the resolution of the "Replication/backup
defaults" thread. I would not want to be in a situation where users who
have not been trained to use replication slots now have yet another
restart-only parameter to set before they can take a backup.
Thanks for the review and the top-up patch. I've applied it and pushed.
I just realized I missed crediting you with the review and the top-up in
the commit message. My apologies for this.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Mon, Jan 16, 2017 at 10:00 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Wed, Jan 4, 2017 at 3:32 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:This patch looks reasonable to me.
Attached is a top-up patch with a few small fixups.
I suggest to wait for the resolution of the "Replication/backup
defaults" thread. I would not want to be in a situation where users who
have not been trained to use replication slots now have yet another
restart-only parameter to set before they can take a backup.Thanks for the review and the top-up patch. I've applied it and pushed.
- if (replication_slot && includewal != STREAM_WAL)
+ if ((replication_slot || no_slot) && includewal != STREAM_WAL)Why do we need to check "no_slot" here? Since no_slot=true means that
no temporary replication slot is specified, it's fine even with both -X none
and fetch.
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 Mon, Jan 16, 2017 at 4:33 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Jan 16, 2017 at 10:00 PM, Magnus Hagander <magnus@hagander.net>
wrote:On Wed, Jan 4, 2017 at 3:32 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:This patch looks reasonable to me.
Attached is a top-up patch with a few small fixups.
I suggest to wait for the resolution of the "Replication/backup
defaults" thread. I would not want to be in a situation where users who
have not been trained to use replication slots now have yet another
restart-only parameter to set before they can take a backup.Thanks for the review and the top-up patch. I've applied it and pushed.
- if (replication_slot && includewal != STREAM_WAL) + if ((replication_slot || no_slot) && includewal != STREAM_WAL)Why do we need to check "no_slot" here? Since no_slot=true means that
no temporary replication slot is specified, it's fine even with both -X
none
and fetch.
Um yeah, that looks like a bad merge actually. Will fix.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/