"standby clone": honour -S/--superuser setting

Refactor superuser connection detection.

dbutils.c

@@ -25,55 +25,6 @@ static void _populate_node_record(PGresult *res, t_node_info *node_info, int row
 static bool _create_update_node_record(PGconn *conn, char *action, t_node_info *node_info);
 static bool		_create_event_record(PGconn *conn, t_configuration_options *options, int node_id, char *event, bool successful, char *details, t_event_info *event_info);
 
-/* =================== */
-/* extension functions */
-/* =================== */
-
-t_extension_status
-get_repmgr_extension_status(PGconn *conn)
-{
-	PQExpBufferData	  query;
-	PGresult		 *res;
-
-	/* TODO: check version */
-
-	initPQExpBuffer(&query);
-
-	appendPQExpBuffer(&query,
-					  "	  SELECT ae.name, e.extname "
-					  "     FROM pg_catalog.pg_available_extensions ae "
-					  "LEFT JOIN pg_catalog.pg_extension e "
-					  "       ON e.extname=ae.name "
-					  "	   WHERE ae.name='repmgr' ");
-
-	res = PQexec(conn, query.data);
-
-	termPQExpBuffer(&query);
-
-	if (PQresultStatus(res) != PGRES_TUPLES_OK)
-	{
-		log_error(_("unable to execute extension query:\n	%s"),
-				PQerrorMessage(conn));
-		PQclear(res);
-
-		return REPMGR_UNKNOWN;
-	}
-
-	/* 1. Check extension is actually available */
-
-	if (PQntuples(res) == 0)
-	{
-		return REPMGR_UNAVAILABLE;
-	}
-
-	/* 2. Check if extension installed */
-	if (PQgetisnull(res, 0, 1) == 0)
-	{
-		return REPMGR_INSTALLED;
-	}
-
-	return REPMGR_AVAILABLE;
-}
 
 /* ==================== */
 /* Connection functions */
@@ -241,6 +192,28 @@ establish_db_connection_by_params(const char *keywords[], const char *values[],
 	return conn;
 }
 
+
+bool
+is_superuser_connection(PGconn *conn, t_connection_user *userinfo)
+{
+	char			 *current_user;
+	const char		 *superuser_status;
+	bool			  is_superuser;
+
+	current_user = PQuser(conn);
+	superuser_status = PQparameterStatus(conn, "is_superuser");
+	is_superuser = (strcmp(superuser_status, "on") == 0) ? true : false;
+
+	if (userinfo != NULL)
+	{
+		strncpy(userinfo->username, current_user, MAXLEN);
+		userinfo->is_superuser = is_superuser;
+	}
+
+	return is_superuser;
+}
+
+
 /* =============================== */
 /* conninfo manipulation functions */
 /* =============================== */
@@ -1000,6 +973,58 @@ bool atobool(const char *value)
 		: false;
 }
 
+
+/* =================== */
+/* extension functions */
+/* =================== */
+
+t_extension_status
+get_repmgr_extension_status(PGconn *conn)
+{
+	PQExpBufferData	  query;
+	PGresult		 *res;
+
+	/* TODO: check version */
+
+	initPQExpBuffer(&query);
+
+	appendPQExpBuffer(&query,
+					  "	  SELECT ae.name, e.extname "
+					  "     FROM pg_catalog.pg_available_extensions ae "
+					  "LEFT JOIN pg_catalog.pg_extension e "
+					  "       ON e.extname=ae.name "
+					  "	   WHERE ae.name='repmgr' ");
+
+	res = PQexec(conn, query.data);
+
+	termPQExpBuffer(&query);
+
+	if (PQresultStatus(res) != PGRES_TUPLES_OK)
+	{
+		log_error(_("unable to execute extension query:\n	%s"),
+				PQerrorMessage(conn));
+		PQclear(res);
+
+		return REPMGR_UNKNOWN;
+	}
+
+	/* 1. Check extension is actually available */
+
+	if (PQntuples(res) == 0)
+	{
+		return REPMGR_UNAVAILABLE;
+	}
+
+	/* 2. Check if extension installed */
+	if (PQgetisnull(res, 0, 1) == 0)
+	{
+		return REPMGR_INSTALLED;
+	}
+
+	return REPMGR_AVAILABLE;
+}
+
+
 /* ===================== */
 /* Node record functions */
 /* ===================== */
@@ -1811,5 +1836,3 @@ stop_backup(PGconn *conn, char *last_wal_segment, int server_version_num)
 
 	return true;
 }
-
-

dbutils.h

@@ -114,6 +114,12 @@ typedef struct s_replication_slot
 }	t_replication_slot;
 
 
+typedef struct s_connection_user
+{
+	char username[MAXLEN];
+	bool is_superuser;
+}   t_connection_user;
+
 /* connection functions */
 PGconn *establish_db_connection(const char *conninfo,
 								const bool exit_on_error);
@@ -126,8 +132,7 @@ PGconn *establish_db_connection_by_params(const char *keywords[],
 										  const char *values[],
 										  const bool exit_on_error);
 
-/* extension functions */
+bool		is_superuser_connection(PGconn *conn, t_connection_user *userinfo);
-t_extension_status get_repmgr_extension_status(PGconn *conn);
 
 /* conninfo manipulation functions */
 bool		get_conninfo_value(const char *conninfo, const char *keyword, char *output);
@@ -161,6 +166,8 @@ int			is_standby(PGconn *conn);
 PGconn	   *get_master_connection(PGconn *standby_conn, int *master_id, char *master_conninfo_out);
 int			get_master_node_id(PGconn *conn);
 
+/* extension functions */
+t_extension_status get_repmgr_extension_status(PGconn *conn);
 
 /* result functions */
 bool		atobool(const char *value);

repmgr-action-master.c

@@ -189,8 +189,16 @@ do_master_register(void)
 	commit_transaction(conn);
 	PQfinish(conn);
 
-	log_notice(_("master node record (id: %i) %s"),
+	if (record_found)
-			   config_file_options.node_id,
+	{
-			   record_found ? "updated" : "registered");
+		log_notice(_("master node record (id: %i) updated"),
+				   config_file_options.node_id);
+	}
+	else
+	{
+		log_notice(_("master node record (id: %i) registered"),
+				   config_file_options.node_id);
+	}
+
 	return;
 }

repmgr-action-standby.c

@@ -111,9 +111,6 @@ do_standby_clone(void)
 	 * to the node we're cloning from) to write to recovery.conf
 	 */
 
-	/*
-	 * detecting the cloning mode
-	 */
 	mode = get_standby_clone_mode();
 
 	/*
@@ -131,9 +128,9 @@ do_standby_clone(void)
 	}
 
 	/*
-	 * If dest_dir (-D/--pgdata) was provided, this will become the new data
+	 * If a data directory (-D/--pgdata) was provided, use that, otherwise
-	 * directory (otherwise repmgr will default to using the same directory
+	 * repmgr will default to using the same directory path as on the source
-	 * path as on the source host).
+	 * host.
 	 *
 	 * Note that barman mode requires -D/--pgdata.
 	 *
@@ -144,7 +141,7 @@ do_standby_clone(void)
 	if (runtime_options.data_dir[0])
 	{
 		local_data_directory_provided = true;
-		log_notice(_("destination directory '%s' provided"),
+		log_notice(_("destination directory \"%s\" provided"),
 				   runtime_options.data_dir);
 	}
 	else if (mode == barman)
@@ -449,6 +446,9 @@ check_barman_config(void)
 static void
 check_source_server()
 {
+	PGconn			 *superuser_conn = NULL;
+	PGconn			 *privileged_conn = NULL;
+
 	char		cluster_size[MAXLEN];
 	t_node_info node_record = T_NODE_INFO_INITIALIZER;
 	int			query_result;
@@ -548,13 +548,20 @@ check_source_server()
 	}
 
 	/* Fetch the source's data directory */
-	if (get_pg_setting(source_conn, "data_directory", upstream_data_directory) == false)
+	get_superuser_connection(&source_conn, &superuser_conn, &privileged_conn);
+
+	if (get_pg_setting(privileged_conn, "data_directory", upstream_data_directory) == false)
 	{
 		log_error(_("unable to retrieve source node's data directory"));
 		log_hint(_("STANDBY CLONE must be run as a database superuser"));
 		PQfinish(source_conn);
+		if(superuser_conn != NULL)
+			PQfinish(superuser_conn);
+
 		exit(ERR_BAD_CONFIG);
 	}
+	if(superuser_conn != NULL)
+		PQfinish(superuser_conn);
 
 	/*
 	 * If no target data directory was explicitly provided, we'll default to
@@ -588,8 +595,6 @@ check_source_server()
 	 * Copy the source connection so that we have some default values,
 	 * particularly stuff like passwords extracted from PGPASSFILE;
 	 * these will be overridden from the upstream conninfo, if provided.
-	 *
-	 * XXX only allow passwords if --use-conninfo-password
 	 */
 	conn_to_param_list(source_conn, &recovery_conninfo);
 
@@ -723,10 +728,11 @@ check_source_server_via_barman()
 static void
 initialise_direct_clone(void)
 {
-	PGresult *res;
+	PGconn			 *superuser_conn = NULL;
-	int i;
+	PGconn			 *privileged_conn = NULL;
-
+	PGresult		 *res;
 	PQExpBufferData	  query;
+	int		 		  i;
 
 	/*
 	 * Check the destination data directory can be used
@@ -735,7 +741,7 @@ initialise_direct_clone(void)
 
 	if (!create_pg_dir(local_data_directory, runtime_options.force))
 	{
-		log_error(_("unable to use directory %s"),
+		log_error(_("unable to use directory \"%s\""),
 				  local_data_directory);
 		log_hint(_("use -F/--force to force this directory to be overwritten"));
 		exit(ERR_BAD_CONFIG);
@@ -772,7 +778,7 @@ initialise_direct_clone(void)
 			appendPQExpBuffer(&query,
 							  "SELECT spcname "
 							  "  FROM pg_catalog.pg_tablespace "
-							  " WHERE pg_tablespace_location(oid) = '%s'",
+							  " WHERE pg_catalog.pg_tablespace_location(oid) = '%s'",
 							  cell->old_dir);
 			res = PQexec(source_conn, query.data);
 
@@ -801,16 +807,23 @@ initialise_direct_clone(void)
 
 	/*
 	 * Obtain configuration file locations
+	 *
 	 * We'll check to see whether the configuration files are in the data
 	 * directory - if not we'll have to copy them via SSH, if copying
 	 * requested.
 	 *
+	 * This will require superuser permissions, so we'll attempt to connect
+	 * as -S/--superuser (if provided), otherwise check the current connection
+	 * user has superuser rights.
+	 *
 	 * XXX: if configuration files are symlinks to targets outside the data
 	 * directory, they won't be copied by pg_basebackup, but we can't tell
 	 * this from the below query; we'll probably need to add a check for their
 	 * presence and if missing force copy by SSH
 	 */
 
+	get_superuser_connection(&source_conn, &superuser_conn, &privileged_conn);
+
 	initPQExpBuffer(&query);
 
 	appendPQExpBuffer(&query,
@@ -820,22 +833,26 @@ initialise_direct_clone(void)
 					  "     WHERE name = 'data_directory' "
 					  "  ) "
 					  "    SELECT DISTINCT(sourcefile), "
-					  "           regexp_replace(sourcefile, '^.*\\/', '') AS filename, "
+					  "           pg_catalog.regexp_replace(sourcefile, '^.*\\/', '') AS filename, "
 					  "           sourcefile ~ ('^' || dd.data_directory) AS in_data_dir "
 					  "      FROM dd, pg_catalog.pg_settings ps "
 					  "     WHERE sourcefile IS NOT NULL "
 					  "  ORDER BY 1 ");
 
 	log_debug("standby clone: %s", query.data);
-	res = PQexec(source_conn, query.data);
+	res = PQexec(privileged_conn, query.data);
 	termPQExpBuffer(&query);
 
 	if (PQresultStatus(res) != PGRES_TUPLES_OK)
 	{
 		log_error(_("unable to retrieve configuration file locations:\n  %s"),
-				  PQerrorMessage(source_conn));
+				  PQerrorMessage(privileged_conn));
 		PQclear(res);
 		PQfinish(source_conn);
+
+		if (superuser_conn != NULL)
+			PQfinish(superuser_conn);
+
 		exit(ERR_BAD_CONFIG);
 	}
 
@@ -873,15 +890,19 @@ initialise_direct_clone(void)
 					  "  ORDER BY 1 ");
 
 	log_debug("standby clone: %s", query.data);
-	res = PQexec(source_conn, query.data);
+	res = PQexec(privileged_conn, query.data);
 	termPQExpBuffer(&query);
 
 	if (PQresultStatus(res) != PGRES_TUPLES_OK)
 	{
 		log_error(_("unable to retrieve configuration file locations:\n  %s"),
-				  PQerrorMessage(source_conn));
+				  PQerrorMessage(privileged_conn));
 		PQclear(res);
 		PQfinish(source_conn);
+
+		if (superuser_conn != NULL)
+			PQfinish(superuser_conn);
+
 		exit(ERR_BAD_CONFIG);
 	}
 
@@ -909,7 +930,7 @@ initialise_direct_clone(void)
 		PQExpBufferData event_details;
 		initPQExpBuffer(&event_details);
 
-		if (create_replication_slot(source_conn, repmgr_slot_name, server_version_num, &event_details) == false)
+		if (create_replication_slot(privileged_conn, repmgr_slot_name, server_version_num, &event_details) == false)
 		{
 			log_error("%s", event_details.data);
 
@@ -921,15 +942,24 @@ initialise_direct_clone(void)
 								event_details.data);
 
 			PQfinish(source_conn);
+
+			if (superuser_conn != NULL)
+				PQfinish(superuser_conn);
+
 			exit(ERR_DB_QUERY);
 		}
 
 		termPQExpBuffer(&event_details);
 
-		log_notice(_("replication slot '%s' created on upstream node (node_id: %i)"),
+		log_notice(_("replication slot \"%s\" created on upstream node (node_id: %i)"),
 				   repmgr_slot_name,
 				   upstream_node_id);
 	}
+
+	if (superuser_conn != NULL)
+		PQfinish(superuser_conn);
+
+	return;
 }
 
 

repmgr-client-global.h

@@ -125,4 +125,5 @@ extern char * make_pg_path(char *file);
 
 extern bool create_recovery_file(const char *data_dir, t_conninfo_param_list *recovery_conninfo);
 
+extern void get_superuser_connection(PGconn **conn, PGconn **superuser_conn, PGconn **privileged_conn);
 #endif

repmgr-client.c

@@ -988,7 +988,7 @@ do_help(void)
  * user if not a superuser.
  *
  * Note:
- *   This should be the only place where superuser rights are required.
+ *   This is one of two places where superuser rights are required.
  *   We should also consider possible scenarious where a non-superuser
  *   has sufficient privileges to install the extension.
  */
@@ -1000,9 +1000,9 @@ create_repmgr_extension(PGconn *conn)
 	PGresult		 *res;
 
 	t_extension_status extension_status;
-	char			 *current_user;
+
-	const char		 *superuser_status;
+	t_connection_user  userinfo;
-	bool			  is_superuser;
+	bool			 is_superuser = false;
 	PGconn			 *superuser_conn = NULL;
 	PGconn			 *schema_create_conn = NULL;
 
@@ -1020,7 +1020,7 @@ create_repmgr_extension(PGconn *conn)
 
 		case REPMGR_INSTALLED:
 			/* TODO: check version */
-			log_info(_("extension \"repmgr\" already installed"));
+			log_info(_("\"repmgr\" extension is already installed"));
 			return true;
 
 		case REPMGR_AVAILABLE:
@@ -1029,48 +1029,11 @@ create_repmgr_extension(PGconn *conn)
 
 	}
 
+	/* 3. Attempt to get a superuser connection */
 
-	/* 3. Check if repmgr user is superuser, if not connect as superuser */
+	is_superuser = is_superuser_connection(conn, &userinfo);
-	current_user = PQuser(conn);
-	superuser_status = PQparameterStatus(conn, "is_superuser");
 
-	is_superuser = (strcmp(superuser_status, "on") == 0) ? true : false;
+	get_superuser_connection(&conn, &superuser_conn, &schema_create_conn);
-
-	if (is_superuser == false)
-	{
-		if (runtime_options.superuser[0] == '\0')
-		{
-			log_error(_("\"%s\" is not a superuser and no superuser name supplied"), current_user);
-			log_hint(_("supply a valid superuser name with -S/--superuser"));
-			return false;
-		}
-
-		superuser_conn = establish_db_connection_as_user(config_file_options.conninfo,
-														 runtime_options.superuser,
-														 false);
-
-		if (PQstatus(superuser_conn) != CONNECTION_OK)
-		{
-			log_error(_("unable to establish superuser connection as \"%s\""),
-					  runtime_options.superuser);
-			return false;
-		}
-
-		/* check provided superuser really is superuser */
-		superuser_status = PQparameterStatus(superuser_conn, "is_superuser");
-		if (strcmp(superuser_status, "off") == 0)
-		{
-			log_error(_("\"%s\" is not a superuser"), runtime_options.superuser);
-			PQfinish(superuser_conn);
-			return false;
-		}
-
-		schema_create_conn = superuser_conn;
-	}
-	else
-	{
-		schema_create_conn = conn;
-	}
 
 	/* 4. Create extension */
 	initPQExpBuffer(&query);
@@ -1089,7 +1052,7 @@ create_repmgr_extension(PGconn *conn)
 		log_hint(_("check that the provided user has sufficient privileges for CREATE EXTENSION"));
 
 		PQclear(res);
-		if (superuser_conn != 0)
+		if (superuser_conn != NULL)
 			PQfinish(superuser_conn);
 		return false;
 	}
@@ -1103,14 +1066,14 @@ create_repmgr_extension(PGconn *conn)
 
 		appendPQExpBuffer(&query,
 						  "GRANT USAGE ON SCHEMA repmgr TO %s",
-						  current_user);
+						  userinfo.username);
 		res = PQexec(schema_create_conn, query.data);
 
 		termPQExpBuffer(&query);
 		if (PQresultStatus(res) != PGRES_COMMAND_OK)
 		{
 			log_error(_("unable to grant usage on \"repmgr\" extension to %s:\n  %s"),
-					  current_user,
+					  userinfo.username,
 					  PQerrorMessage(schema_create_conn));
 			PQclear(res);
 
@@ -1123,7 +1086,7 @@ create_repmgr_extension(PGconn *conn)
 		initPQExpBuffer(&query);
 		appendPQExpBuffer(&query,
 						  "GRANT ALL ON ALL TABLES IN SCHEMA repmgr TO %s",
-						  current_user);
+						  userinfo.username);
 		res = PQexec(schema_create_conn, query.data);
 
 		termPQExpBuffer(&query);
@@ -1131,18 +1094,18 @@ create_repmgr_extension(PGconn *conn)
 		if (PQresultStatus(res) != PGRES_COMMAND_OK)
 		{
 			log_error(_("unable to grant permission on tables on \"repmgr\" extension to %s:\n  %s"),
-					  current_user,
+					  userinfo.username,
 					  PQerrorMessage(schema_create_conn));
 			PQclear(res);
 
-			if (superuser_conn != 0)
+			if (superuser_conn != NULL)
 				PQfinish(superuser_conn);
 
 			return false;
 		}
 	}
 
-	if (superuser_conn != 0)
+	if (superuser_conn != NULL)
 		PQfinish(superuser_conn);
 
 	log_notice(_("\"repmgr\" extension successfully installed"));
@@ -1287,6 +1250,57 @@ local_command(const char *command, PQExpBufferData *outputbuf)
 }
 
 
+void
+get_superuser_connection(PGconn **conn, PGconn **superuser_conn, PGconn **privileged_conn)
+{
+	t_connection_user userinfo;
+	bool			  is_superuser;
+
+	is_superuser = is_superuser_connection(*conn, &userinfo);
+
+	if (is_superuser == true)
+	{
+		*privileged_conn = *conn;
+
+		return;
+	}
+
+	// XXX largely duplicatied from create_repmgr_extension()
+	if (runtime_options.superuser[0] == '\0')
+	{
+		log_error(_("\"%s\" is not a superuser and no superuser name supplied"), userinfo.username);
+		log_hint(_("supply a valid superuser name with -S/--superuser"));
+		PQfinish(*conn);
+		exit(ERR_BAD_CONFIG);
+	}
+
+	*superuser_conn = establish_db_connection_as_user(config_file_options.conninfo,
+													 runtime_options.superuser,
+													 false);
+
+	if (PQstatus(*superuser_conn) != CONNECTION_OK)
+	{
+		log_error(_("unable to establish superuser connection as \"%s\""),
+				  runtime_options.superuser);
+
+		PQfinish(*conn);
+		exit(ERR_BAD_CONFIG);
+	}
+
+	/* check provided superuser really is superuser */
+	if (!is_superuser_connection(*superuser_conn, NULL))
+	{
+		log_error(_("\"%s\" is not a superuser"), runtime_options.superuser);
+		PQfinish(*superuser_conn);
+		PQfinish(*conn);
+		exit(ERR_BAD_CONFIG);
+	}
+
+	*privileged_conn = *superuser_conn;
+	return;
+}
+
+
 /*
  * check_upstream_config()
  *