From c67aa155813e3b50c7d5f0e93f61909bc803e2bc Mon Sep 17 00:00:00 2001 From: Ian Barwick Date: Wed, 2 Aug 2017 23:04:24 +0900 Subject: [PATCH] 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 | 13 +++++++++---- configfile.h | 2 +- doc/changes-in-repmgr4.md | 29 ++++++++++++++++++++++++----- repmgr-action-standby.c | 30 +++++++++++------------------- repmgr-client.c | 33 +++++++-------------------------- repmgr.conf.sample | 9 ++------- 6 files changed, 54 insertions(+), 62 deletions(-) diff --git a/configfile.c b/configfile.c index 8a4c4fcb..1a4f264d 100644 --- a/configfile.c +++ b/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")); diff --git a/configfile.h b/configfile.h index d7474c29..32f8484c 100644 --- a/configfile.h +++ b/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 */ diff --git a/doc/changes-in-repmgr4.md b/doc/changes-in-repmgr4.md index af681972..0fef0b67 100644 --- a/doc/changes-in-repmgr4.md +++ b/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` diff --git a/repmgr-action-standby.c b/repmgr-action-standby.c index c2cca016..7af83380 100644 --- a/repmgr-action-standby.c +++ b/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 - * repmgr will default to using the same directory path as on the source - * host. + * Copy the provided data directory; if a configuration file was provided, + * use the (mandatory) value from that; if -D/--pgdata was provided, use that; + * 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) { diff --git a/repmgr-client.c b/repmgr-client.c index 1b6a7f0d..beb7a1a2 100644 --- a/repmgr-client.c +++ b/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; } diff --git a/repmgr.conf.sample b/repmgr.conf.sample index a1c31153..7482d1d1 100644 --- a/repmgr.conf.sample +++ b/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