Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

Am Mittwoch, den 27.09.2017, 10:10 -0400 schrieb Peter Eisentraut:
> On 9/11/17 03:11, Michael Banck wrote:
> > So my patch only moves the slot creation slightly further forward,
> > AFAICT.
> 
> I have committed this patch, along with some associated cleanup.

Thanks!

> > AIUI, wal streaming always begins at last checkpoint and from my tests
> > the restart_lsn of the created replication slot is also before that
> > checkpoint's lsn. However, I hope somebody more familiar with the
> > WAL/replication slot code could comment on that.  What I dropped in the
> > refactoring is the RESERVE_WAL that used to be there when the temporary
> > slot gets created, I have readded that now.
> 
> I had to make some changes to that, because the way your patch was
> written it would use RESERVE_WAL also for the calls from pg_receivewal
> --create-slot, which would have been a behavior change.  So I added
> another argument to CreateReplicationSlot() to control that.

Euh right, thanks for catching that.

> > I also added a TAP test case that tries to check that the restart_lsn is
> > lower than the checkpoint_lsn, which appears to be the case.
> 
> I think that test was written incorrectly, because it didn't actually
> check the $checkpoint_lsn that it read.  I have not included that test
> in the committed patch.  Feel free to send another patch if you want to
> add more or different tests.

I will take a look at that.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Peter Eisentraut]

On 9/11/17 03:11, Michael Banck wrote:
> So my patch only moves the slot creation slightly further forward,
> AFAICT.

I have committed this patch, along with some associated cleanup.

> AIUI, wal streaming always begins at last checkpoint and from my tests
> the restart_lsn of the created replication slot is also before that
> checkpoint's lsn. However, I hope somebody more familiar with the
> WAL/replication slot code could comment on that.  What I dropped in the
> refactoring is the RESERVE_WAL that used to be there when the temporary
> slot gets created, I have readded that now.

I had to make some changes to that, because the way your patch was
written it would use RESERVE_WAL also for the calls from pg_receivewal
--create-slot, which would have been a behavior change.  So I added
another argument to CreateReplicationSlot() to control that.

> I also added a TAP test case that tries to check that the restart_lsn is
> lower than the checkpoint_lsn, which appears to be the case.

I think that test was written incorrectly, because it didn't actually
check the $checkpoint_lsn that it read.  I have not included that test
in the committed patch.  Feel free to send another patch if you want to
add more or different tests.

-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Jeff Janes]

On Thu, Sep 14, 2017 at 8:23 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 9/12/17 16:39, Michael Banck wrote:
> > We could split up the logic here and create the optional physical
> > replication slot in the main connection and the temporary one in the WAL
> > streamer connection, but this would keep any fragility around for
> > (likely more frequently used) temporary replication slots. It would make
> > the patch much smaller though if I revert touching temporary slots at
> > all.
>
> That's what I was thinking.
>
> But:
>
> If the race condition concern that Jeff was describing is indeed
> correct, then the current use of temporary replication slots would be
> equally affected.  So I think either we already have a problem, or we
> don't and then this patch wouldn't introduce a new one.
>
> I don't know the details of this well enough.
>

It is possible the race condition is entirely theoretical, as the WAL files
won't be eligible for recycling until the end of the *next* (future)
checkpoint (for no good reason, as far as I can tell, although fortunate in
this case) so that should give plenty of opportunity for the WAL streamer
to connect in all but the weirdest cases.  When it reserves the WAL, I
assume it does so back-dating to the LSN position reported by
pg_start_backup and will promptly fail if those are no longer available?

I don't want to hold up the patch based on theoretical questions when it
solves an actual problem.

Cheers,

Jeff

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Peter Eisentraut]

On 9/12/17 16:39, Michael Banck wrote:
> We could split up the logic here and create the optional physical
> replication slot in the main connection and the temporary one in the WAL
> streamer connection, but this would keep any fragility around for
> (likely more frequently used) temporary replication slots. It would make
> the patch much smaller though if I revert touching temporary slots at
> all.

That's what I was thinking.

But:

If the race condition concern that Jeff was describing is indeed
correct, then the current use of temporary replication slots would be
equally affected.  So I think either we already have a problem, or we
don't and then this patch wouldn't introduce a new one.

I don't know the details of this well enough.

Thoughts from others?

-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

Am Dienstag, den 12.09.2017, 08:53 -0400 schrieb Peter Eisentraut:
> On 9/11/17 03:11, Michael Banck wrote:
> > > Is there a race condition here?  The slot is created after the checkpoint
> > > is completed.  But you have to start streaming from the LSN where the
> > > checkpoint started, so shouldn't the slot be created before the checkpoint
> > > is started?
> > 
> > So my patch only moves the slot creation slightly further forward,
> > AFAICT.
> > 
> > AIUI, wal streaming always begins at last checkpoint and from my tests
> > the restart_lsn of the created replication slot is also before that
> > checkpoint's lsn. However, I hope somebody more familiar with the
> > WAL/replication slot code could comment on that.  What I dropped in the
> > refactoring is the RESERVE_WAL that used to be there when the temporary
> > slot gets created, I have readded that now.
> 
> Maybe there is an argument to be made here about whether this is correct
> or not, but why bother and risk the fragility?  Why not create the
> replication slot first thing.  I would put it after the server version
> checks and before we write recovery.conf.

The replication slots are created via the replication protocol through
the second background connection that is used for WAL streaming in
StartLogStreamer().

By their nature temporary replication slots must be created by that WAL
streamer using them and cannot be created by the main connection which
initiates the snapshot (or else you get a "replication slot
"pg_basebackup_XXX" is active for PID XXX" error in the WAL streamer).
So ISTM we cannot rip out CreateReplicationSlot() (or the manual
CREATE_REPLICATION_SLOT that is currently in master) from
StartLogStreamer() at least for temporary slots.

We could split up the logic here and create the optional physical
replication slot in the main connection and the temporary one in the WAL
streamer connection, but this would keep any fragility around for
(likely more frequently used) temporary replication slots. It would make
the patch much smaller though if I revert touching temporary slots at
all.

The alternative would be to call StartLogStreamer() earlier, but it
requires xlogstart as argument so we cannot move its call before the
checkpoint is taken and xlogstart is determined, the earliest I managed
was when starttli is determined, which is just few instructions earlier
than now.

Thoughts?


Michael 

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Peter Eisentraut]

On 9/11/17 03:11, Michael Banck wrote:
>> Is there a race condition here?  The slot is created after the checkpoint
>> is completed.  But you have to start streaming from the LSN where the
>> checkpoint started, so shouldn't the slot be created before the checkpoint
>> is started?
> 
> So my patch only moves the slot creation slightly further forward,
> AFAICT.
> 
> AIUI, wal streaming always begins at last checkpoint and from my tests
> the restart_lsn of the created replication slot is also before that
> checkpoint's lsn. However, I hope somebody more familiar with the
> WAL/replication slot code could comment on that.  What I dropped in the
> refactoring is the RESERVE_WAL that used to be there when the temporary
> slot gets created, I have readded that now.

Maybe there is an argument to be made here about whether this is correct
or not, but why bother and risk the fragility?  Why not create the
replication slot first thing.  I would put it after the server version
checks and before we write recovery.conf.

-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,


On Fri, Sep 08, 2017 at 08:41:56AM +0200, Michael Banck wrote:
> Am Mittwoch, den 06.09.2017, 12:22 -0400 schrieb Peter Eisentraut:
> > On 8/18/17 05:28, Michael Banck wrote:
> > > > > Rebased, squashed and slighly edited version attached. I've added this
> > > > > to the 2017-07 commitfest now as well:
> > > > > 
> > > > > https://commitfest.postgresql.org/14/1112/
> > > > 
> > > > Can you rebase this past some conflicting changes?
> > > 
> > > Thanks for letting me know, PFA a rebased version.
> > 
> > I have reviewed the thread so far.  I think there is general agreement
> > that something like this would be good to have.
> > 
> > I have not found any explanation, however, why the "if not exists"
> > behavior is desirable, let alone as the default.  I can only think of
> > two workflows here:  Either you have scripts for previous PG versions
> > that create the slot externally, in which can you omit --create, or you
> > use the new functionality to have pg_basebackup create the slot.  I
> > don't see any use for pg_basebackup to opportunistically use a slot if
> > it happens to exist.  Even if there is one, it should not be the
> > default.  So please change that.
> 
> Ok, I tried to research why that was the case and couldn't find any
> trace of a discussion either.
> 
> So we should just error out in CreateReplicationSlot() in case a slot
> exists, right? I think having yet another option like --create-if-not-
> exists does not sound needed from what you wrote above.
> 
> > A minor point, I suggest to print the message about the replication slot
> > being created *after* the slot has been created.  This aligns with how
> > logical replication subscriptions work.  
> 
> Ok.
> 
> > I don't see the need for printing a message about temporary slots. 
> > Since they are temporary, they will go away automatically, so there is
> > nothing the user needs to know there.
> 
> Ok. I thought I'd remembered some request around having this reported
> always (maybe from Magnus), but I couldn't find anything in the prior
> discussions either.
> 
> If we don't print the message for temporary slots, then the
> CreateReplicationSlot() refactoring and the addition of the
> temp_replication_slot argument would be no longer needed, or is this
> something useful on its own?

New reworked and rebased patch attached, including setting RESERVE_WAL
for physical replication slots and the additional TAP test comparing the
restart_lsn with the checkpoint_lsn.

The only thing from the above I did not change yet is the removal of the
temporary slots creation message in verbose mode.  I have the feeling
knowing the temporary slot name could be helpful for debugging and
pg_basebackup is already pretty chatty in verbose mode so I preferred to
keep it, but I can remove it of course if that is the consensus.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
>From fca622f6f7c6ef9e566c5a1b72044b824e7e02cc Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 10 Sep 2017 18:07:33 +0200
Subject: [PATCH] Add option to create a replication slot in pg_basebackup if
 not yet present.

When requesting a particular replication slot, the new pg_basebackup option
--create-slot tries to create it before starting to replicate from it unless
it exists already.

In order to allow reporting of temporary replication slot creation in --verbose
mode, further refactor the slot creation logic. Add new argument `is_temporary'
to CreateReplicationSlot() in streamutil.c which specifies whether a physical
replication slot is temporary or not. Update all callers. Move the creation of
the temporary replication slot for pg_basebackup from receivelog.c to
pg_basebackup.c and mention it in --verbose mode. At the same time, also create
the temporary slot via CreateReplicationSlot() instead of creating the
temporary slot with an explicit SQL command.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 12 ++
 src/bin/pg_basebackup/pg_basebackup.c| 56 ++--
 src/bin/pg_basebackup/pg_receivewal.c|  2 +-
 src/bin/pg_basebackup/pg_recvlogical.c   |  2 +-
 src/bin/pg_basebackup/receivelog.c   | 18 -
 src/bin/pg_basebackup/streamutil.c   | 15 +---
 src/bin/pg_basebackup/streamutil.h   |  2 +-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 34 +++--
 8 files changed, 107 insertions(+), 34 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 2454d35af3..57086295d3 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -269,6 +269,18 @@ PostgreSQL documentation
  
 
  
+  -C
+  

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

On Fri, Sep 08, 2017 at 10:30:20AM -0700, Jeff Janes wrote:
> On Wed, Sep 6, 2017 at 9:22 AM, Peter Eisentraut <
> peter.eisentr...@2ndquadrant.com> wrote:
> 
> > On 8/18/17 05:28, Michael Banck wrote:
> > >>> Rebased, squashed and slighly edited version attached. I've added this
> > >>> to the 2017-07 commitfest now as well:
> > >>>
> > >>> https://commitfest.postgresql.org/14/1112/
> > >> Can you rebase this past some conflicting changes?
> > > Thanks for letting me know, PFA a rebased version.
> >
> > I have reviewed the thread so far.  I think there is general agreement
> > that something like this would be good to have.
> >
> > I have not found any explanation, however, why the "if not exists"
> > behavior is desirable, let alone as the default.  I can only think of
> > two workflows here:  Either you have scripts for previous PG versions
> > that create the slot externally, in which can you omit --create, or you
> > use the new functionality to have pg_basebackup create the slot.  I
> > don't see any use for pg_basebackup to opportunistically use a slot if
> > it happens to exist.  Even if there is one, it should not be the
> > default.  So please change that.
> 
> +1.  I'd rather just get an error and re-run without the --create switch.
> That way you are forced to think about what you are doing.

OK.
 
> Is there a race condition here?  The slot is created after the checkpoint
> is completed.  But you have to start streaming from the LSN where the
> checkpoint started, so shouldn't the slot be created before the checkpoint
> is started?

So my patch only moves the slot creation slightly further forward,
AFAICT.

AIUI, wal streaming always begins at last checkpoint and from my tests
the restart_lsn of the created replication slot is also before that
checkpoint's lsn. However, I hope somebody more familiar with the
WAL/replication slot code could comment on that.  What I dropped in the
refactoring is the RESERVE_WAL that used to be there when the temporary
slot gets created, I have readded that now.

I also added a TAP test case that tries to check that the restart_lsn is
lower than the checkpoint_lsn, which appears to be the case.

If there is still a race condition here, do you have a suggestion in how
to try to trigger it?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
>From fca622f6f7c6ef9e566c5a1b72044b824e7e02cc Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 10 Sep 2017 18:07:33 +0200
Subject: [PATCH] Add option to create a replication slot in pg_basebackup if
 not yet present.

When requesting a particular replication slot, the new pg_basebackup option
--create-slot tries to create it before starting to replicate from it unless
it exists already.

In order to allow reporting of temporary replication slot creation in --verbose
mode, further refactor the slot creation logic. Add new argument `is_temporary'
to CreateReplicationSlot() in streamutil.c which specifies whether a physical
replication slot is temporary or not. Update all callers. Move the creation of
the temporary replication slot for pg_basebackup from receivelog.c to
pg_basebackup.c and mention it in --verbose mode. At the same time, also create
the temporary slot via CreateReplicationSlot() instead of creating the
temporary slot with an explicit SQL command.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 12 ++
 src/bin/pg_basebackup/pg_basebackup.c| 56 ++--
 src/bin/pg_basebackup/pg_receivewal.c|  2 +-
 src/bin/pg_basebackup/pg_recvlogical.c   |  2 +-
 src/bin/pg_basebackup/receivelog.c   | 18 -
 src/bin/pg_basebackup/streamutil.c   | 15 +---
 src/bin/pg_basebackup/streamutil.h   |  2 +-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 34 +++--
 8 files changed, 107 insertions(+), 34 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 2454d35af3..57086295d3 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -269,6 +269,18 @@ PostgreSQL documentation
  
 
  
+  -C
+  --create-slot
+  
+   
+This option can only be used together with the --slot
+option.  It causes the specified slot name to be created unless it
+exists already.
+   
+  
+ 
+
+ 
   -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 51509d150e..35ea707384 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -92,6 +92,8 @@ static pg_time_t 

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Jeff Janes]

On Wed, Sep 6, 2017 at 9:22 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 8/18/17 05:28, Michael Banck wrote:
> >>> Rebased, squashed and slighly edited version attached. I've added this
> >>> to the 2017-07 commitfest now as well:
> >>>
> >>> https://commitfest.postgresql.org/14/1112/
> >> Can you rebase this past some conflicting changes?
> > Thanks for letting me know, PFA a rebased version.
>
> I have reviewed the thread so far.  I think there is general agreement
> that something like this would be good to have.
>
> I have not found any explanation, however, why the "if not exists"
> behavior is desirable, let alone as the default.  I can only think of
> two workflows here:  Either you have scripts for previous PG versions
> that create the slot externally, in which can you omit --create, or you
> use the new functionality to have pg_basebackup create the slot.  I
> don't see any use for pg_basebackup to opportunistically use a slot if
> it happens to exist.  Even if there is one, it should not be the
> default.  So please change that.
>

+1.  I'd rather just get an error and re-run without the --create switch.
That way you are forced to think about what you are doing.


Is there a race condition here?  The slot is created after the checkpoint
is completed.  But you have to start streaming from the LSN where the
checkpoint started, so shouldn't the slot be created before the checkpoint
is started?

Cheers,

Jeff

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

Am Mittwoch, den 06.09.2017, 12:22 -0400 schrieb Peter Eisentraut:
> On 8/18/17 05:28, Michael Banck wrote:
> > > > Rebased, squashed and slighly edited version attached. I've added this
> > > > to the 2017-07 commitfest now as well:
> > > > 
> > > > https://commitfest.postgresql.org/14/1112/
> > > 
> > > Can you rebase this past some conflicting changes?
> > 
> > Thanks for letting me know, PFA a rebased version.
> 
> I have reviewed the thread so far.  I think there is general agreement
> that something like this would be good to have.
> 
> I have not found any explanation, however, why the "if not exists"
> behavior is desirable, let alone as the default.  I can only think of
> two workflows here:  Either you have scripts for previous PG versions
> that create the slot externally, in which can you omit --create, or you
> use the new functionality to have pg_basebackup create the slot.  I
> don't see any use for pg_basebackup to opportunistically use a slot if
> it happens to exist.  Even if there is one, it should not be the
> default.  So please change that.

Ok, I tried to research why that was the case and couldn't find any
trace of a discussion either.

So we should just error out in CreateReplicationSlot() in case a slot
exists, right? I think having yet another option like --create-if-not-
exists does not sound needed from what you wrote above.

> A minor point, I suggest to print the message about the replication slot
> being created *after* the slot has been created.  This aligns with how
> logical replication subscriptions work.  

Ok.

> I don't see the need for printing a message about temporary slots. 
> Since they are temporary, they will go away automatically, so there is
> nothing the user needs to know there.

Ok. I thought I'd remembered some request around having this reported
always (maybe from Magnus), but I couldn't find anything in the prior
discussions either.

If we don't print the message for temporary slots, then the
CreateReplicationSlot() refactoring and the addition of the
temp_replication_slot argument would be no longer needed, or is this
something useful on its own?


Thanks,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

not directly related to the topic, but:

On Tue, Mar 21, 2017 at 03:34:00PM -0400, Robert Haas wrote:
> For example, somebody creates a replica using the new super-easy
> method, and then blows it away without dropping the slot from the
> master, 

Just a thought, but maybe there should be some pg_dropstandby command or
similar, which cleans everything (what exactly?) up? That might go some
way to ensure this does not happen.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Peter Eisentraut]

On 8/18/17 05:28, Michael Banck wrote:
>>> Rebased, squashed and slighly edited version attached. I've added this
>>> to the 2017-07 commitfest now as well:
>>>
>>> https://commitfest.postgresql.org/14/1112/
>> Can you rebase this past some conflicting changes?
> Thanks for letting me know, PFA a rebased version.

I have reviewed the thread so far.  I think there is general agreement
that something like this would be good to have.

I have not found any explanation, however, why the "if not exists"
behavior is desirable, let alone as the default.  I can only think of
two workflows here:  Either you have scripts for previous PG versions
that create the slot externally, in which can you omit --create, or you
use the new functionality to have pg_basebackup create the slot.  I
don't see any use for pg_basebackup to opportunistically use a slot if
it happens to exist.  Even if there is one, it should not be the
default.  So please change that.

A minor point, I suggest to print the message about the replication slot
being created *after* the slot has been created.  This aligns with how
logical replication subscriptions work.  I don't see the need for
printing a message about temporary slots.  Since they are temporary,
they will go away automatically, so there is nothing the user needs to
know there.

With these two changes, I think I'd be content with this patch.

-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

On Tue, Aug 15, 2017 at 02:14:58PM -0700, Jeff Janes wrote:
> On Sun, Apr 9, 2017 at 4:22 AM, Michael Banck 
> wrote:
> > Rebased, squashed and slighly edited version attached. I've added this
> > to the 2017-07 commitfest now as well:
> >
> > https://commitfest.postgresql.org/14/1112/
> 
> Can you rebase this past some conflicting changes?

Thanks for letting me know, PFA a rebased version.

I also adressed the following message which appeared when starting the
TAP tests:

't/010_pg_basebackup.pl ... "my" variable $lsn masks earlier declaration
in same scope at t/010_pg_basebackup.pl line 287.'


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
>From 62973bba3338abfbf4d9e2f0f05a338eb283b4b8 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Fri, 18 Aug 2017 11:28:26 +0200
Subject: [PATCH] Add option to create a replication slot in pg_basebackup if
 not yet present.

When requesting a particular replication slot, the new pg_basebackup option
--create-slot tries to create it before starting to replicate from it in case
it does not exist already.

In order to allow reporting of temporary replication slot creation in --verbose
mode, further refactor the slot creation logic. Add new argument `is_temporary'
to CreateReplicationSlot() in streamutil.c which specifies whether a physical
replication slot is temporary or not. Update all callers. Move the creation of
the temporary replication slot for pg_basebackup from receivelog.c to
pg_basebackup.c and mention it in --verbose mode. At the same time, also create
the temporary slot via CreateReplicationSlot() instead of creating the
temporary slot with an explicit SQL command.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 12 ++
 src/bin/pg_basebackup/pg_basebackup.c| 58 ++--
 src/bin/pg_basebackup/pg_receivewal.c|  2 +-
 src/bin/pg_basebackup/pg_recvlogical.c   |  2 +-
 src/bin/pg_basebackup/receivelog.c   | 18 -
 src/bin/pg_basebackup/streamutil.c   | 18 ++---
 src/bin/pg_basebackup/streamutil.h   |  2 +-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 25 ++--
 8 files changed, 103 insertions(+), 34 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 2454d35af3..5397a185d2 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -269,6 +269,18 @@ PostgreSQL documentation
  
 
  
+  -C
+  --create-slot
+  
+   
+This option can only be used together with the --slot 
+option.  It causes the specified slot name to be created if it does not
+exist already.  
+   
+  
+ 
+
+ 
   -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 dfb9b5ddcb..2bc7e868e0 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -92,6 +92,8 @@ 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 create_slot = false;
+static bool no_slot = false;
 
 static bool success = false;
 static bool made_new_pgdata = false;
@@ -337,6 +339,7 @@ usage(void)
 			 " write recovery.conf for replication\n"));
 	printf(_("  -S, --slot=SLOTNAMEreplication slot to use\n"));
 	printf(_("  --no-slot  prevent creation of temporary replication slot\n"));
+	printf(_("  -C, --create-slot  create replication slot if not present already\n"));
 	printf(_("  -T, --tablespace-mapping=OLDDIR=NEWDIR\n"
 			 " relocate tablespace in OLDDIR to NEWDIR\n"));
 	printf(_("  -X, --wal-method=none|fetch|stream\n"
@@ -492,8 +495,6 @@ LogStreamerMain(logstreamer_param *param)
 	stream.partial_suffix = NULL;
 	stream.replication_slot = replication_slot;
 	stream.temp_slot = param->temp_slot;
-	if (stream.temp_slot && !stream.replication_slot)
-		stream.replication_slot = psprintf("pg_basebackup_%d", (int) PQbackendPID(param->bgconn));
 
 	if (format == 'p')
 		stream.walmethod = CreateWalDirectoryMethod(param->xlog, 0, do_sync);
@@ -586,6 +587,29 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
 	else
 		param->temp_slot = temp_replication_slot;
 
+	/*
+	 * Create replication slot if one is needed.
+	 */
+	if (!no_slot && (temp_replication_slot || create_slot))
+	{
+		if (!replication_slot)
+			replication_slot = psprintf("pg_basebackup_%d", (int) getpid());
+
+		if (verbose)
+		{
+			

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Jeff Janes]

On Sun, Apr 9, 2017 at 4:22 AM, Michael Banck 
wrote:

> Hi,
>
> Am Freitag, den 24.03.2017, 19:32 +0100 schrieb Michael Banck:
> > On Thu, Mar 23, 2017 at 12:41:54PM +0100, Magnus Hagander wrote:
> > > On Tue, Mar 21, 2017 at 8:34 PM, Robert Haas 
> wrote:
> > > > So I tend to think that there should always be some explicit user
> > > > action to cause the creation of a slot, like --create-slot-if-needed
> > > > or --create-slot=name.  That still won't prevent careless use of that
> > > > option but it's less dangerous than assuming that a user who refers
> to
> > > > a nonexistent slot intended to create it when, perhaps, they just
> > > > typo'd it.
> > >
> > > Well, the explicit user action would be "--slot". But sure, I can
> > > definitely live with a --create-if-not-exists.
> >
> > Can we just make that option create slot and don't worry if one exists
> > already? CreateReplicationSlot() can be told to not mind about already
> > existing slots, and I don't see a huge point in erroring out if the slot
> > exists already, unless somebody can show that this leads to data loss
> > somehow.
> >
> > > The important thing, to me, is that you can run it as a single
> > > command, which makes it a lot easier to work with. (And not like we
> > > currently have for pg_receivewal which requires a separate command to
> > > create the slot)
> >
> > Oh, that is how it works with pg_receivewal, I have to admit I've never
> > used it so was a bit confused about this when I read its code.
> >
> > So in that case I think we don't necessarily need to have the same user
> > interface at all. I first thought about just adding "-C, --create" (as
> > in "--create --slot=foo"), but this on second thought this looked a bit
> > shortsighted - who knows what flashy thing pg_basebackup might create in
> > 5 years... So I settled on --create-slot, which is only slightly more to
> > type (albeit repetive, "--create-slot --slot=foo"), but adding a short
> > option "-C" would be fine I thinkg "-C -S foo".
> >
> > So attached is a patch with adds that option. If people really think it
> > should be --create-slot-if-not-exists instead I can update the patch, of
> > course.
> >
> > I again added a second patch with some further refactoring which makes
> > it print a message on temporary slot creation in verbose mode.
>
> Rebased, squashed and slighly edited version attached. I've added this
> to the 2017-07 commitfest now as well:
>
> https://commitfest.postgresql.org/14/1112/



Can you rebase this past some conflicting changes?

Thanks,

Jeff

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

Am Freitag, den 24.03.2017, 19:32 +0100 schrieb Michael Banck:
> On Thu, Mar 23, 2017 at 12:41:54PM +0100, Magnus Hagander wrote:
> > On Tue, Mar 21, 2017 at 8:34 PM, Robert Haas  wrote:
> > > So I tend to think that there should always be some explicit user
> > > action to cause the creation of a slot, like --create-slot-if-needed
> > > or --create-slot=name.  That still won't prevent careless use of that
> > > option but it's less dangerous than assuming that a user who refers to
> > > a nonexistent slot intended to create it when, perhaps, they just
> > > typo'd it.
> > 
> > Well, the explicit user action would be "--slot". But sure, I can
> > definitely live with a --create-if-not-exists. 
> 
> Can we just make that option create slot and don't worry if one exists
> already? CreateReplicationSlot() can be told to not mind about already
> existing slots, and I don't see a huge point in erroring out if the slot
> exists already, unless somebody can show that this leads to data loss
> somehow.
> 
> > The important thing, to me, is that you can run it as a single
> > command, which makes it a lot easier to work with. (And not like we
> > currently have for pg_receivewal which requires a separate command to
> > create the slot)
> 
> Oh, that is how it works with pg_receivewal, I have to admit I've never
> used it so was a bit confused about this when I read its code.
> 
> So in that case I think we don't necessarily need to have the same user
> interface at all. I first thought about just adding "-C, --create" (as
> in "--create --slot=foo"), but this on second thought this looked a bit
> shortsighted - who knows what flashy thing pg_basebackup might create in
> 5 years... So I settled on --create-slot, which is only slightly more to
> type (albeit repetive, "--create-slot --slot=foo"), but adding a short
> option "-C" would be fine I thinkg "-C -S foo".
> 
> So attached is a patch with adds that option. If people really think it
> should be --create-slot-if-not-exists instead I can update the patch, of
> course.
> 
> I again added a second patch with some further refactoring which makes
> it print a message on temporary slot creation in verbose mode.

Rebased, squashed and slighly edited version attached. I've added this
to the 2017-07 commitfest now as well:

https://commitfest.postgresql.org/14/1112/


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
From 47fc0d543a43441f239f109d08bfc7c082f4c091 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Fri, 24 Mar 2017 18:27:47 +0100
Subject: [PATCH] Add option to create a replication slot in pg_basebackup if
 not yet present.

When requesting a particular replication slot, the new pg_basebackup option
--create-slot tries to create it before starting to replicate from it in case
it does not exist already.

In order to allow reporting of temporary replication slot creation in --verbose
mode, further refactor the slot creation logic. Add new argument `is_temporary'
to CreateReplicationSlot() in streamutil.c which specifies whether a physical
replication slot is temporary or not. Update all callers. Move the creation of
the temporary replication slot for pg_basebackup from receivelog.c to
pg_basebackup.c and mention it in --verbose mode. At the same time, also create
the temporary slot via CreateReplicationSlot() instead of creating the
temporary slot with an explicit SQL command.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 12 ++
 src/bin/pg_basebackup/pg_basebackup.c| 58 ++--
 src/bin/pg_basebackup/pg_receivewal.c|  2 +-
 src/bin/pg_basebackup/pg_recvlogical.c   |  2 +-
 src/bin/pg_basebackup/receivelog.c   | 18 -
 src/bin/pg_basebackup/streamutil.c   | 18 ++---
 src/bin/pg_basebackup/streamutil.h   |  2 +-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 23 +--
 8 files changed, 102 insertions(+), 33 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index e1cec9d..65ca8dc 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -269,6 +269,18 @@ PostgreSQL documentation
  
 
  
+  -C
+  --create-slot
+  
+   
+This option can only be used together with the --slot 
+option.  It causes the specified slot name to be created if it does not
+exist already.  
+   
+  
+ 
+
+ 
   -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 40ec0e1..6442a54 100644
--- 

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

On Thu, Mar 23, 2017 at 12:41:54PM +0100, Magnus Hagander wrote:
> On Tue, Mar 21, 2017 at 8:34 PM, Robert Haas  wrote:
> > So I tend to think that there should always be some explicit user
> > action to cause the creation of a slot, like --create-slot-if-needed
> > or --create-slot=name.  That still won't prevent careless use of that
> > option but it's less dangerous than assuming that a user who refers to
> > a nonexistent slot intended to create it when, perhaps, they just
> > typo'd it.
> 
> Well, the explicit user action would be "--slot". But sure, I can
> definitely live with a --create-if-not-exists. 

Can we just make that option create slot and don't worry if one exists
already? CreateReplicationSlot() can be told to not mind about already
existing slots, and I don't see a huge point in erroring out if the slot
exists already, unless somebody can show that this leads to data loss
somehow.

> The important thing, to me, is that you can run it as a single
> command, which makes it a lot easier to work with. (And not like we
> currently have for pg_receivewal which requires a separate command to
> create the slot)

Oh, that is how it works with pg_receivewal, I have to admit I've never
used it so was a bit confused about this when I read its code.

So in that case I think we don't necessarily need to have the same user
interface at all. I first thought about just adding "-C, --create" (as
in "--create --slot=foo"), but this on second thought this looked a bit
shortsighted - who knows what flashy thing pg_basebackup might create in
5 years... So I settled on --create-slot, which is only slightly more to
type (albeit repetive, "--create-slot --slot=foo"), but adding a short
option "-C" would be fine I thinkg "-C -S foo".

So attached is a patch with adds that option. If people really think it
should be --create-slot-if-not-exists instead I can update the patch, of
course.

I again added a second patch with some further refactoring which makes
it print a message on temporary slot creation in verbose mode.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
>From 4e37e110ac402e67874f729832b330a837284d4b Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Fri, 24 Mar 2017 18:27:47 +0100
Subject: [PATCH 1/2] Add option to create a replication slot in pg_basebackup
 if not yet present.

When reqeusting a particular replication slot, the new option --create tries to
create it before starting to replicate from it in case it does not exist
already.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 12 
 src/bin/pg_basebackup/pg_basebackup.c| 44 +++-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 23 +--
 3 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index e1cec9d..789f649 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -269,6 +269,18 @@ PostgreSQL documentation
  
 
  
+  -C
+  --create-slot
+  
+   
+This option can only be used together with the --slot 
+	option.  It causes the specified slot name to be created if it does not
+exist already.  
+   
+  
+ 
+
+ 
   -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 0a4944d..2af2e22 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -92,6 +92,7 @@ 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 create_slot = false;
 
 static bool success = false;
 static bool made_new_pgdata = false;
@@ -337,6 +338,7 @@ usage(void)
 			 " write recovery.conf for replication\n"));
 	printf(_("  -S, --slot=SLOTNAMEreplication slot to use\n"));
 	printf(_("  --no-slot  prevent creation of temporary replication slot\n"));
+	printf(_("  -C, --create-slot  create replication slot if not present already\n"));
 	printf(_("  -T, --tablespace-mapping=OLDDIR=NEWDIR\n"
 	  " relocate tablespace in OLDDIR to NEWDIR\n"));
 	printf(_("  -X, --wal-method=none|fetch|stream\n"
@@ -581,6 +583,19 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
 	else
 		param->temp_slot = temp_replication_slot;
 
+	/*
+	 * Create permanent physical replication slot if requested.
+	 */
+	if (replication_slot && create_slot)
+	{
+		if (verbose)
+			fprintf(stderr, 

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Magnus Hagander]

On Tue, Mar 21, 2017 at 8:34 PM, Robert Haas  wrote:

> On Sun, Mar 19, 2017 at 12:01 PM, Magnus Hagander 
> wrote:
> > I think maybe we should output a message when the slot is created, at
> least
> > in verbose mode, to make sure people realize that happened. Does that
> seem
> > reasonable?
>
> Slots are great until you leave one lying around by accident.  I'm
> afraid that no matter what we do, we're going to start getting
> complaints from people who mess that up.  For example, somebody
> creates a replica using the new super-easy method, and then blows it
> away without dropping the slot from the master, and then days or weeks
> later pg_wal fills up and takes the server down.  The user says, oh,
> these old write-ahead log files should have gotten removed, and
> removes them all.  Oops.


> So I tend to think that there should always be some explicit user
> action to cause the creation of a slot, like --create-slot-if-needed
> or --create-slot=name.  That still won't prevent careless use of that
> option but it's less dangerous than assuming that a user who refers to
> a nonexistent slot intended to create it when, perhaps, they just
> typo'd it.
>

Well, the explicit user action would be "--slot". But sure, I can
definitely live with a --create-if-not-exists. The important thing, to me,
is that you can run it as a single command, which makes it a lot easier to
work with. (And not like we currently have for pg_receivewal which requires
a separate command to create the slot)

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Kyotaro HORIGUCHI]

Hello, I favor this feature.

At Wed, 22 Mar 2017 00:18:19 -0400, Peter Eisentraut 
 wrote in 
<1f5daba9-773d-9281-5608-37f049025...@2ndquadrant.com>
> On 3/21/17 15:34, Robert Haas wrote:
> > So I tend to think that there should always be some explicit user
> > action to cause the creation of a slot, like --create-slot-if-needed
> > or --create-slot=name.  That still won't prevent careless use of that
> > option but it's less dangerous than assuming that a user who refers to
> > a nonexistent slot intended to create it when, perhaps, they just
> > typo'd it.
> 
> I have the same concern.

A slot created as !immeediately_reserve (even though currently
CREATE_REPLICATION_SLOT command doesn't seem to have the option)
won't do such a trick but I agree to the point. I think that any
explicit action is required unless any anticipated catastrophic
end caused by remainig slots is evaded implicitly.

Do ephemeral or temporary slots help this case?

Otherwise, I'm proposing a patch to ignore restart-lsn of slots
that let too many WALs to stay on.

https://www.postgresql.org/message-id/20170228.122736.123383594.horiguchi.kyot...@lab.ntt.co.jp

Instaed just ignoring restart-lsn, like Andres' suggestion,
removing (or just disabling) a slot that is marked as
'auto-removable' with the same kind (including disconnection
timeout) of trigger also will work.


I had a similar annoyance with CREATE SUBSCRIPTION. It implicitly
creates a slot on publisher and subscriber fails to have the same
subscription after re-initialization. Of couse DROP SUBSCRIPTION
doesn't help the case. Users don't have a clue to solution, I
suppose. But this would be another topic.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Peter Eisentraut]

On 3/21/17 15:34, Robert Haas wrote:
> So I tend to think that there should always be some explicit user
> action to cause the creation of a slot, like --create-slot-if-needed
> or --create-slot=name.  That still won't prevent careless use of that
> option but it's less dangerous than assuming that a user who refers to
> a nonexistent slot intended to create it when, perhaps, they just
> typo'd it.

I have the same concern.

-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

Am Mittwoch, den 22.03.2017, 00:40 +0100 schrieb Michael Banck:
> I guess if we decide (physical) slots should not be created implicitly,
> then using the same UI as pg_receivewal makes sense for the sake of
> consistency, i.e. "--slot=name --create-slot [--if-not-exists]". That is
> rather verbose though and I don't think the --if-not-exists is great UX,
> but maybe there is some use-case for erroring out if a slot already
> exists that I haven't thought of.

Thinking about this some more, there's the case of somebody accidentally
using an already existing slot that was meant for another standby which
happens to be down just at that moment.

>From some quick testing, that makes replication fail once the other
standby is started up again, but soon after the new standby is taken
down, replication picks up without apparent problems for the other
standby.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Am Dienstag, den 21.03.2017, 15:34 -0400 schrieb Robert Haas:
> On Sun, Mar 19, 2017 at 12:01 PM, Magnus Hagander  wrote:
> > I think maybe we should output a message when the slot is created, at least
> > in verbose mode, to make sure people realize that happened. Does that seem
> > reasonable?
> 
> Slots are great until you leave one lying around by accident.  I'm
> afraid that no matter what we do, we're going to start getting
> complaints from people who mess that up.  For example, somebody
> creates a replica using the new super-easy method, and then blows it
> away without dropping the slot from the master, and then days or weeks
> later pg_wal fills up and takes the server down.  The user says, oh,
> these old write-ahead log files should have gotten removed, and
> removes them all.  Oops.

Hrm. 

Maybe it would be useful to log a warning like "slot XY has not been
active for X minutes", based on some threshold, though possibly the
people not keeping an eye on their replication slots won't spot that in
the logfiles, either.

> So I tend to think that there should always be some explicit user
> action to cause the creation of a slot, like --create-slot-if-needed
> or --create-slot=name.  That still won't prevent careless use of that
> option but it's less dangerous than assuming that a user who refers to
> a nonexistent slot intended to create it when, perhaps, they just
> typo'd it.

I guess if we decide (physical) slots should not be created implicitly,
then using the same UI as pg_receivewal makes sense for the sake of
consistency, i.e. "--slot=name --create-slot [--if-not-exists]". That is
rather verbose though and I don't think the --if-not-exists is great UX,
but maybe there is some use-case for erroring out if a slot already
exists that I haven't thought of.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Robert Haas]

On Sun, Mar 19, 2017 at 12:01 PM, Magnus Hagander  wrote:
> I think maybe we should output a message when the slot is created, at least
> in verbose mode, to make sure people realize that happened. Does that seem
> reasonable?

Slots are great until you leave one lying around by accident.  I'm
afraid that no matter what we do, we're going to start getting
complaints from people who mess that up.  For example, somebody
creates a replica using the new super-easy method, and then blows it
away without dropping the slot from the master, and then days or weeks
later pg_wal fills up and takes the server down.  The user says, oh,
these old write-ahead log files should have gotten removed, and
removes them all.  Oops.

So I tend to think that there should always be some explicit user
action to cause the creation of a slot, like --create-slot-if-needed
or --create-slot=name.  That still won't prevent careless use of that
option but it's less dangerous than assuming that a user who refers to
a nonexistent slot intended to create it when, perhaps, they just
typo'd it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Am Dienstag, den 21.03.2017, 12:52 +0100 schrieb Michael Banck:
> New patches attached.

On second though, there was a superfluous whitespace change in
t/010_pg_basebackup.pl, and I've moved the check-for-hex regex fix to
the second patch as it's cosmetic and not related to changing the --slot
creation behaviour.


Michael
 
-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

From 2ab1be27a341ecdcd2d6a3dd5ce535aba5e16b03 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 19 Mar 2017 10:58:13 +0100
Subject: [PATCH 01/29] Create replication slot in pg_basebackup if requested
 and not yet present.

If a replication slot is explicitly requested, try to create it before
starting to replicate from it.
---
 src/bin/pg_basebackup/pg_basebackup.c| 15 +++
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 15 ++-
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 0a4944d..69ca4be 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -581,6 +581,21 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
 	else
 		param->temp_slot = temp_replication_slot;
 
+	/*
+	 * Try to create a permanent replication slot if one is specified. Do
+	 * not error out if the slot already exists, other errors are already
+	 * reported by CreateReplicationSlot().
+	 */
+	if (!temp_replication_slot && replication_slot)
+	{
+		if (verbose)
+			fprintf(stderr, _("%s: creating replication slot \"%s\"\n"),
+	progname, replication_slot);
+
+		if (!CreateReplicationSlot(param->bgconn, replication_slot, NULL, true, true))
+			return 1;
+	}
+
 	if (format == 'p')
 	{
 		/*
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 14bd813..70c3284 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 => 72;
+use Test::More tests => 73;
 
 program_help_ok('pg_basebackup');
 program_version_ok('pg_basebackup');
@@ -246,14 +246,19 @@ $node->command_ok(
 	'pg_basebackup -X stream runs with --no-slot');
 
 $node->command_fails(
-	[ 'pg_basebackup', '-D', "$tempdir/fail", '-S', 'slot1' ],
+	[ 'pg_basebackup', '-D', "$tempdir/fail", '-X', 'none', '-S', 'slot0' ],
 	'pg_basebackup with replication slot fails without -X stream');
-$node->command_fails(
+$node->command_ok(
 	[   'pg_basebackup', '-D',
 		"$tempdir/backupxs_sl_fail", '-X',
 		'stream','-S',
-		'slot1' ],
-	'pg_basebackup fails with nonexistent replication slot');
+		'slot0' ],
+	'pg_basebackup -S creates previously nonexistent replication slot');
+
+my $lsn = $node->safe_psql('postgres',
+	q{SELECT restart_lsn FROM pg_replication_slots WHERE slot_name = 'slot0'}
+);
+like($lsn, qr!^0/[0-9A-F]{7,8}$!, 'slot is present');
 
 $node->safe_psql('postgres',
 	q{SELECT * FROM pg_create_physical_replication_slot('slot1')});
-- 
2.1.4

From 5f0f31f61bf668de937868840a97c0a56dc2cd5d Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Tue, 21 Mar 2017 12:50:22 +0100
Subject: [PATCH 02/29] Refactor replication slot creation in pg_basebackup et
 al.

Add new argument `is_temporary' to CreateReplicationSlot() in streamutil.c
which specifies whether a physical replication slot is temporary or not. Update
all callers.

Move the creation of the temporary replication slot for pg_basebackup from
receivelog.c to pg_basebackup.c. At the same time, also create the temporary
slot via CreateReplicationSlot() instead of creating the temporary slot with an
explicit SQL command.
---
 src/bin/pg_basebackup/pg_basebackup.c| 28 +---
 src/bin/pg_basebackup/pg_receivewal.c|  2 +-
 src/bin/pg_basebackup/pg_recvlogical.c   |  2 +-
 src/bin/pg_basebackup/receivelog.c   | 18 --
 src/bin/pg_basebackup/streamutil.c   | 18 +-
 src/bin/pg_basebackup/streamutil.h   |  2 +-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl |  2 +-
 7 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 69ca4be..c7ae281 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -92,6 +92,7 @@ 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 no_slot = false;
 
 

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

Am Dienstag, den 21.03.2017, 11:03 +0900 schrieb Michael Paquier:
> On Tue, Mar 21, 2017 at 1:32 AM, Michael Banck
>  wrote:
>  /*
> + * Try to create a permanent replication slot if one is specified. Do
> + * not error out if the slot already exists, other errors are already
> + * reported by CreateReplicationSlot().
> + */
> +if (!stream->temp_slot && stream->replication_slot)
> +{
> +if (!CreateReplicationSlot(conn, stream->replication_slot,
> NULL, true, true))
> +return false;
> +}
> This could be part of an else taken from the previous if where
> temp_slot is checked.

Not sure how this would work, as ISTM the if clause above only sets the
name for param->temp_slot (if supported), but doesn't distinguish which
kind of slot (if any) will actually be created. 

I've done it for the second (refactoring) patch though, but I had to
make `no_slot' a global variable to not have the logic be too
complicated.

> There should be some TAP tests included. And +1 for sharing the same
> behavior as pg_receivewal.

Well, I've adjusted the TAP tests in so far as it's now checking that
the physical slot got created, previously it checked for the opposite:

|-$node->command_fails(
|+$node->command_ok(
|[   'pg_basebackup', '-D',
|"$tempdir/backupxs_sl_fail", '-X',
|'stream','-S',
|-   'slot1' ],
|-   'pg_basebackup fails with nonexistent replication slot');
|+   'slot0' ],
|+   'pg_basebackup runs with nonexistent replication slot');
|+
|+my $lsn = $node->safe_psql('postgres',
|+   q{SELECT restart_lsn FROM pg_replication_slots WHERE slot_name
|= 'slot0'}
|+);
|+like($lsn, qr!^0/[0-9A-F]{7,8}$!, 'slot is present');

I have now made the message a bit clearer by saying "pg_basebackup -S
creates previously nonexistent replication slot".

Probably there could be additional TAP tests, but off the top of my head
I can't think of any?

New patches attached.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
From 93337fe0ef320fe8ef68a9b64c4df85a63c4346c Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 19 Mar 2017 10:58:13 +0100
Subject: [PATCH 1/2] Create replication slot in pg_basebackup if requested and
 not yet present.

If a replication slot is explicitly requested, try to create it before
starting to replicate from it.
---
 src/bin/pg_basebackup/pg_basebackup.c| 15 +++
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 19 ---
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 0a4944d..69ca4be 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -581,6 +581,21 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
 	else
 		param->temp_slot = temp_replication_slot;
 
+	/*
+	 * Try to create a permanent replication slot if one is specified. Do
+	 * not error out if the slot already exists, other errors are already
+	 * reported by CreateReplicationSlot().
+	 */
+	if (!temp_replication_slot && replication_slot)
+	{
+		if (verbose)
+			fprintf(stderr, _("%s: creating replication slot \"%s\"\n"),
+	progname, replication_slot);
+
+		if (!CreateReplicationSlot(param->bgconn, replication_slot, NULL, true, true))
+			return 1;
+	}
+
 	if (format == 'p')
 	{
 		/*
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 14bd813..f50005f 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 => 72;
+use Test::More tests => 73;
 
 program_help_ok('pg_basebackup');
 program_version_ok('pg_basebackup');
@@ -246,17 +246,22 @@ $node->command_ok(
 	'pg_basebackup -X stream runs with --no-slot');
 
 $node->command_fails(
-	[ 'pg_basebackup', '-D', "$tempdir/fail", '-S', 'slot1' ],
+	[ 'pg_basebackup', '-D', "$tempdir/fail", '-X', 'none', '-S', 'slot0' ],
 	'pg_basebackup with replication slot fails without -X stream');
-$node->command_fails(
+$node->command_ok(
 	[   'pg_basebackup', '-D',
 		"$tempdir/backupxs_sl_fail", '-X',
 		'stream','-S',
-		'slot1' ],
-	'pg_basebackup fails with nonexistent replication slot');
+		'slot0' ],
+	'pg_basebackup -S creates previously nonexistent replication slot');
+
+my $lsn = $node->safe_psql('postgres',
+	q{SELECT restart_lsn FROM pg_replication_slots WHERE slot_name = 'slot0'}
+);
+like($lsn, 

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Michael Paquier]

On Tue, Mar 21, 2017 at 1:32 AM, Michael Banck
 wrote:
> On Mon, Mar 20, 2017 at 02:42:32PM +0300, Arthur Zakirov wrote:
>> Also maybe it would be good if pg_basebackup had a way to drop created slot.
>> Although "drop slot" is not related with concept of automatically created
>> slots, it will good if user will have a way to drop slots.
>
> If you want to drop the slot after basebackup finishes you'd just use a
> temporary slot (i.e. the default), or am I understanding your use-case
> incorrectly?

Yup, agreed.

> I assumed the primary use-case for creating a non-temporary slot is to
> keep it around while the standby is active.

Indeed.

 /*
+ * Try to create a permanent replication slot if one is specified. Do
+ * not error out if the slot already exists, other errors are already
+ * reported by CreateReplicationSlot().
+ */
+if (!stream->temp_slot && stream->replication_slot)
+{
+if (!CreateReplicationSlot(conn, stream->replication_slot,
NULL, true, true))
+return false;
+}
This could be part of an else taken from the previous if where
temp_slot is checked.

There should be some TAP tests included. And +1 for sharing the same
behavior as pg_receivewal.
-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

On Mon, Mar 20, 2017 at 02:42:32PM +0300, Arthur Zakirov wrote:
> Also maybe it would be good if pg_basebackup had a way to drop created slot.
> Although "drop slot" is not related with concept of automatically created
> slots, it will good if user will have a way to drop slots.

If you want to drop the slot after basebackup finishes you'd just use a
temporary slot (i.e. the default), or am I understanding your use-case
incorrectly?

I assumed the primary use-case for creating a non-temporary slot is to
keep it around while the standby is active.


Michael


-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Arthur Zakirov]

Hello,

On 19.03.2017 21:45, Michael Banck wrote:


So the patch I sent earlier creates the slot in ReceiveXlogStream() in
receivewal.c, as that's where the temp slot gets created as well, but
now I wonder whether that is maybe not the best place, as pg_receivewal
also calls that function. The other problem with receivewal.c is that
`verbose' isn't around in it so I don't how I'd print out a message
there.

So probably it is better to create the slot in pg_basebackup.c's
StartLogStreamer(), see the attached first patch, that one also adds
a verbose message.


I think such too. I suppose it is more clearly. StartLogStreamer() is 
better place for creating permanent and temporary slots.


Also maybe it would be good if pg_basebackup had a way to drop created 
slot. Although "drop slot" is not related with concept of automatically 
created slots, it will good if user will have a way to drop slots.


--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


--
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] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

On Sun, Mar 19, 2017 at 05:01:23PM +0100, Magnus Hagander wrote:
> On Sun, Mar 19, 2017 at 11:21 AM, Michael Banck 
> wrote:
> > So I propose the attached tiny patch to just create the slot (if it does
> > not exist already) in pg_basebackup, somewhat similar to what
> > pg_receivewal does, albeit unconditionally, if the user explicitly
> > requested a slot:
> >
> > $ pg_basebackup --pgdata=data2 --write-recovery-conf --slot=pg2
> > $ echo $?
> > 0
> > $ psql -c "DROP_REPLICATION_SLOT pg2" "host=127.0.0.1 port=5432
> > replication=database user=mba dbname=postgres"
> > SELECT
> > $
> >
> > This would get us somewhat closer to near zero-config replication, in my
> > opinion. Pardon me if that was discussed already and shot down
> >
> > Comments?
> 
> I think this is a good idea, since it makes replication slots easier to
> use. The change to use temporary slots was good for backups, but didn't
> help people setting up replicas.
> 
> I've been annoyed for a while we didn't have a "create slot" mode in
> pg_basebackup, but doing it integrated like this is definitely a step even
> better than that.

Great!
 
> I think maybe we should output a message when the slot is created, at least
> in verbose mode, to make sure people realize that happened. Does that seem
> reasonable?

So the patch I sent earlier creates the slot in ReceiveXlogStream() in
receivewal.c, as that's where the temp slot gets created as well, but
now I wonder whether that is maybe not the best place, as pg_receivewal
also calls that function. The other problem with receivewal.c is that
`verbose' isn't around in it so I don't how I'd print out a message
there.

So probably it is better to create the slot in pg_basebackup.c's
StartLogStreamer(), see the attached first patch, that one also adds
a verbose message.

I also now realized I didn't ran the TAP tests and they need updating,
this has been done in the first attached patch as well. Independently of
this patch series I think those two hunks from the third patch are
applicable to current master as well:

 $node->command_fails(
-   [ 'pg_basebackup', '-D', "$tempdir/fail", '-S', 'slot1' ],
+   [ 'pg_basebackup', '-D', "$tempdir/fail", '-X', 'none', '-S', 'slot0' ],
'pg_basebackup with replication slot fails without -X stream');

as '-X stream' is now the default, and (more cosmetically)

-like($lsn, qr!^0/[0-9A-Z]{7,8}$!, 'restart LSN of slot has advanced');
+like($lsn, qr!^0/[0-9A-F]{7,8}$!, 'restart LSN of slot has advanced');

as we are looking for hex values.

However, the other thing to ponder is that we don't really know whether
the user maybe setup a replication slot on the primary in preparation
already as there seems to be no way to query the list of current slots
via the replication protocal, and setting up another normal connection
just to query pg_replication_slots seems to be overkill. So maybe the
user would be confused then why the slot is created, but IDK. 

If there are other uses for querying the available replication slots,
maybe the protocol could be extended to that end for 11.

Finally, it is a bit inconsitent that we'd report the creation of the
permanent slot, but not the temporary one. 

I took a look and it seems the main reason why ReceiveXlogStream() does
not call streamutil,c's CreateReplicationSlot() seems to be that the
latter has no notion of temporary slots yet.  I'm attaching a second
patch which refactors things a bit more, adding a `is_temporary'
argument to CreateReplicationSlot() and moving the creation of the
temporary slot to pg_basebackup.c's StartLogStreamer() as well (as
pg_recvlogical and pg_receivewal do not deal with temp slots anyway).
This way, the creation of the temporary slot can be mention on --verbose
as well.

Personally, I think it'd be good to be able to have the --slot option
just work in 10, so if the first patch is still acceptable (pending
review) for 10 but not the second, that'd be entirely fine.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
>From 835e6fe3e30d534bc5918d1d6c399074a9a13626 Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sun, 19 Mar 2017 10:58:13 +0100
Subject: [PATCH 1/2] Create replication slot in pg_basebackup if requested and
 not yet present.

If a replication slot is explicitly requested, try to create it before
starting to replicate from it.
---
 src/bin/pg_basebackup/pg_basebackup.c| 15 +++
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 19 ---
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 0a4944d..69ca4be 100644
--- 

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Magnus Hagander]

On Sun, Mar 19, 2017 at 11:21 AM, Michael Banck 
wrote:

> Hi,
>
> with the default configuration improvements so far for 10, it seems to
> be very easy to setup streaming replication (at least locally):
>
> $ initdb --pgdata=data1
> $ pg_ctl --pgdata=data1 start
> $ pg_basebackup --pgdata=data2 --write-recovery-conf
> $ sed -i -e 's/^#port.=.5432/port = 5433/' \
> > -e 's/^#hot_standby.=.off/hot_standby = on/' \
> > data2/postgresql.conf
> $ pg_ctl --pgdata=data2 start
>
> (there might be a case for having hot_standby=on by default, but I think
> that got discussed elsewhere and is anyway a different thread).
>
> However, the above does not use replication slots, and if you want to do
> so, you get:
>
> $ pg_basebackup --pgdata=data2 --write-recovery-conf --slot=pg2
> 2017-03-19 11:04:37.978 CET [25362] ERROR:  replication slot "pg2" does
> not exist
> pg_basebackup: could not send replication command "START_REPLICATION":
> ERROR:  replication slot "pg2" does not exist
> pg_basebackup: child process exited with error 1
> pg_basebackup: removing data directory "data2"
>
> The error message is clear enough, but I wonder whether people will
> start writing streaming replication tutorials just glossing over this
> because it's one more step to run CREATE_REPLICATION_SLOT manually.
>
> So I propose the attached tiny patch to just create the slot (if it does
> not exist already) in pg_basebackup, somewhat similar to what
> pg_receivewal does, albeit unconditionally, if the user explicitly
> requested a slot:
>
> $ pg_basebackup --pgdata=data2 --write-recovery-conf --slot=pg2
> $ echo $?
> 0
> $ psql -c "DROP_REPLICATION_SLOT pg2" "host=127.0.0.1 port=5432
> replication=database user=mba dbname=postgres"
> SELECT
> $
>
> This would get us somewhat closer to near zero-config replication, in my
> opinion. Pardon me if that was discussed already and shot down
>
> Comments?
>

I think this is a good idea, since it makes replication slots easier to
use. The change to use temporary slots was good for backups, but didn't
help people setting up replicas.

I've been annoyed for a while we didn't have a "create slot" mode in
pg_basebackup, but doing it integrated like this is definitely a step even
better than that.

I think maybe we should output a message when the slot is created, at least
in verbose mode, to make sure people realize that happened. Does that seem
reasonable?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/