Fix declaration of "create_checkpoint()"

Creates a Postgres checkpoint after `pg_ctl promote` runs on the former standby and before `pg_rewind` runs on the former master. This fixes the race condition that was reported in https://github.com/2ndQuadrant/repmgr/issues/372

CONTRIBUTING.md

@@ -21,9 +21,11 @@ copy of the relevant Copyright Assignment Form.
 Code style
 ----------
 
-Code in repmgr is formatted to a consistent style using the following command:
+Code in repmgr should be formatted to the same standards as the main PostgreSQL
+project. For more details see:
 
-    astyle --style=ansi --indent=tab --suffix=none *.c *.h
+    https://www.postgresql.org/docs/current/static/source-format.html
 
 Contributors should reformat their code similarly before submitting code to
 the project, in order to minimize merge conflicts with other work.
+

HISTORY

@@ -1,3 +1,6 @@
+3.3.3   2017-06
+        repmgr: fix `standby register --force` when updating existing node record (Ian)
+
 3.3.2   2017-06-01
         Add support for PostgreSQL 10 (Ian)
         repmgr: ensure --replication-user option is honoured when passing database

README.md

@@ -189,6 +189,14 @@ system.
   Instructions can be found in the APT section of the PostgreSQL Wiki
   ( https://wiki.postgresql.org/wiki/Apt ).
 
+  *NOTE*: repmgr 3.3 packages are now only available via a 2ndQuadrant-hosted
+  repository which can be installed like this:
+
+    apt-key adv --fetch-keys http://packages.2ndquadrant.com/repmgr3/apt/0xD3FA41F6.asc
+
+    echo deb http://packages.2ndquadrant.com/repmgr3/apt/ $(lsb_release -cs)-2ndquadrant main > /etc/apt/sources.list.d/repmgr3.list
+
+
 See `PACKAGES.md` for details on building .deb and .rpm packages from the
 `repmgr` source code.
 
@@ -202,7 +210,7 @@ See `PACKAGES.md` for details on building .deb and .rpm packages from the
 Release tarballs are also available:
 
     https://github.com/2ndQuadrant/repmgr/releases
-    http://repmgr.org/downloads.php
+    http://repmgr.org/
 
 `repmgr` is compiled in the same way as a PostgreSQL extension using the PGXS
 infrastructure, e.g.:
@@ -314,7 +322,13 @@ The following replication settings may need to be adjusted:
     max_wal_senders = 10
 
     # Ensure WAL files contain enough information to enable read-only queries
-    # on the standby
+    # on the standby.
+    #
+    #  PostgreSQL 9.5 and earlier: one of 'hot_standby' or 'logical'
+    #  PostgreSQL 9.6 and later: one of 'replica' or 'logical'
+    #    ('hot_standby' will still be accepted as an alias for 'replica')
+    #
+    # See: https://www.postgresql.org/docs/current/static/runtime-config-wal.html#GUC-WAL-LEVEL
 
     wal_level = 'hot_standby'
 
@@ -400,7 +414,8 @@ least the following parameters:
 - `conninfo`: a valid connection string for the `repmgr` database on the
      *current* server. (On the standby, the database will not yet exist, but
      `repmgr` needs to know the connection details to complete the setup
-     process).
+     process). *NOTE* this must be a keyword/value string, not a connection
+     URI; this limitation will be removed in a future `repmgr` version.
 
 `repmgr.conf` should not be stored inside the PostgreSQL data directory,
 as it could be overwritten when setting up or reinitialising the PostgreSQL
@@ -425,7 +440,7 @@ to include this schema name, e.g.
 ### Initialise the master server
 
 To enable `repmgr` to support a replication cluster, the master node must
-be registered with `repmgr`, which creates the `repmgr` database and adds
+be registered with `repmgr`, which creates the `repmgr` metadatabase and adds
 a metadata record for the server:
 
     $ repmgr -f repmgr.conf master register
@@ -631,7 +646,7 @@ In order to enable Barman support for `repmgr standby clone`, you must
 ensure that:
 
 - the name of the server configured in Barman is equal to the
-  `cluster_name` setting in `repmgr.conf`;
+  `cluster` setting in `repmgr.conf`;
 - the `barman_server` setting in `repmgr.conf` is set to the SSH
   hostname of the Barman server;
 - the `restore_command` setting in `repmgr.conf` is configured to
@@ -996,6 +1011,13 @@ both passwordless SSH access and the path of `repmgr.conf` on that server.
 > careful preparation and with adequate attention. In particular you should
 > be confident that your network environment is stable and reliable.
 >
+> Additionally you should be sure that the current master can be shut down
+> quickly and cleanly. In particular, access from applications should be
+> minimalized or preferably blocked completely. Also check that there is
+> no backlog of files waiting to be archived, as PostgreSQL will not shut
+> down until archiving completes, and that any standbys attached to the
+> current primary don't have a significant amount of replication lag.
+>
 > We recommend running `repmgr standby switchover` at the most verbose
 > logging level (`--log-level DEBUG --verbose`) and capturing all output
 > to assist troubleshooting any problems.
@@ -1062,7 +1084,7 @@ should have been updated to reflect this:
 
 ### Caveats
 
-- The functionality provided `repmgr standby switchover` is primarily aimed
+- The functionality provided by `repmgr standby switchover` is primarily aimed
   at a two-server master/standby replication cluster and currently does
   not support additional standbys.
 - `repmgr standby switchover` is designed to use the `pg_rewind` utility,
@@ -1076,11 +1098,6 @@ should have been updated to reflect this:
   the `repmgrd` may try and promote a standby by itself.
 - Any other standbys attached to the old master will need to be manually
   instructed to point to the new master (e.g. with `repmgr standby follow`).
-- You must ensure that following a server start using `pg_ctl`, log output
-  is not send to STDERR (the default behaviour). If logging is not configured,
-  we recommend setting `logging_collector=on` in `postgresql.conf` and
-  providing an explicit `-l/--log` setting in `repmgr.conf`'s `pg_ctl_options`
-  parameter.
 
 We hope to remove some of these restrictions in future versions of `repmgr`.
 
@@ -1610,7 +1627,7 @@ which contains connection details for the local database.
 
     Creates a witness server as a separate PostgreSQL instance. This instance
     can be on a separate server or a server running an existing node. The
-    witness server contain a copy of the repmgr metadata tables but will not
+    witness server contains a copy of the repmgr metadata tables but will not
     be set up as a standby; instead it will update its metadata copy each
     time a failover occurs.
 

config.c

@@ -30,7 +30,7 @@ static void tablespace_list_append(t_configuration_options *options, const char
 static void exit_with_errors(ItemList *config_errors);
 
 const static char *_progname = NULL;
-static char config_file_path[MAXPGPATH];
+static char config_file_path[MAXPGPATH] = "";
 static bool config_file_provided = false;
 bool config_file_found = false;
 

dbutils.c

@@ -428,6 +428,8 @@ int
 get_server_version(PGconn *conn, char *server_version)
 {
 	PGresult   *res;
+	int         server_version_num;
+
 	res = PQexec(conn,
 				 "SELECT current_setting('server_version_num'), "
 				 "       current_setting('server_version')");
@@ -441,9 +443,12 @@ get_server_version(PGconn *conn, char *server_version)
 	}
 
 	if (server_version != NULL)
-		strcpy(server_version, PQgetvalue(res, 0, 0));
+		strcpy(server_version, PQgetvalue(res, 0, 1));
 
-	return atoi(PQgetvalue(res, 0, 0));
+	server_version_num = atoi(PQgetvalue(res, 0, 0));
+
+	PQclear(res);
+	return server_version_num;
 }
 
 
@@ -1716,6 +1721,27 @@ create_event_record(PGconn *conn, t_configuration_options *options, int node_id,
 	return success;
 }
 
+void
+create_checkpoint(PGconn *conn)
+{
+	char		sqlquery[MAXLEN];
+	PGresult   *res;
+
+	sqlquery_snprintf(sqlquery, "CHECKPOINT");
+	log_verbose(LOG_DEBUG, "checkpoint:\n%s\n", sqlquery);
+
+	res = PQexec(conn, sqlquery);
+	if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
+	{
+		log_err(_("Unable to create CHECKPOINT:\n%s\n"),
+				PQerrorMessage(conn));
+		PQfinish(conn);
+		exit(ERR_DB_QUERY);
+	}
+
+	log_notice(_("CHECKPOINT created\n"));
+}
+
 
 bool
 update_node_record(PGconn *conn, char *action, int node, char *type, int upstream_node, char *cluster_name, char *node_name, char *conninfo, int priority, char *slot_name, bool active)

dbutils.h

@@ -135,9 +135,9 @@ bool        update_node_record(PGconn *conn, char *action, int node, char *type,
 bool        update_node_record_status(PGconn *conn, char *cluster_name, int this_node_id, char *type, int upstream_node_id, bool active);
 bool        update_node_record_set_upstream(PGconn *conn, char *cluster_name, int this_node_id, int new_upstream_node_id);
 bool        create_event_record(PGconn *conn, t_configuration_options *options, int node_id, char *event, bool successful, char *details);
+void        create_checkpoint(PGconn *conn);
 
 int		    get_node_replication_state(PGconn *conn, char *node_name, char *output);
 t_server_type parse_node_type(const char *type);
 int			get_data_checksum_version(const char *data_directory);
 #endif
-

docs/repmgrd-node-fencing.md

@@ -49,6 +49,14 @@ the `%include` directive (available from PgBouncer 1.6) to include a separate
 configuration file, `/etc/pgbouncer.database.ini`, which will be modified by
 `repmgr`.
 
+* * *
+
+> *NOTE*: in this self-contained demonstration, `pgbouncer` is running on the
+> database servers, however in a production environment it will make more
+> sense to run `pgbouncer` on either separate nodes or the application server.
+
+* * *
+
 `/etc/pgbouncer.ini` should look something like this:
 
     [pgbouncer]
@@ -125,7 +133,7 @@ The actual script is as follows; adjust the configurable items as appropriate:
 
         psql -d $REPMGR_DB -U $REPMGR_USER -t -A \
           -c "SELECT '${PGBOUNCER_DATABASE}-ro= ' || conninfo || ' application_name=pgbouncer_${HOST}' \
-              FROM $REPMGR_SCHEMA.repl_nodes \
+              FROM ${REPMGR_SCHEMA}.repl_nodes \
               WHERE node_name='${HOST}'" >> $PGBOUNCER_DATABASE_INI_NEW
 
         rsync $PGBOUNCER_DATABASE_INI_NEW $HOST:$PGBOUNCER_DATABASE_INI

repmgr.c

@@ -283,7 +283,11 @@ main(int argc, char **argv)
 
 	/*
 	 * Pre-set any defaults , which can be overwritten if matching
-	 * command line parameters are provided
+	 * command line parameters are provided.
+	 *
+	 * Note: PQconndefaults() does not provide a default value for
+	 * "dbname", but if none is provided will default to "username"
+	 * when the connection is made.
 	 */
 
 	for (c = 0; c < source_conninfo.size && source_conninfo.keywords[c]; c++)
@@ -316,7 +320,6 @@ main(int argc, char **argv)
 	}
 
 	/* set default user for -R/--remote-user */
-
 	{
 		struct passwd *pw = NULL;
 
@@ -330,15 +333,6 @@ main(int argc, char **argv)
 		strncpy(runtime_options.username, pw->pw_name, MAXLEN);
 	}
 
-	/*
-	 * Though libpq will default to the username as dbname, PQconndefaults()
-	 * doesn't return this
-	 */
-	if (runtime_options.dbname[0] == '\0')
-	{
-		strncpy(runtime_options.dbname, runtime_options.username, MAXLEN);
-	}
-
 	while ((c = getopt_long(argc, argv, "?Vd:h:p:U:S:D:f:R:w:k:FWIvb:rcL:tm:C:l:P", long_options,
 							&optindex)) != -1)
 	{
@@ -1307,7 +1301,7 @@ build_cluster_matrix(t_node_matrix_rec ***matrix_rec_dest, int *name_length)
 		 */
 		appendPQExpBuffer(&command,
 						  "\"%s -d '%s' --cluster '%s' ",
-						  make_pg_path("repmgr"),
+						  make_pg_path((char *)progname()),
 						  PQgetvalue(res, i, 0),
 						  PQgetvalue(res, i, 5));
 
@@ -1572,7 +1566,7 @@ build_cluster_crosscheck(t_node_status_cube ***dest_cube, int *name_length)
 
 		appendPQExpBuffer(&command,
 						  "%s -d '%s' --cluster '%s' --node=%i ",
-						  make_pg_path("repmgr"),
+						  make_pg_path((char *)progname()),
 						  PQgetvalue(res, i, 0),
 						  options.cluster_name,
 						  remote_node_id);
@@ -2119,7 +2113,7 @@ do_standby_register(void)
 
 	if (node_result)
 	{
-		if (node_record.active == true)
+		if (node_record.active == true && node_record.node_id != options.node)
 		{
 			log_err(_("Node %i exists already with node_name \"%s\"\n"),
 					  node_record.node_id,
@@ -2638,6 +2632,7 @@ get_tablespace_data_barman
 	return SUCCESS;
 }
 
+
 void
 get_barman_property(char *dst, char *name, char *local_repmgr_directory)
 {
@@ -4092,7 +4087,16 @@ stop_backup:
 
 	/* Finally, write the recovery.conf file */
 
-	create_recovery_file(local_data_directory, &recovery_conninfo);
+	if (create_recovery_file( local_data_directory, &recovery_conninfo) == false)
+	{
+		/* create_recovery_file() will log an error */
+		log_notice(_("unable to create recovery.conf; see preceding error messages\n"));
+		log_hint(_("data directory (\"%s\") may need to be cleaned up manually\n"),
+				 local_data_directory);
+
+		PQfinish(source_conn);
+		exit(ERR_BAD_CONFIG);
+	}
 
 	if (mode == barman)
 	{
@@ -4511,6 +4515,12 @@ do_standby_promote(void)
 
 	log_notice(_("STANDBY PROMOTE successful\n"));
 
+	/*
+	 * Force a checkpoint so that pg_rewind on former master can tell that the
+	 * servers have diverged.
+	 */
+	create_checkpoint(conn);
+
 	/* Log the event */
 	create_event_record(conn,
 						&options,
@@ -4978,7 +4988,9 @@ do_standby_switchover(void)
 
 	if (r != 0)
 	{
-		log_err(_("unable to connect via SSH to host %s, user %s\n"), remote_host, runtime_options.remote_user);
+		log_err(_("unable to connect via SSH to host \"%s\", user \"%s\"\n"),
+				remote_host, runtime_options.remote_user);
+		exit(ERR_BAD_CONFIG);
 	}
 
 	if (get_pg_setting(remote_conn, "data_directory", remote_data_directory) == false)
@@ -5281,7 +5293,7 @@ do_standby_switchover(void)
 		initPQExpBuffer(&remote_command_str);
 		appendPQExpBuffer(&remote_command_str,
 						  "%s standby archive-config -f ",
-						  make_pg_path("repmgr"));
+						  make_pg_path((char *)progname()));
 		appendShellString(&remote_command_str, runtime_options.remote_config_file);
 		appendPQExpBuffer(&remote_command_str,
 						  " --config-archive-dir=");
@@ -5459,7 +5471,7 @@ do_standby_switchover(void)
 		/* --force */
 		appendPQExpBuffer(&remote_command_str,
 						  "%s standby restore-config -D ",
-						  make_pg_path("repmgr"));
+						  make_pg_path((char *)progname()));
 		appendShellString(&remote_command_str, remote_data_directory);
 
 		/*
@@ -5534,14 +5546,20 @@ do_standby_switchover(void)
 
 		appendPQExpBuffer(&remote_command_str,
 						  "%s -D ",
-						  make_pg_path("repmgr"));
+						  make_pg_path((char *)progname()));
 		appendShellString(&remote_command_str, remote_data_directory);
 		appendPQExpBuffer(&remote_command_str, " -f ");
 		appendShellString(&remote_command_str, runtime_options.remote_config_file);
 		appendPQExpBuffer(&remote_command_str,
-						 " %s --rsync-only --force --ignore-external-config-files standby clone",
+						 " %s --rsync-only --force standby clone",
 						 repmgr_db_cli_params);
 
+		if (runtime_options.copy_external_config_files == true)
+		{
+			appendPQExpBuffer(&remote_command_str,
+							  " --copy-external-config-files");
+		}
+
 		log_debug("Executing:\n%s\n", remote_command_str.data);
 
 		initPQExpBuffer(&command_output);
@@ -5565,7 +5583,7 @@ do_standby_switchover(void)
 	initPQExpBuffer(&remote_command_str);
 	appendPQExpBuffer(&remote_command_str,
 					  "%s -D ",
-					  make_pg_path("repmgr"));
+					  make_pg_path((char *)progname()));
 	appendShellString(&remote_command_str, remote_data_directory);
 	appendPQExpBuffer(&remote_command_str, " -f ");
 	appendShellString(&remote_command_str, runtime_options.remote_config_file);
@@ -5864,7 +5882,8 @@ do_standby_restore_config(void)
 		exit(ERR_BAD_CONFIG);
 	}
 
-	while ((arcdir_ent = readdir(arcdir)) != NULL) {
+	while ((arcdir_ent = readdir(arcdir)) != NULL)
+	{
 		struct stat statbuf;
 		char arcdir_ent_path[MAXPGPATH];
 		PQExpBufferData src_file;
@@ -5952,9 +5971,9 @@ do_witness_create(void)
 	char		master_hba_file[MAXLEN];
 	bool        success;
 
-	char		witness_port[MAXLEN];
+	char		witness_port[MAXLEN] = "";
-	char		repmgr_user[MAXLEN];
+	char		repmgr_user[MAXLEN] = "";
-	char		repmgr_db[MAXLEN];
+	char		repmgr_db[MAXLEN] = "";
 
 	/*
 	 * Extract the repmgr user and database names from the conninfo string
@@ -7371,7 +7390,8 @@ check_parameters_for_action(const int action)
 			item_list_append(&cli_warnings, _("-c/--fast-checkpoint can only be used when executing STANDBY CLONE"));
 		}
 
-		if (runtime_options.copy_external_config_files)
+		/* can be used for "standby switchover" too */
+		if (action != STANDBY_SWITCHOVER && runtime_options.copy_external_config_files)
 		{
 			item_list_append(&cli_warnings, _("--copy-external-config-files can only be used when executing STANDBY CLONE"));
 		}
@@ -8418,17 +8438,12 @@ remote_command(const char *host, const char *user, const char *command, PQExpBuf
 	}
 	else
 	{
-		/*
+		while (fgets(output, MAXLEN, fp) != NULL)
-		 * When executed remotely, repmgr commands which execute pg_ctl (particularly
-		 * `repmgr standby follow`) will see the pg_ctl command appear to fail with a
-		 * non-zero return code when the output from the executed pg_ctl command
-		 * has nowhere to go, even though the command actually succeeds. We'll consume an
-		 * arbitrary amount of output and throw it away to work around this.
-		 */
-		int i = 0;
-		while (fgets(output, MAXLEN, fp) != NULL && i < 10)
 		{
-			i++;
+			if (!feof(fp))
+			{
+				break;
+			}
 		}
 	}
 

repmgr.conf.sample

@@ -26,11 +26,14 @@
                  # the server's hostname or another identifier unambiguously
                  # associated with the server to avoid confusion
 
-# Database connection information as a conninfo string
+# Database connection information as a conninfo string (this must be a
-# This must be accessible to all servers in the cluster; for details see:
+# keyword/value string, not a connection URI).
 #
 #   https://www.postgresql.org/docs/current/static/libpq-connect.html#LIBPQ-CONNSTRING
 #
+# All servers in the cluster must be able to access the database
+# using this connection string.
+#
 #conninfo='host=192.168.204.104 dbname=repmgr user=repmgr'
 #
 # If repmgrd is in use, consider explicitly setting `connect_timeout` in the