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.ba...@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.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 <michael.ba...@credativ.de>
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..29592fd 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 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');
$node->safe_psql('postgres',
- q{SELECT * FROM pg_create_physical_replication_slot('slot1')});
+ q{SELECT * FROM pg_create_physical_replication_slot('slot1')});
my $lsn = $node->safe_psql('postgres',
q{SELECT restart_lsn FROM pg_replication_slots WHERE slot_name = 'slot1'}
);
@@ -268,7 +273,7 @@ $node->command_ok(
$lsn = $node->safe_psql('postgres',
q{SELECT restart_lsn FROM pg_replication_slots WHERE slot_name = 'slot1'}
);
-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');
$node->command_ok(
[ 'pg_basebackup', '-D', "$tempdir/backupxs_sl_R", '-X',
--
2.1.4
>From 75c5e59ef08e1408c2130bda77c39773f659f760 Mon Sep 17 00:00:00 2001
From: Michael Banck <michael.ba...@credativ.de>
Date: Sun, 19 Mar 2017 19:44:25 +0100
Subject: [PATCH 2/2] 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, call CreateReplicationSlot()
instead of creating the temporary slot with an explicit SQL command.
---
src/bin/pg_basebackup/pg_basebackup.c | 22 ++++++++++++++++++----
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 +-
6 files changed, 34 insertions(+), 30 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 69ca4be..622099c 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -487,8 +487,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) getpid());
if (format == 'p')
stream.walmethod = CreateWalDirectoryMethod(param->xlog, 0, do_sync);
@@ -582,6 +580,22 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
param->temp_slot = temp_replication_slot;
/*
+ * Create temporary replication slot if one is needed
+ */
+ if (temp_replication_slot)
+ {
+ if (!replication_slot)
+ replication_slot = psprintf("pg_basebackup_%d", (int) getpid());
+
+ if (verbose)
+ fprintf(stderr, _("%s: creating temporary replication slot \"%s\"\n"),
+ progname, replication_slot);
+
+ if (!CreateReplicationSlot(param->bgconn, replication_slot, NULL, true, true, false))
+ disconnect_and_exit(1);
+ }
+
+ /*
* 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().
@@ -592,8 +606,8 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
fprintf(stderr, _("%s: creating replication slot \"%s\"\n"),
progname, replication_slot);
- if (!CreateReplicationSlot(param->bgconn, replication_slot, NULL, true, true))
- return 1;
+ if (!CreateReplicationSlot(param->bgconn, replication_slot, NULL, true, false, true))
+ disconnect_and_exit(1);
}
if (format == 'p')
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 15348ad..d221633 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -699,7 +699,7 @@ main(int argc, char **argv)
progname, replication_slot);
if (!CreateReplicationSlot(conn, replication_slot, NULL, true,
- slot_exists_ok))
+ false, slot_exists_ok))
disconnect_and_exit(1);
disconnect_and_exit(0);
}
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 6b081bd..14ce537 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -980,7 +980,7 @@ main(int argc, char **argv)
progname, replication_slot);
if (!CreateReplicationSlot(conn, replication_slot, plugin,
- false, slot_exists_ok))
+ false, false, slot_exists_ok))
disconnect_and_exit(1);
startpos = InvalidXLogRecPtr;
}
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index f415135..d884793 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -509,24 +509,6 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
}
/*
- * Create temporary replication slot if one is needed
- */
- if (stream->temp_slot)
- {
- snprintf(query, sizeof(query),
- "CREATE_REPLICATION_SLOT \"%s\" TEMPORARY PHYSICAL RESERVE_WAL",
- stream->replication_slot);
- res = PQexec(conn, query);
- if (PQresultStatus(res) != PGRES_TUPLES_OK)
- {
- fprintf(stderr, _("%s: could not create temporary replication slot \"%s\": %s"),
- progname, stream->replication_slot, PQerrorMessage(conn));
- PQclear(res);
- return false;
- }
- }
-
- /*
* initialize flush position to starting point, it's the caller's
* responsibility that that's sane.
*/
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 507da5e..67d801a 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -322,7 +322,7 @@ RunIdentifySystem(PGconn *conn, char **sysid, TimeLineID *starttli,
*/
bool
CreateReplicationSlot(PGconn *conn, const char *slot_name, const char *plugin,
- bool is_physical, bool slot_exists_ok)
+ bool is_physical, bool is_temporary, bool slot_exists_ok)
{
PQExpBuffer query;
PGresult *res;
@@ -335,12 +335,20 @@ CreateReplicationSlot(PGconn *conn, const char *slot_name, const char *plugin,
/* Build query */
if (is_physical)
- appendPQExpBuffer(query, "CREATE_REPLICATION_SLOT \"%s\" PHYSICAL",
- slot_name);
+ if (is_temporary)
+ appendPQExpBuffer(query, "CREATE_REPLICATION_SLOT \"%s\" TEMPORARY PHYSICAL",
+ slot_name);
+ else
+ appendPQExpBuffer(query, "CREATE_REPLICATION_SLOT \"%s\" PHYSICAL",
+ slot_name);
else
{
- appendPQExpBuffer(query, "CREATE_REPLICATION_SLOT \"%s\" LOGICAL \"%s\"",
- slot_name, plugin);
+ if (is_temporary)
+ appendPQExpBuffer(query, "CREATE_REPLICATION_SLOT \"%s\" TEMPORARY LOGICAL \"%s\"",
+ slot_name, plugin);
+ else
+ appendPQExpBuffer(query, "CREATE_REPLICATION_SLOT \"%s\" LOGICAL \"%s\"",
+ slot_name, plugin);
if (PQserverVersion(conn) >= 100000)
/* pg_recvlogical doesn't use an exported snapshot, so suppress */
appendPQExpBuffer(query, " NOEXPORT_SNAPSHOT");
diff --git a/src/bin/pg_basebackup/streamutil.h b/src/bin/pg_basebackup/streamutil.h
index 460dcb5..1fe4168 100644
--- a/src/bin/pg_basebackup/streamutil.h
+++ b/src/bin/pg_basebackup/streamutil.h
@@ -32,7 +32,7 @@ extern PGconn *GetConnection(void);
/* Replication commands */
extern bool CreateReplicationSlot(PGconn *conn, const char *slot_name,
- const char *plugin, bool is_physical,
+ const char *plugin, bool is_physical, bool is_temporary,
bool slot_exists_ok);
extern bool DropReplicationSlot(PGconn *conn, const char *slot_name);
extern bool RunIdentifySystem(PGconn *conn, char **sysid,
--
2.1.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
