Hi,
Am Dienstag, den 21.03.2017, 11:03 +0900 schrieb Michael Paquier:
> On Tue, Mar 21, 2017 at 1:32 AM, Michael Banck
> <michael.ba...@credativ.de> 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 <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..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, 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 e4bd7b0a40ced05efbd2740b5b4f8ed6b08eae6c Mon Sep 17 00:00:00 2001
From: Michael Banck <michael.ba...@credativ.de>
Date: Tue, 21 Mar 2017 12:50:22 +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, 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 +-
6 files changed, 33 insertions(+), 37 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;
static bool success = false;
static bool made_new_pgdata = false;
@@ -487,8 +488,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,18 +581,26 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
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().
+ * Create replication slot if one is needed.
*/
- if (!temp_replication_slot && replication_slot)
+ if (!no_slot)
{
+ if (!replication_slot)
+ replication_slot = psprintf("pg_basebackup_%d", (int) getpid());
+
if (verbose)
- fprintf(stderr, _("%s: creating replication slot \"%s\"\n"),
- progname, replication_slot);
+ {
+ if (temp_replication_slot)
+ fprintf(stderr, _("%s: creating temporary replication slot \"%s\"\n"),
+ progname, replication_slot);
+ else
+ 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,
+ temp_replication_slot, true))
+ disconnect_and_exit(1);
}
if (format == 'p')
@@ -2110,7 +2117,6 @@ main(int argc, char **argv)
int c;
int option_index;
- bool no_slot = false;
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
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
