Make "pgdata" a mandatory configuration file setting

There are some circumstances, e.g. during switchover operations,
where repmgr may need to operate on a data directory while the
server isn't running, in which case there's no way to retrieve
that information.

configfile.c

@@ -209,8 +209,8 @@ _parse_config(t_configuration_options *options, ItemList *error_list, ItemList *
 	options->node_id = UNKNOWN_NODE_ID;
 	memset(options->node_name, 0, sizeof(options->node_name));
 	memset(options->conninfo, 0, sizeof(options->conninfo));
-	memset(options->pg_bindir, 0, sizeof(options->pg_bindir));
 	memset(options->pgdata, 0, sizeof(options->pgdata));
+	memset(options->pg_bindir, 0, sizeof(options->pg_bindir));
 	options->replication_type = REPLICATION_TYPE_PHYSICAL;
 
 	/*
@@ -342,6 +342,8 @@ _parse_config(t_configuration_options *options, ItemList *error_list, ItemList *
 			strncpy(options->node_name, value, MAXLEN);
 		else if (strcmp(name, "conninfo") == 0)
 			strncpy(options->conninfo, value, MAXLEN);
+		else if (strcmp(name, "pgdata") == 0)
+			strncpy(options->pgdata, value, MAXPGPATH);
 		else if (strcmp(name, "replication_user") == 0)
 		{
 			if (strlen(value) < NAMEDATALEN)
@@ -352,8 +354,6 @@ _parse_config(t_configuration_options *options, ItemList *error_list, ItemList *
 		}
 		else if (strcmp(name, "pg_bindir") == 0)
 			strncpy(options->pg_bindir, value, MAXPGPATH);
-		else if (strcmp(name, "pgdata") == 0)
-			strncpy(options->pgdata, value, MAXPGPATH);
 
 		else if (strcmp(name, "replication_type") == 0)
 		{
@@ -465,7 +465,7 @@ _parse_config(t_configuration_options *options, ItemList *error_list, ItemList *
 		else if (strcmp(name, "barman_config") == 0)
 			strncpy(options->barman_config, value, MAXLEN);
 
-		/* undocumented test settings */
+		/* undocumented settings for testing */
 		else if (strcmp(name, "promote_delay") == 0)
 			options->promote_delay = repmgr_atoi(value, name, error_list, 1);
 
@@ -557,6 +557,11 @@ _parse_config(t_configuration_options *options, ItemList *error_list, ItemList *
 		item_list_append(error_list, _("\"node_name\": required parameter was not found"));
 	}
 
+	if (!strlen(options->pgdata))
+	{
+		item_list_append(error_list, _("\"pgdata\": required parameter was not found"));
+	}
+
 	if (!strlen(options->conninfo))
 	{
 		item_list_append(error_list, _("\"conninfo\": required parameter was not found"));

configfile.h

@@ -53,9 +53,9 @@ typedef struct
     int			node_id;
 	char		node_name[MAXLEN];
 	char		conninfo[MAXLEN];
+	char		pgdata[MAXPGPATH];
 	char		replication_user[NAMEDATALEN];
 	char		pg_bindir[MAXPGPATH];
-	char		pgdata[MAXPGPATH];
 	int			replication_type;
 
 	/* log settings */

doc/changes-in-repmgr4.md

@@ -50,13 +50,25 @@ The `--csv` option now emits a third column indicating the recovery
 status of the node.
 
 
-Removed configuration file options
+Configuration file changes
-----------------------------------
+--------------------------
 
-- `upstream_node`: see note about `--upstream-node-id` above.
+### Required settings
+
+Following 4 parameters are mandatory:
+
+- node_id
+- node_name
+- conninfo
+- pgdata
+
+
+### Renamed settings
+
+Some settings have been renamed for clarity/consistency
+node -> node_id
+name -> node_name
 
-Logging changes
----------------
 
 - Following configuration file parameters have been renamed for consistency
   with other parameters (and conform to the pattern used by PostgreSQL itself,
@@ -64,6 +76,13 @@ Logging changes
   - `loglevel` has been renamed to `log_level`
   - `logfile` has been renamed to `log_file`
   - `logfacility` has been renamed to `log_facility`
+
+### Removed settings
+
+- `cluster` has been removed
+- `upstream_node`: see note about `--upstream-node-id` above.
+
+### Logging changes
 - default value for `log_level` is `INFO` rather than `NOTICE`.
 - new parameter `log_status_interval`
 

repmgr-action-standby.c

@@ -99,17 +99,22 @@ do_standby_clone(void)
 	mode = get_standby_clone_mode();
 
 	/*
-	 * If a data directory (-D/--pgdata) was provided, use that, otherwise
+	 * Copy the provided data directory; if a configuration file was provided,
-	 * repmgr will default to using the same directory path as on the source
+	 * use the (mandatory) value from that; if -D/--pgdata was provided, use that;
-	 * host.
+	 * otherwise repmgr will default to using the same directory path as on the
+	 * source host. The last case will only ever occur when executing "repmgr
+	 * standby clone" with no configuration file.
 	 *
 	 * Note that barman mode requires -D/--pgdata.
 	 *
-	 * If -D/--pgdata is not supplied, and we're not cloning from barman,
+	 * If no data directory is explicitly provided, and we're not cloning from barman,
 	 * the source host's data directory will be fetched later, after
-	 * we've connected to it.
+	 * we've connected to it, in check_source_server().
+	 *
 	 */
-	if (runtime_options.data_dir[0])
+
+	get_node_data_directory(local_data_directory);
+	if (local_data_directory[0] != '\0')
 	{
 		local_data_directory_provided = true;
 		log_notice(_("destination directory \"%s\" provided"),
@@ -125,19 +130,6 @@ do_standby_clone(void)
 		exit(ERR_BAD_CONFIG);
 	}
 
-	/*
-	 * Target directory (-D/--pgdata) provided - use that as new data directory
-	 * (useful when executing backup on local machine only or creating the backup
-	 * in a different local directory when backup source is a remote host)
-	 *
-	 * Note: if no directory provided, check_source_server() will later set
-	 * local_data_directory from the upstream configuration.
-	 */
-	if (local_data_directory_provided == true)
-	{
-		strncpy(local_data_directory, runtime_options.data_dir, MAXPGPATH);
-	}
-
 	/* Sanity-check barman connection and installation */
 	if (mode == barman)
 	{

repmgr-client.c

@@ -1091,6 +1091,8 @@ check_cli_parameters(const int action)
 											action_name(action));
 				}
 
+				// XXX if -D/--pgdata provided, and also config_file_options.pgdaga, warn -D/--pgdata will be ignored
+
 				if (*runtime_options.upstream_conninfo)
 				{
 					if (runtime_options.use_recovery_conninfo_password == true)
@@ -1120,6 +1122,7 @@ check_cli_parameters(const int action)
 			/*
 			 * if `repmgr standby follow` executed with host params, ensure data
 			 * directory was provided
+			 * XXX not needed
 			 */
 			if (runtime_options.host_param_provided == true)
 			{
@@ -1132,17 +1135,6 @@ check_cli_parameters(const int action)
 		}
 		break;
 
-		case NODE_RESTORE_CONFIG:
-		{
-			if (strcmp(runtime_options.data_dir, "") == 0)
-			{
-				item_list_append(&cli_errors, _("-D/--pgdata required when executing NODE RESTORE-CONFIG"));
-			}
-
-			config_file_required = false;
-		}
-		break;
-
 		case CLUSTER_SHOW:
 		case CLUSTER_MATRIX:
 		case CLUSTER_CROSSCHECK:
@@ -2791,37 +2783,26 @@ data_dir_required_for_action(t_server_action action)
 }
 
 
-// optionally pass conn?
 void
 get_node_data_directory(char *data_dir_buf)
 {
 	PGconn *conn = NULL;
-	bool success = false;
 
+	/*
+	 * the configuration file setting has priority, and will always be
+	 * set when a configuration file was provided
+	 */
 	if (config_file_options.pgdata[0] != '\0')
 	{
 		strncpy(data_dir_buf, config_file_options.pgdata, MAXPGPATH);
 		return;
 	}
 
-	if (strlen(config_file_options.conninfo))
-		conn = establish_db_connection(config_file_options.conninfo, true);
-	else
-		conn = establish_db_connection_by_params(&source_conninfo, true);
-
-	success = get_pg_setting(conn, "data_directory", data_dir_buf);
-
-	PQfinish(conn);
-
-	if (success == true)
-		return;
-
 	if (runtime_options.data_dir[0] != '\0')
 	{
 		strncpy(data_dir_buf, runtime_options.data_dir, MAXPGPATH);
 		return;
 	}
 
-
 	return;
 }

repmgr.conf.sample

@@ -10,7 +10,7 @@
 # Required configuration items
 # =============================================================================
 #
-# repmgr and repmgrd require the following items to be configured.
+# repmgr and repmgrd require the following items to be explicitly configured.
 
 
 #node_id=                               # A unique integer greater than zero
@@ -35,17 +35,12 @@
                                         # connection attempt is abandoned; for details see:
                                         #  https://www.postgresql.org/docs/current/static/libpq-connect.html#LIBPQ-CONNECT-CONNECT-TIMEOUT
 
+#pgdata                                 # The node's data directory
 
 # =============================================================================
 
 # Optional configuration items
 # =============================================================================
-#------------------------------------------------------------------------------
-# General settings
-#------------------------------------------------------------------------------
-
-#pgdata                                 # The node's data directory; usually only required
-                                        # if the repmgr user is not a superuser
 
 #------------------------------------------------------------------------------
 # Replication settings