Re: [HACKERS] pg_basebackup and replication slots

[Joshua D. Drake]

On 10/26/2015 08:14 PM, Michael Paquier wrote:

On Tue, Oct 27, 2015 at 7:18 AM, José Luis Tallón wrote:

Given the rest of the thread any possibility whatsoever that it'd be
backpatched into 9.5 before release?
 Guess it'd be a very welcome addition


This will be available in 9.6. 9.5 is aimed at being stabilized now,
so no new features are added at this point.



I know this isn't the answer people are looking for but it is also 
possible to back patch this. Of course we won't put it in the main tree 
but this might be something worth leaving outside (until 9.6).


JD

--
Command Prompt, Inc. - http://www.commandprompt.com/  503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Announcing "I'm offended" is basically telling the world you can't
control your own emotions, so everyone else should do it for you.


--
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_basebackup and replication slots

[Michael Paquier]

On Tue, Oct 27, 2015 at 7:18 AM, José Luis Tallón wrote:
> Given the rest of the thread any possibility whatsoever that it'd be
> backpatched into 9.5 before release?
> Guess it'd be a very welcome addition

This will be available in 9.6. 9.5 is aimed at being stabilized now,
so no new features are added at this point.
-- 
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_basebackup and replication slots

[José Luis Tallón]

On 10/26/2015 12:58 PM, Joshua D. Drake wrote:

Hello,

The fact that pg_basebackup doesn't use replicaiton slots, is that a 
technical limitation or just a, "we need a patch"?




Given the rest of the thread any possibility whatsoever that it'd be 
backpatched into 9.5 before release?

Guess it'd be a very welcome addition


Thanks,

/ J.L.



--
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_basebackup and replication slots

[Shulgin, Oleksandr]

On Mon, Oct 26, 2015 at 12:58 PM, Joshua D. Drake 
wrote:

> Hello,
>
> The fact that pg_basebackup doesn't use replicaiton slots, is that a
> technical limitation or just a, "we need a patch"?
>

I believe it does, but only in master so far, not even in 9.5:
http://www.postgresql.org/docs/devel/static/app-pgbasebackup.html

--
Alex

Re: [HACKERS] pg_basebackup and replication slots

[Euler Taveira]

On 26-10-2015 08:58, Joshua D. Drake wrote:

Hello,

The fact that pg_basebackup doesn't use replicaiton slots, is that a
technical limitation or just a, "we need a patch"?


It is not in 9.5 but it is already there.

commit 0dc848b0314d63188919f1ce943730eac684dccd
Author: Peter Eisentraut 
Date:   Tue Jul 21 21:06:45 2015 -0400

pg_basebackup: Add --slot option

This option specifies a replication slot for WAL streaming (-X stream),
so that there can be continuous replication slot use between WAL
streaming during the base backup and the start of regular streaming
replication.

Reviewed-by: Michael Paquier 


--
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


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

[HACKERS] pg_basebackup and replication slots

[Joshua D. Drake]

Hello,

The fact that pg_basebackup doesn't use replicaiton slots, is that a 
technical limitation or just a, "we need a patch"?


JD
--
Command Prompt, Inc. - http://www.commandprompt.com/  503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Announcing "I'm offended" is basically telling the world you can't
control your own emotions, so everyone else should do it for you.


--
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_basebackup and replication slots

[Peter Eisentraut]

On 7/29/15 8:00 AM, Andres Freund wrote:
> As far as I understand this subthread the goal is to have a
> pg_basebackup that internally creates a slot, so it can guarantee that
> all the required WAL is present till streamed out by -X
> stream/fetch. The problem with just creating a slot is that it'd "leak"
> if pg_basebackup is killed, or the connection breaks.  The idea with the
> ephemeral slot is that it'd automatically kept alive until the base
> backup is complete, but would also automatically be dropped if the base
> backup fails.

I don't think anyone actually said that.  I think the goal was to add
the functionality that pg_basebackup can optionally create a
(persistent) replication slot, both for its own use and for later use by
streaming.  Just so you don't have to call pg_create...slot() or
pg_receivexlog separately to create the slot.  And it was pointed out
that this created slot would need to be removed if pg_basebackup failed
for some reason.

What you describe also seems useful, but is a different sub-issue.



-- 
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_basebackup and replication slots

[Andres Freund]

On 2015-07-29 13:45:22 +0100, Simon Riggs wrote:
> So this would be needed when creating a standalone backup that would not be
> persistently connected to the master, yet we want to bring it up as a
> live/writable server in a single command

I'm not understanding what you mean with 'single command'
here. Currently there's no way to do this safely without configuring a
high enough wal_keep_segments.

You can (since yesterday) manually create a slot, run pg_basebackup, and
drop the slot. But that's not safe if your script is interrupted
somehow. Since many base backups are run in a non-interactive fashion
asking for intervention to clean up in that case imo is not an
acceptable answer.

> and we want to make it easy to script in case our script is killed?

Or the connection dies/is killed, or the server is restarted, or ...


-- 
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_basebackup and replication slots

[Simon Riggs]

On 29 July 2015 at 13:00, Andres Freund  wrote:


> As far as I understand this subthread the goal is to have a
> pg_basebackup that internally creates a slot, so it can guarantee that
> all the required WAL is present till streamed out by -X
> stream/fetch. The problem with just creating a slot is that it'd "leak"
> if pg_basebackup is killed, or the connection breaks.  The idea with the
> ephemeral slot is that it'd automatically kept alive until the base
> backup is complete, but would also automatically be dropped if the base
> backup fails.
>
> Makes sense?
>

So this would be needed when creating a standalone backup that would not be
persistently connected to the master, yet we want to bring it up as a
live/writable server in a single command, and we want to make it easy to
script in case our script is killed?

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] pg_basebackup and replication slots

[Andres Freund]

On 2015-07-29 12:47:01 +0100, Simon Riggs wrote:
> On 29 July 2015 at 11:43, Andres Freund  wrote:
> 
> > On 2015-07-29 09:17:04 +0100, Simon Riggs wrote:
> > > On 29 July 2015 at 09:09, Andres Freund  wrote:
> > > > The point of using a temporary slot is to not have a
> > > > leftover slot afterwards, reserving resources. Especially important if
> > > > the basebackup actually failed...
> > > >
> > >
> > > Creating a slot and then deleting it if the session disconnects does not
> > > successfully provide the functionality desired above.
> >
> > Uh? If you create the slot, start streaming, and then start the
> > basebackup, it does. It does *not* guarantee that the base backup can be
> > converted into a replica, but it's sufficient to guarantee it can
> > brought out of recovery.
> >
> 
> Perhaps we are misunderstanding the word "it" here. "it can be brought out
> of recovery"?

The finished base backup.

> You appear to be saying that a backup that disconnects before completion is
> useful in some way. How so?

I'm not trying to say that at all.

As far as I understand this subthread the goal is to have a
pg_basebackup that internally creates a slot, so it can guarantee that
all the required WAL is present till streamed out by -X
stream/fetch. The problem with just creating a slot is that it'd "leak"
if pg_basebackup is killed, or the connection breaks.  The idea with the
ephemeral slot is that it'd automatically kept alive until the base
backup is complete, but would also automatically be dropped if the base
backup fails.

Makes sense?

We pretty much have all the required infrastructure for that in slot.c,
it's just not exposed to the replication protocol.

Greetings,

Andres Freund


-- 
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_basebackup and replication slots

[Simon Riggs]

On 29 July 2015 at 11:43, Andres Freund  wrote:

> On 2015-07-29 09:17:04 +0100, Simon Riggs wrote:
> > On 29 July 2015 at 09:09, Andres Freund  wrote:
> > > The point of using a temporary slot is to not have a
> > > leftover slot afterwards, reserving resources. Especially important if
> > > the basebackup actually failed...
> > >
> >
> > Creating a slot and then deleting it if the session disconnects does not
> > successfully provide the functionality desired above.
>
> Uh? If you create the slot, start streaming, and then start the
> basebackup, it does. It does *not* guarantee that the base backup can be
> converted into a replica, but it's sufficient to guarantee it can
> brought out of recovery.
>

Perhaps we are misunderstanding the word "it" here. "it can be brought out
of recovery"?

You appear to be saying that a backup that disconnects before completion is
useful in some way. How so?

If the slot is cleaned up on disconnect, as suggested, then you end up with
half a backup and WAL is cleaned up. The only possible use for slots is to
reserve resources (as you say); the resources will clearly not be reserved
if we drop the slot on disconnect. What use is that?

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] pg_basebackup and replication slots

[Andres Freund]

On 2015-07-29 09:17:04 +0100, Simon Riggs wrote:
> On 29 July 2015 at 09:09, Andres Freund  wrote:
> > The point of using a temporary slot is to not have a
> > leftover slot afterwards, reserving resources. Especially important if
> > the basebackup actually failed...
> >
> 
> Creating a slot and then deleting it if the session disconnects does not
> successfully provide the functionality desired above.

Uh? If you create the slot, start streaming, and then start the
basebackup, it does. It does *not* guarantee that the base backup can be
converted into a replica, but it's sufficient to guarantee it can
brought out of recovery.


-- 
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_basebackup and replication slots

[Simon Riggs]

On 29 July 2015 at 09:09, Andres Freund  wrote:

> On 2015-07-29 08:57:38 +0100, Simon Riggs wrote:
> > On 22 July 2015 at 05:43, Michael Paquier 
> wrote:
> >
> >
> > > Now, do we plan to do something about the creation of a slot. I
> > > imagine that it would be useful if we could have --create-slot to
> > > create a slot when beginning a base backup and if-not-exists to
> > > control the error flow. There is already CreateReplicationSlot for
> > > this purpose in streamutils.c so most of the work is already done.
> > > Also, when a base backup fails for a reason or another, we should try
> > > to drop the slot in disconnect_and_exit() if it has been created by
> > > pg_basebackup. if if-not-exists is true and the slot already existed
> > > when beginning, we had better not dropping it perhaps...
> >
> >
> > What is the purpose of creating a temporary slot?
>
> > If we create a slot when one is needed and then drop automatically on
> > session disconnect (as Heikki suggest), what benefit does using a slot
> give
> > us?
>
> The goal is to have a reliable pg_basebackup that doesn't fail due to
> missing WAL (which can easily happen even with -X) . That seems like a
> sane desire.


Agreed, that is sane. Slots are good.


> The point of using a temporary slot is to not have a
> leftover slot afterwards, reserving resources. Especially important if
> the basebackup actually failed...
>

Creating a slot and then deleting it if the session disconnects does not
successfully provide the functionality desired above.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] pg_basebackup and replication slots

[Andres Freund]

On 2015-07-29 08:57:38 +0100, Simon Riggs wrote:
> On 22 July 2015 at 05:43, Michael Paquier  wrote:
> 
> 
> > Now, do we plan to do something about the creation of a slot. I
> > imagine that it would be useful if we could have --create-slot to
> > create a slot when beginning a base backup and if-not-exists to
> > control the error flow. There is already CreateReplicationSlot for
> > this purpose in streamutils.c so most of the work is already done.
> > Also, when a base backup fails for a reason or another, we should try
> > to drop the slot in disconnect_and_exit() if it has been created by
> > pg_basebackup. if if-not-exists is true and the slot already existed
> > when beginning, we had better not dropping it perhaps...
> 
> 
> What is the purpose of creating a temporary slot?

> If we create a slot when one is needed and then drop automatically on
> session disconnect (as Heikki suggest), what benefit does using a slot give
> us?

The goal is to have a reliable pg_basebackup that doesn't fail due to
missing WAL (which can easily happen even with -X) . That seems like a
sane desire.  The point of using a temporary slot is to not have a
leftover slot afterwards, reserving resources. Especially important if
the basebackup actually failed...


-- 
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_basebackup and replication slots

[Simon Riggs]

On 22 July 2015 at 05:43, Michael Paquier  wrote:


> Now, do we plan to do something about the creation of a slot. I
> imagine that it would be useful if we could have --create-slot to
> create a slot when beginning a base backup and if-not-exists to
> control the error flow. There is already CreateReplicationSlot for
> this purpose in streamutils.c so most of the work is already done.
> Also, when a base backup fails for a reason or another, we should try
> to drop the slot in disconnect_and_exit() if it has been created by
> pg_basebackup. if if-not-exists is true and the slot already existed
> when beginning, we had better not dropping it perhaps...


What is the purpose of creating a temporary slot?

My understand was that slots are supposed to be persistent so we should
create them before use.

If we create a slot when one is needed and then drop automatically on
session disconnect (as Heikki suggest), what benefit does using a slot give
us?

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] pg_basebackup and replication slots

[Peter Eisentraut]

On 7/22/15 12:43 AM, Michael Paquier wrote:
> OK, thanks for the updated versions. Those ones look good to me.

Committed, thanks.

> Now, do we plan to do something about the creation of a slot. I
> imagine that it would be useful if we could have --create-slot to
> create a slot when beginning a base backup and if-not-exists to
> control the error flow. There is already CreateReplicationSlot for
> this purpose in streamutils.c so most of the work is already done.
> Also, when a base backup fails for a reason or another, we should try
> to drop the slot in disconnect_and_exit() if it has been created by
> pg_basebackup. if if-not-exists is true and the slot already existed
> when beginning, we had better not dropping it perhaps...

I think it would be worthwhile to work on that, but in my mind it's a
separate feature, and I don't have any use for it, so I'm not going to
rush into it.




-- 
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_basebackup and replication slots

[Michael Paquier]

On Wed, Jul 22, 2015 at 10:15 AM, Peter Eisentraut wrote:
> On 7/2/15 3:37 AM, Michael Paquier wrote:
>> 2)
>>  sub psql
>>  {
>> my ($dbname, $sql) = @_;
>> -   run [ 'psql', '-X', '-q', '-d', $dbname, '-f', '-' ], '<', \$sql or 
>> die;
>> +   my ($stdout, $stderr);
>> +   run [ 'psql', '-X', '-A', '-t', '-q', '-d', $dbname, '-f', '-'
>> ], '<', \$sql, '>', \$stdout, '2>', \$stderr or die;
>> +   chomp $stdout;
>> +   return $stdout;
>>  }
>> RewindTest.pm has a routine called check_query defined. I would be
>> great to extend a bit more psql than what I thought previously by
>> returning from it ($result, $stdout, $strerr) and make check_query
>> directly use it. This way we have a unique interface to run psql and
>> capture output. I don't mind writing this refactoring patch on top of
>> your set if that's necessary.
>
> Let's do that as a separate patch.  Adding multiple return values makes
> calling awkward, so I'd like to sort out the actual use cases before we
> do that.

OK.

>> 3)
>> +command_ok([ 'pg_basebackup', '-D', "$tempdir/backupxs_sl_R", '-X',
>> 'stream', '-S', 'slot1', '-R' ],
>> +   'pg_basebackup with replication slot and -R runs');
>> +$recovery_conf = slurp_file "$tempdir/backupR/recovery.conf";
>> +like(slurp_file("$tempdir/backupxs_sl_R/recovery.conf"),
>> slurp_file is slurping an incorrect file and $recovery_conf is used
>> nowhere after, so you could simply remove this line.
>
> Yeah, that was some leftover garbage.

OK, thanks for the updated versions. Those ones look good to me.

Now, do we plan to do something about the creation of a slot. I
imagine that it would be useful if we could have --create-slot to
create a slot when beginning a base backup and if-not-exists to
control the error flow. There is already CreateReplicationSlot for
this purpose in streamutils.c so most of the work is already done.
Also, when a base backup fails for a reason or another, we should try
to drop the slot in disconnect_and_exit() if it has been created by
pg_basebackup. if if-not-exists is true and the slot already existed
when beginning, we had better not dropping it perhaps...
-- 
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_basebackup and replication slots

[Peter Eisentraut]

On 7/2/15 3:37 AM, Michael Paquier wrote:
> Regarding patch 3, I have more comments:
> 1) I think that documentation should clearly mention that if -R and -S
> are used together, a primary_slot_name entry is added in the
> recovery.conf generated with the slot name defined.

Updated proposal attached.

> 2)
>  sub psql
>  {
> my ($dbname, $sql) = @_;
> -   run [ 'psql', '-X', '-q', '-d', $dbname, '-f', '-' ], '<', \$sql or 
> die;
> +   my ($stdout, $stderr);
> +   run [ 'psql', '-X', '-A', '-t', '-q', '-d', $dbname, '-f', '-'
> ], '<', \$sql, '>', \$stdout, '2>', \$stderr or die;
> +   chomp $stdout;
> +   return $stdout;
>  }
> RewindTest.pm has a routine called check_query defined. I would be
> great to extend a bit more psql than what I thought previously by
> returning from it ($result, $stdout, $strerr) and make check_query
> directly use it. This way we have a unique interface to run psql and
> capture output. I don't mind writing this refactoring patch on top of
> your set if that's necessary.

Let's do that as a separate patch.  Adding multiple return values makes
calling awkward, so I'd like to sort out the actual use cases before we
do that.

> 3)
> +command_ok([ 'pg_basebackup', '-D', "$tempdir/backupxs_sl_R", '-X',
> 'stream', '-S', 'slot1', '-R' ],
> +   'pg_basebackup with replication slot and -R runs');
> +$recovery_conf = slurp_file "$tempdir/backupR/recovery.conf";
> +like(slurp_file("$tempdir/backupxs_sl_R/recovery.conf"),
> slurp_file is slurping an incorrect file and $recovery_conf is used
> nowhere after, so you could simply remove this line.

Yeah, that was some leftover garbage.

>From 7ecb1794c2aaeea9753af07e2f54f6e483af255f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 21 Jul 2015 21:06:45 -0400
Subject: [PATCH 3/3] pg_basebackup: Add --slot option

This option specifies a replication slot for WAL streaming (-X stream),
so that there can be continuous replication slot use between WAL
streaming during the base backup and the start of regular streaming
replication.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 27 ---
 src/bin/pg_basebackup/pg_basebackup.c| 24 +++-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 22 +-
 src/test/perl/TestLib.pm |  5 -
 4 files changed, 72 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index cb8b8a3..5f8b9b7 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -215,15 +215,36 @@ Options
   
 

-Write a minimal recovery.conf in the output directory (or into
-the base archive file when using tar format) to ease setting
-up a standby server.
+Write a minimal recovery.conf in the output
+directory (or into the base archive file when using tar format) to
+ease setting up a standby server.
+The recovery.conf file will record the connection
+settings and, if specified, the replication slot
+that pg_basebackup is using, so that the
+streaming replication will use the same settings later on.

 
   
  
 
  
+  -S slotname
+  --slot=slotname
+  
+   
+This option can only be used together with -X
+stream.  It causes the WAL streaming to use the specified
+replication slot.  If the base backup is intended to be used as a
+streaming replication standby using replication slots, it should then
+use the same replication slot name
+in recovery.conf.  That way, it is ensured that
+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.
+   
+  
+ 
+
+ 
   -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 5363680..80de882 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -239,6 +239,7 @@ usage(void)
 	  " (in kB/s, or use suffix \"k\" or \"M\")\n"));
 	printf(_("  -R, --write-recovery-conf\n"
 			 " write recovery.conf after backup\n"));
+	printf(_("  -S, --slot=SLOTNAMEreplication 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"));
@@ -1536,6 +1537,13 @@ GenerateRecoveryConf(PGconn *conn)
 	appendPQExpBuffer(recoveryconfcontents, "primary_conninfo = '%s'\n", escaped);
 	free(escaped);
 
+	if (replication_slot)
+	{
+		escaped = escape_quotes(replication_slot);
+		appendPQExpBuffer(recoveryconfcontents, "primary_slot_name = '%s'\n"

Re: [HACKERS] pg_basebackup and replication slots

[Michael Paquier]

On Wed, Jul 1, 2015 at 11:35 PM, Peter Eisentraut  wrote:
> On 7/1/15 8:37 AM, Michael Paquier wrote:
>> On Wed, Jul 1, 2015 at 10:33 AM, Peter Eisentraut wrote:
>>> (If you're looking at the patch and wondering why there is no code to
>>> actually do anything with the replication slot, that's because the code
>>> that does the WAL streaming is already aware of replication slots
>>> because of the pg_receivexlog functionality that it also serves.  So all
>>> that needs to be done is set replication_slot.)
>>
>> This is cool to see this patch taking shape.
>>
>> +my ($stdout, $stderr);
>> +run [ 'psql', '-X', '-A', '-t', '-q', '-d', $dbname, '-f', '-' ],
>> '<', \$sql, '>', \$stdout, '2>', \$stderr or die;
>> +chomp $stdout;
>> +return $stdout;
>>
>> Could it be possible to chomp and return $stderr as well here? It
>> seems useful to me to perform sanity checks after calling psql.
>
> Sure, makes sense.

OK, so here is more input about this set of patches.

Patch 1 looks good. It adds some tests to cover pg_basebackup -R and
checks if standby_mode and primary_conninfo are set correctly.
Patch 2 also looks fine. It adds tests for pg_basebackup -X. Both
patches are independent on the feature.

Regarding patch 3, I have more comments:
1) I think that documentation should clearly mention that if -R and -S
are used together, a primary_slot_name entry is added in the
recovery.conf generated with the slot name defined.
2)
 sub psql
 {
my ($dbname, $sql) = @_;
-   run [ 'psql', '-X', '-q', '-d', $dbname, '-f', '-' ], '<', \$sql or die;
+   my ($stdout, $stderr);
+   run [ 'psql', '-X', '-A', '-t', '-q', '-d', $dbname, '-f', '-'
], '<', \$sql, '>', \$stdout, '2>', \$stderr or die;
+   chomp $stdout;
+   return $stdout;
 }
RewindTest.pm has a routine called check_query defined. I would be
great to extend a bit more psql than what I thought previously by
returning from it ($result, $stdout, $strerr) and make check_query
directly use it. This way we have a unique interface to run psql and
capture output. I don't mind writing this refactoring patch on top of
your set if that's necessary.
3)
+command_ok([ 'pg_basebackup', '-D', "$tempdir/backupxs_sl_R", '-X',
'stream', '-S', 'slot1', '-R' ],
+   'pg_basebackup with replication slot and -R runs');
+$recovery_conf = slurp_file "$tempdir/backupR/recovery.conf";
+like(slurp_file("$tempdir/backupxs_sl_R/recovery.conf"),
slurp_file is slurping an incorrect file and $recovery_conf is used
nowhere after, so you could simply remove this line.
Regards,
-- 
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_basebackup and replication slots

[Peter Eisentraut]

On 7/1/15 8:37 AM, Michael Paquier wrote:
> On Wed, Jul 1, 2015 at 10:33 AM, Peter Eisentraut wrote:
>> (If you're looking at the patch and wondering why there is no code to
>> actually do anything with the replication slot, that's because the code
>> that does the WAL streaming is already aware of replication slots
>> because of the pg_receivexlog functionality that it also serves.  So all
>> that needs to be done is set replication_slot.)
> 
> This is cool to see this patch taking shape.
> 
> +my ($stdout, $stderr);
> +run [ 'psql', '-X', '-A', '-t', '-q', '-d', $dbname, '-f', '-' ],
> '<', \$sql, '>', \$stdout, '2>', \$stderr or die;
> +chomp $stdout;
> +return $stdout;
> 
> Could it be possible to chomp and return $stderr as well here? It
> seems useful to me to perform sanity checks after calling psql.

Sure, makes sense.



-- 
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_basebackup and replication slots

[Michael Paquier]

On Wed, Jul 1, 2015 at 10:33 AM, Peter Eisentraut wrote:
> (If you're looking at the patch and wondering why there is no code to
> actually do anything with the replication slot, that's because the code
> that does the WAL streaming is already aware of replication slots
> because of the pg_receivexlog functionality that it also serves.  So all
> that needs to be done is set replication_slot.)

This is cool to see this patch taking shape.

+my ($stdout, $stderr);
+run [ 'psql', '-X', '-A', '-t', '-q', '-d', $dbname, '-f', '-' ],
'<', \$sql, '>', \$stdout, '2>', \$stderr or die;
+chomp $stdout;
+return $stdout;

Could it be possible to chomp and return $stderr as well here? It
seems useful to me to perform sanity checks after calling psql.
-- 
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_basebackup and replication slots

[Peter Eisentraut]

On 5/21/15 8:42 AM, Peter Eisentraut wrote:
> I wonder why pg_basebackup doesn't have any support for replication slots.
> 
> When relying on replication slots to hang on to WAL data, there is a gap
> between when pg_basebackup finishes and streaming replication is started
> where WAL data could be thrown away by the primary.
> 
> Looking at the code, the -X stream method could easily specify a
> replication slot.  (Might be nice if it could also create it in the same
> run.)

Here is a patch set for this.

(If you're looking at the patch and wondering why there is no code to
actually do anything with the replication slot, that's because the code
that does the WAL streaming is already aware of replication slots
because of the pg_receivexlog functionality that it also serves.  So all
that needs to be done is set replication_slot.)

See also the concurrent discussion on (optionally?) initializing
restart_lsn when the replication slot is created
(http://www.postgresql.org/message-id/flat/b8d538ac5587c84898b261fcb8c7d8a41fde1...@ex10-mbx-36009.ant.amazon.com#b8d538ac5587c84898b261fcb8c7d8a41fde1...@ex10-mbx-36009.ant.amazon.com),
which might have an impact on the details of this change.


>From a718282cbc18566732a0d9c3e82c8fca752f4812 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 30 Jun 2015 21:15:05 -0400
Subject: [PATCH 1/3] pg_basebackup: Add tests for -R option

---
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 9 -
 src/test/perl/TestLib.pm | 8 
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index c8c9250..0ddfffe 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -2,7 +2,7 @@
 use warnings;
 use Cwd;
 use TestLib;
-use Test::More tests => 35;
+use Test::More tests => 39;
 
 program_help_ok('pg_basebackup');
 program_version_ok('pg_basebackup');
@@ -138,3 +138,10 @@
 command_ok([ 'pg_basebackup', '-D', "$tempdir/tarbackup_l3", '-Ft' ],
 	'pg_basebackup tar with long symlink target');
 psql 'postgres', "DROP TABLESPACE tblspc3;";
+
+command_ok([ 'pg_basebackup', '-D', "$tempdir/backupR", '-R' ],
+	'pg_basebackup -R runs');
+ok(-f "$tempdir/backupR/recovery.conf", 'recovery.conf was created');
+my $recovery_conf = slurp_file "$tempdir/backupR/recovery.conf";
+like($recovery_conf, qr/^standby_mode = 'on'$/m, 'recovery.conf sets standby_mode');
+like($recovery_conf, qr/^primary_conninfo = '.*port=$ENV{PGPORT}.*'$/m, 'recovery.conf sets primary_conninfo');
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index ef42366..60bd7d5 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -11,6 +11,7 @@ our @EXPORT = qw(
   start_test_server
   restart_test_server
   psql
+  slurp_file
   system_or_bail
 
   command_ok
@@ -129,6 +130,13 @@ sub psql
 	run [ 'psql', '-X', '-q', '-d', $dbname, '-f', '-' ], '<', \$sql or die;
 }
 
+sub slurp_file
+{
+	local $/;
+	local @ARGV = @_;
+	<>
+}
+
 sub system_or_bail
 {
 	system(@_) == 0 or BAIL_OUT("system @_ failed: $?");
-- 
2.4.5

>From 934309249f1fed9fd008eccd32906fbfd763811a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 30 Jun 2015 21:15:29 -0400
Subject: [PATCH 2/3] pg_basebackup: Add tests for -X option

---
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 13 -
 src/test/perl/TestLib.pm | 10 ++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 0ddfffe..52f1575 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -2,7 +2,7 @@
 use warnings;
 use Cwd;
 use TestLib;
-use Test::More tests => 39;
+use Test::More tests => 44;
 
 program_help_ok('pg_basebackup');
 program_version_ok('pg_basebackup');
@@ -46,6 +46,10 @@
 	'pg_basebackup runs');
 ok(-f "$tempdir/backup/PG_VERSION", 'backup was created');
 
+is_deeply([sort(slurp_dir("$tempdir/backup/pg_xlog/"))],
+		  [sort qw(. .. archive_status)],
+		  'no WAL files copied');
+
 command_ok(
 	[   'pg_basebackup', '-D', "$tempdir/backup2", '--xlogdir',
 		"$tempdir/xlog2" ],
@@ -145,3 +149,10 @@
 my $recovery_conf = slurp_file "$tempdir/backupR/recovery.conf";
 like($recovery_conf, qr/^standby_mode = 'on'$/m, 'recovery.conf sets standby_mode');
 like($recovery_conf, qr/^primary_conninfo = '.*port=$ENV{PGPORT}.*'$/m, 'recovery.conf sets primary_conninfo');
+
+command_ok([ 'pg_basebackup', '-D', "$tempdir/backupxf", '-X', 'fetch' ],
+	'pg_basebackup -X fetch runs');
+ok(grep(/^[0-9A-F]{24}$/, slurp_dir("$tempdir/backupxf/pg_xlog")), 'WAL files copied');
+command_ok([ 'pg_basebackup', '-D', "$tempdir/backupxs", '-X', 'stream' ],
+	'pg_basebackup -X stream runs');
+ok(grep(/^[0-9A-F]{24}$/, slurp_dir("$tempdir/backupxf/pg_xlog")), 'WAL files copied');
diff --git a

Re: [HACKERS] pg_basebackup and replication slots

[Heikki Linnakangas]

On 05/21/2015 03:42 PM, Peter Eisentraut wrote:

I wonder why pg_basebackup doesn't have any support for replication slots.

When relying on replication slots to hang on to WAL data, there is a gap
between when pg_basebackup finishes and streaming replication is started
where WAL data could be thrown away by the primary.

Looking at the code, the -X stream method could easily specify a
replication slot.  (Might be nice if it could also create it in the same
run.)


Yeah, would be nice. The automatically-created slot should also be 
automatically removed if the backup is aborted for any reason, i.e. if 
the connection dies.


- Heikki



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

[HACKERS] pg_basebackup and replication slots

[Peter Eisentraut]

I wonder why pg_basebackup doesn't have any support for replication slots.

When relying on replication slots to hang on to WAL data, there is a gap
between when pg_basebackup finishes and streaming replication is started
where WAL data could be thrown away by the primary.

Looking at the code, the -X stream method could easily specify a
replication slot.  (Might be nice if it could also create it in the same
run.)


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