Re: [HACKERS] Create replication slot in pg_basebackup if requestedand not yet present - Mailing list pgsql-hackers

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 <michael.banck@credativ.de>
> 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.banck@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