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 <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.
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 <michael.ba...@credativ.de> 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 </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 40ec0e1..6442a54 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=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" @@ -487,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); @@ -581,6 +582,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) + { + 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, + temp_replication_slot, true)) + disconnect_and_exit(1); + } + if (format == 'p') { /* @@ -2090,12 +2114,12 @@ 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; int option_index; - bool no_slot = false; progname = get_progname(argv[0]); set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup")); @@ -2117,7 +2141,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 +2177,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 +2365,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, but --no-slot was requested\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/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, diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index 1d3c498..f1ca874 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
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers