Re: [HACKERS] pg_basebackups and slots

[Magnus Hagander]

On Mon, Jan 16, 2017 at 4:33 PM, Fujii Masao  wrote:

> On Mon, Jan 16, 2017 at 10:00 PM, Magnus Hagander 
> wrote:
> >
> >
> > On Wed, Jan 4, 2017 at 3:32 PM, Peter Eisentraut
> >  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/

Re: [HACKERS] pg_basebackups and slots

[Fujii Masao]

On Mon, Jan 16, 2017 at 10:00 PM, Magnus Hagander  wrote:
>
>
> On Wed, Jan 4, 2017 at 3:32 PM, Peter Eisentraut
>  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

Re: [HACKERS] pg_basebackups and slots

[Magnus Hagander]

On Wed, Jan 4, 2017 at 3:32 PM, Peter Eisentraut <
peter.eisentr...@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/

Re: [HACKERS] pg_basebackups and slots

[Peter Eisentraut]

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
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 @@ Options
 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.

+   
+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.
+   
   
  
 
@@ -252,7 +257,13 @@ Options


 Temporary replication slots are created by default if no slot name
-is given with the -S when using log streaming.
+is given with the option -S when using log streaming.
+   
+   
+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.

   
  
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;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_basebackups and slots

[Peter Eisentraut]

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

Re: [HACKERS] pg_basebackups and slots

[Magnus Hagander]

On Sat, Dec 17, 2016 at 3:54 PM, Peter Eisentraut <
peter.eisentr...@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/

Re: [HACKERS] pg_basebackups and slots

[Magnus Hagander]

On Fri, Dec 16, 2016 at 7:40 AM, Michael Paquier 
wrote:

> On Fri, Dec 16, 2016 at 3:32 PM, Magnus Hagander 
> 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/
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
  
 
  
+  --no-slot
+  
+   
+This option prevents the creation of a temporary replication slot
+during the backup even if it's supported by the server.
+   
+   
+Temporary replication slots are created by default if no slot name
+is given with the -S when using log streaming.
+   
+  
+ 
+
+ 
   -T olddir=newdir
   --tablespace-mapping=olddir=newdir
   
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	10
 
+/*
+ * Temporary replication slots are supported from version 10.
+ */
+#define MINIMUM_VERSION_FOR_TEMP_SLOTS 10
+
 /* 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=SLOTNAMEreplication 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);

Re: [HACKERS] pg_basebackups and slots

[Peter Eisentraut]

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

Re: [HACKERS] pg_basebackups and slots

[Magnus Hagander]

On Fri, Dec 16, 2016 at 12:41 PM, Petr Jelinek  wrote:

> On 16/12/16 07:32, Magnus Hagander wrote:
> >
> > On Dec 16, 2016 07:27, "Michael Paquier"  > > wrote:
> >
> >
> >
> > On Thu, Dec 15, 2016 at 7:28 PM, Magnus Hagander
> > mailto:mag...@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  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_.
> > > * 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".
>
> +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/

Re: [HACKERS] pg_basebackups and slots

[Petr Jelinek]

On 16/12/16 07:32, Magnus Hagander wrote:
> 
> On Dec 16, 2016 07:27, "Michael Paquier"  > wrote:
> 
> 
> 
> On Thu, Dec 15, 2016 at 7:28 PM, Magnus Hagander
> mailto:mag...@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  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_.
> > * 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". 

+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

Re: [HACKERS] pg_basebackups and slots

[Michael Paquier]

On Fri, Dec 16, 2016 at 3:32 PM, Magnus Hagander  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

Re: [HACKERS] pg_basebackups and slots

[Abhijit Menon-Sen]

At 2016-12-16 07:32:24 +0100, mag...@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

Re: [HACKERS] pg_basebackups and slots

[Magnus Hagander]

On Dec 16, 2016 07:27, "Michael Paquier"  wrote:



On Thu, Dec 15, 2016 at 7:28 PM, Magnus Hagander 
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  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_.
> * 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

Re: [HACKERS] pg_basebackups and slots

[Michael Paquier]

On Thu, Dec 15, 2016 at 7:28 PM, Magnus Hagander  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  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_.
> * 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

Re: [HACKERS] pg_basebackups and slots

[Feike Steenbergen]

> 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.

Re: [HACKERS] pg_basebackups and slots

[Magnus Hagander]

On Thu, Dec 15, 2016 at 10:26 AM, Magnus Hagander 
wrote:

>
>
> On Thu, Dec 15, 2016 at 10:04 AM, Magnus Hagander 
> 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  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_.
* 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/
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
  
 
  
+  --no-slot
+  
+   
+This option prevents the creation of a temporary replication slot
+during the backup even if it's supported by the server.
+   
+   
+Temporary replication slots are created by default if no slot name
+is given with the -S when using log streaming.
+   
+  
+ 
+
+ 
   -T olddir=newdir
   --tablespace-mapping=olddir=newdir
   
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	10
 
+/*
+ * Temporary replication slots are supported from version 10.
+ */
+#define MINIMUM_VERSION_FOR_TEMP_SLOTS 10
+
 /* 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=SLOTNAMEreplication 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 @@ StartLo

Re: [HACKERS] pg_basebackups and slots

[Magnus Hagander]

On Thu, Dec 15, 2016 at 10:04 AM, Magnus Hagander 
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/

[HACKERS] pg_basebackups and slots

[Magnus Hagander]

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/