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 <robertmh...@gmail.com> 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 <michael.ba...@credativ.de>
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
</varlistentry>
<varlistentry>
+ <term><option>-C</option></term>
+ <term><option>--create-slot</option></term>
+ <listitem>
+ <para>
+ This option can only be used together with the <literal>--slot</literal>
+ option. It causes the specified slot name to be created if it does not
+ exist already.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-T <replaceable class="parameter">olddir</replaceable>=<replaceable class="parameter">newdir</replaceable></option></term>
<term><option>--tablespace-mapping=<replaceable class="parameter">olddir</replaceable>=<replaceable class="parameter">newdir</replaceable></option></term>
<listitem>
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=SLOTNAME replication 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, _("%s: creating replication slot \"%s\"\n"),
+ progname, replication_slot);
+
+ if (!CreateReplicationSlot(param->bgconn, replication_slot, NULL, true, true))
+ disconnect_and_exit(1);
+ }
+
if (format == 'p')
{
/*
@@ -2090,6 +2105,7 @@ main(int argc, char **argv)
{"progress", no_argument, NULL, 'P'},
{"waldir", required_argument, NULL, 1},
{"no-slot", no_argument, NULL, 2},
+ {"create-slot", no_argument, NULL, 'C'},
{NULL, 0, NULL, 0}
};
int c;
@@ -2117,7 +2133,7 @@ main(int argc, char **argv)
atexit(cleanup_directories_atexit);
- while ((c = getopt_long(argc, argv, "D:F:r:RT:X:l:nNzZ:d:c:h:p:U:s:S:wWvP",
+ while ((c = getopt_long(argc, argv, "D:F:r:RS:CT:X:l:nNzZ:d:c:h:p:U:s:wWvP",
long_options, &option_index)) != -1)
{
switch (c)
@@ -2153,6 +2169,9 @@ main(int argc, char **argv)
replication_slot = pg_strdup(optarg);
temp_replication_slot = false;
break;
+ case 'C':
+ create_slot = true;
+ break;
case 2:
no_slot = true;
break;
@@ -2338,6 +2357,29 @@ main(int argc, char **argv)
temp_replication_slot = false;
}
+ if (create_slot)
+ {
+ if (!replication_slot)
+ {
+ fprintf(stderr,
+ _("%s: creation of replication slots requires a slot name\n"),
+ progname);
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ progname);
+ exit(1);
+ }
+
+ if (no_slot)
+ {
+ fprintf(stderr,
+ _("%s: creation of replication slots requires a slot\n"),
+ progname);
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ progname);
+ exit(1);
+ }
+ }
+
if (strcmp(xlog_dir, "") != 0)
{
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..9ae7795 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 => 76;
program_help_ok('pg_basebackup');
program_version_ok('pg_basebackup');
@@ -246,15 +246,32 @@ $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(
[ 'pg_basebackup', '-D',
"$tempdir/backupxs_sl_fail", '-X',
'stream', '-S',
- 'slot1' ],
+ 'slot0' ],
'pg_basebackup fails with nonexistent replication slot');
+$node->command_fails(
+ [ 'pg_basebackup', '-D', "$tempdir/backupxs_slot", '-C' ],
+ 'pg_basebackup -C fails without slot name');
+
+$node->command_fails(
+ [ 'pg_basebackup', '-D', "$tempdir/backupxs_slot", '-C', '-S', 'slot0', '--no-slot' ],
+ 'pg_basebackup fails with -C -S --no-slot');
+
+$node->command_ok(
+ [ 'pg_basebackup', '-D', "$tempdir/backupxs_slot", '-C', '-S', 'slot0' ],
+ 'pg_basebackup -C -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')});
my $lsn = $node->safe_psql('postgres',
--
2.1.4
>From b81733fb3a6b9d3c517eb78598716e2d34e51cb3 Mon Sep 17 00:00:00 2001
From: Michael Banck <michael.ba...@credativ.de>
Date: Fri, 24 Mar 2017 19:08:02 +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 | 25 +++++++++++++++++--------
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(+), 34 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 2af2e22..5b073cb 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -93,6 +93,7 @@ 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;
@@ -489,8 +490,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);
@@ -584,15 +583,25 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
param->temp_slot = temp_replication_slot;
/*
- * Create permanent physical replication slot if requested.
+ * Create replication slot if one is needed.
*/
- if (replication_slot && create_slot)
+ if (!no_slot && (temp_replication_slot || create_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))
+ if (!CreateReplicationSlot(param->bgconn, replication_slot, NULL, true,
+ temp_replication_slot, true))
disconnect_and_exit(1);
}
@@ -2087,6 +2096,7 @@ main(int argc, char **argv)
{"max-rate", required_argument, NULL, 'r'},
{"write-recovery-conf", no_argument, NULL, 'R'},
{"slot", required_argument, NULL, 'S'},
+ {"create", no_argument, NULL, 'C'},
{"tablespace-mapping", required_argument, NULL, 'T'},
{"wal-method", required_argument, NULL, 'X'},
{"gzip", no_argument, NULL, 'z'},
@@ -2111,7 +2121,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 1f2b406..3f65852 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -321,7 +321,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;
@@ -334,12 +334,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
