Add a RecordStatus return type for functions which populate record structures

Unify a bunch of slightly different ways of handling the result.
2026-03-23 07:06:30 +00:00 · 2017-06-23 16:16:46 +09:00
parent 0823a83f92
commit dbaa2e0b44
5 changed files with 61 additions and 50 deletions
--- a/dbutils.c
+++ b/dbutils.c
@@ -22,7 +22,7 @@ static PGconn *_establish_db_connection(const char *conninfo,
 static PGconn  *_get_master_connection(PGconn *standby_conn, int *master_id, char *master_conninfo_out, bool quiet);

 static bool _set_config(PGconn *conn, const char *config_param, const char *sqlquery);
-static int  _get_node_record(PGconn *conn, char *sqlquery, t_node_info *node_info);
+static RecordStatus _get_node_record(PGconn *conn, char *sqlquery, t_node_info *node_info);
 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);
@@ -1131,7 +1131,7 @@ get_repmgr_extension_status(PGconn *conn)
 /* ===================== */


-static int
+static RecordStatus
 _get_node_record(PGconn *conn, char *sqlquery, t_node_info *node_info)
 {
 	int         ntuples;
@@ -1142,7 +1142,7 @@ _get_node_record(PGconn *conn, char *sqlquery, t_node_info *node_info)
 	if (PQresultStatus(res) != PGRES_TUPLES_OK)
 	{
 		PQclear(res);
-		return NODE_RECORD_QUERY_ERROR;
+		return RECORD_ERROR;
 	}

 	ntuples = PQntuples(res);
@@ -1150,7 +1150,7 @@ _get_node_record(PGconn *conn, char *sqlquery, t_node_info *node_info)
 	if (ntuples == 0)
 	{
 		PQclear(res);
-		return NODE_RECORD_NOT_FOUND;
+		return RECORD_NOT_FOUND;
 	}

 	_populate_node_record(res, node_info, 0);
@@ -1235,11 +1235,11 @@ get_node_type_string(t_server_type type)
 }


-int
+RecordStatus
 get_node_record(PGconn *conn, int node_id, t_node_info *node_info)
 {
 	PQExpBufferData	  query;
-	int		    result;
+	RecordStatus	  result;

 	initPQExpBuffer(&query);
 	appendPQExpBuffer(&query,
@@ -1253,7 +1253,7 @@ get_node_record(PGconn *conn, int node_id, t_node_info *node_info)
 	result = _get_node_record(conn, query.data, node_info);
 	termPQExpBuffer(&query);

-	if (result == NODE_RECORD_NOT_FOUND)
+	if (result == RECORD_NOT_FOUND)
 	{
 		log_verbose(LOG_DEBUG, "get_node_record(): no record found for node %i", node_id);
 	}
@@ -1262,11 +1262,11 @@ get_node_record(PGconn *conn, int node_id, t_node_info *node_info)
 }


-int
+RecordStatus
 get_node_record_by_name(PGconn *conn, const char *node_name, t_node_info *node_info)
 {
 	PQExpBufferData	  query;
-	int 		result;
+	RecordStatus	  result;

 	initPQExpBuffer(&query);

@@ -1314,11 +1314,11 @@ get_master_node_record(PGconn *conn, t_node_info *node_info)
 bool
 get_local_node_record(PGconn *conn, int node_id, t_node_info *node_info)
 {
-	bool	     record_found;
+	RecordStatus	     record_status;

-	record_found = get_node_record(conn, node_id, node_info);
+	record_status = get_node_record(conn, node_id, node_info);

-	if (record_found == false)
+	if (record_status != RECORD_FOUND)
 	{
 		log_error(_("unable to retrieve record for local node"));
 		log_detail(_("local node id is  %i"), node_id);
@@ -1328,7 +1328,7 @@ get_local_node_record(PGconn *conn, int node_id, t_node_info *node_info)
 		exit(ERR_BAD_CONFIG);
 	}

-	return record_found;
+	return true;
 }


@@ -1920,7 +1920,7 @@ bool
 create_replication_slot(PGconn *conn, char *slot_name, int server_version_num, PQExpBufferData *error_msg)
 {
 	PQExpBufferData		query;
-	int					query_res;
+	RecordStatus		record_status;
 	PGresult  		   *res;
 	t_replication_slot  slot_info;

@@ -1930,9 +1930,9 @@ create_replication_slot(PGconn *conn, char *slot_name, int server_version_num, P
 	 * if not we can reuse it as-is
 	 */

-	query_res = get_slot_record(conn, slot_name, &slot_info);
+	record_status = get_slot_record(conn, slot_name, &slot_info);

-	if (query_res)
+	if (record_status == RECORD_FOUND)
 	{
 		if (strcmp(slot_info.slot_type, "physical") != 0)
 		{
--- a/dbutils.h
+++ b/dbutils.h
@@ -15,9 +15,6 @@
 #include "config.h"
 #include "strutil.h"

-#define NODE_RECORD_NOT_FOUND   0
-#define NODE_RECORD_QUERY_ERROR -1
-
 typedef enum {
 	UNKNOWN = 0,
 	MASTER,
@@ -39,6 +36,12 @@ typedef enum {
 	RECTYPE_STANDBY
 } t_recovery_type;

+typedef enum {
+	RECORD_ERROR = -1,
+	RECORD_FOUND,
+	RECORD_NOT_FOUND
+} RecordStatus;
+
 /*
 * Struct to store node information
 */
@@ -198,8 +201,8 @@ bool		atobool(const char *value);
 t_server_type parse_node_type(const char *type);
 const char * get_node_type_string(t_server_type type);

-int			get_node_record(PGconn *conn, int node_id, t_node_info *node_info);
-int			get_node_record_by_name(PGconn *conn, const char *node_name, t_node_info *node_info);
+RecordStatus get_node_record(PGconn *conn, int node_id, t_node_info *node_info);
+RecordStatus get_node_record_by_name(PGconn *conn, const char *node_name, t_node_info *node_info);
 bool		get_local_node_record(PGconn *conn, int node_id, t_node_info *node_info);
 bool		get_master_node_record(PGconn *conn, t_node_info *node_info);
 void		get_downstream_node_records(PGconn *conn, int node_id, NodeInfoList *nodes);
@@ -218,7 +221,7 @@ bool        create_event_record_extended(PGconn *conn, t_configuration_options *
 /* replication slot functions */
 bool		create_replication_slot(PGconn *conn, char *slot_name, int server_version_num, PQExpBufferData *error_msg);
 bool		drop_replication_slot(PGconn *conn, char *slot_name);
-int			get_slot_record(PGconn *conn, char *slot_name, t_replication_slot *record);
+RecordStatus get_slot_record(PGconn *conn, char *slot_name, t_replication_slot *record);

 /* asynchronous query functions */
 bool		cancel_query(PGconn *conn, int timeout);
--- a/repmgr-action-master.c
+++ b/repmgr-action-master.c
@@ -26,7 +26,8 @@ do_master_register(void)
 	int			current_master_id = UNKNOWN_NODE_ID;
 	t_recovery_type recovery_type;
 	t_node_info node_info = T_NODE_INFO_INITIALIZER;
-	int			record_found;
+	RecordStatus record_status;
+
 	bool		record_created;

 	PQExpBufferData	  event_description;
@@ -120,9 +121,9 @@ do_master_register(void)
 	 * update it if --force set
 	 */

-	record_found = get_node_record(conn, config_file_options.node_id, &node_info);
+	record_status = get_node_record(conn, config_file_options.node_id, &node_info);

-	if (record_found)
+	if (record_status == RECORD_FOUND)
 	{
 		if (!runtime_options.force)
 		{
@@ -172,7 +173,7 @@ do_master_register(void)

 	initPQExpBuffer(&event_description);

-	if (record_found)
+	if (record_status == RECORD_FOUND)
 	{
 		record_created = update_node_record(conn,
 											"master register",
@@ -230,7 +231,7 @@ do_master_register(void)
 		exit(ERR_DB_QUERY);
 	}

-	if (record_found)
+	if (record_status == RECORD_FOUND)
 	{
 		log_notice(_("master node record (id: %i) updated"),
 				   config_file_options.node_id);
--- a/repmgr-action-standby.c
+++ b/repmgr-action-standby.c
@@ -1250,7 +1250,9 @@ do_standby_follow(void)
 	char		data_dir[MAXPGPATH];
 	t_conninfo_param_list recovery_conninfo;
 	char	   *errmsg = NULL;
-	int			query_result;
+
+	RecordStatus record_status;
+
 	char		restart_command[MAXLEN];
 	int			r;

@@ -1388,11 +1390,11 @@ do_standby_follow(void)
 	 * and to get the upstream node ID, which we'll need to know if
 	 * replication slots are in use and we want to delete the old slot.
 	 */
-	query_result = get_node_record(master_conn,
-								   config_file_options.node_id,
-								   &local_node_record);
+	record_status = get_node_record(master_conn,
+									  config_file_options.node_id,
+									  &local_node_record);

-	if (query_result != 1)
+	if (record_status != RECORD_FOUND)
 	{
 		/* this shouldn't happen, but if it does we'll plough on regardless */
 		log_warning(_("unable to retrieve record for node %i"),
@@ -1484,7 +1486,7 @@ do_standby_follow(void)
 	if (config_file_options.use_replication_slots && runtime_options.host_param_provided == false && original_upstream_node_id != UNKNOWN_NODE_ID)
 	{
 		t_node_info upstream_node_record  = T_NODE_INFO_INITIALIZER;
-		int			upstream_query_result;
+		RecordStatus upstream_record_status;

 		log_verbose(LOG_INFO, "attempting to remove replication slot from old upstream node %i",
 					original_upstream_node_id);
@@ -1492,13 +1494,13 @@ do_standby_follow(void)
 		/* XXX should we poll for server restart? */
 		local_conn = establish_db_connection(config_file_options.conninfo, true);

-		upstream_query_result = get_node_record(local_conn,
-												original_upstream_node_id,
-												&upstream_node_record);
+		upstream_record_status = get_node_record(local_conn,
+												   original_upstream_node_id,
+												   &upstream_node_record);

 		PQfinish(local_conn);

-		if (upstream_query_result != 1)
+		if (upstream_record_status != RECORD_FOUND)
 		{
 			log_warning(_("unable to retrieve node record for old upstream node %i"),
 						original_upstream_node_id);
@@ -1581,7 +1583,7 @@ check_source_server()

 	char			   cluster_size[MAXLEN];
 	t_node_info		   node_record = T_NODE_INFO_INITIALIZER;
-	int				   query_result;
+	RecordStatus	   record_status;
 	t_extension_status extension_status;

 	/* Attempt to connect to the upstream server to verify its configuration */
@@ -1753,9 +1755,9 @@ check_source_server()
 	else
 		upstream_node_id = config_file_options.upstream_node_id;

-	query_result = get_node_record(source_conn, upstream_node_id, &node_record);
+	record_status = get_node_record(source_conn, upstream_node_id, &node_record);

-	if (query_result)
+	if (record_status == RECORD_FOUND)
 	{
 		upstream_record_found = true;
 		strncpy(recovery_conninfo_str, node_record.conninfo, MAXLEN);
@@ -1766,9 +1768,9 @@ check_source_server()
 	 * check that there's no existing node record with the same name but
 	 * different ID
 	 */
-	query_result = get_node_record_by_name(source_conn, config_file_options.node_name, &node_record);
+	record_status = get_node_record_by_name(source_conn, config_file_options.node_name, &node_record);

-	if (query_result && node_record.node_id != config_file_options.node_id)
+	if (record_status == RECORD_FOUND && node_record.node_id != config_file_options.node_id)
 	{
 		log_error(_("another node (node_id: %i) already exists with node_name \"%s\""),
 				  node_record.node_id,
@@ -3017,11 +3019,11 @@ static void
 drop_replication_slot_if_exists(PGconn *conn, int node_id, char *slot_name)
 {
 	t_replication_slot  slot_info;
-	int					query_res;
+	int					record_status;

-	query_res = get_slot_record(conn,slot_name, &slot_info);
+	record_status = get_slot_record(conn,slot_name, &slot_info);

-	if (query_res)
+	if (record_status == RECORD_FOUND)
 	{
 		if (slot_info.active == false)
 		{
--- a/repmgrd.c
+++ b/repmgrd.c
@@ -61,6 +61,8 @@ main(int argc, char **argv)
 	char		cli_loglevel[MAXLEN] = "";
 	bool		cli_monitoring_history = false;

+	RecordStatus record_status;
+
 	static struct option long_options[] =
 	{
 /* general options */
@@ -232,15 +234,18 @@ main(int argc, char **argv)
 	/*
 	 * sanity checks
 	 *
-	 * Note: previous repmgr versions checked the PostgreSQL version at this point,
-	 * but we'll skip that and assume the presence of a node record means we're
-	 * dealing with a supported installation.
+	 * Note: previous repmgr versions checked the PostgreSQL version at this
+	 * point, but we'll skip that and assume the presence of a node record
+	 * means we're dealing with a supported installation.
+	 *
+	 * The absence of a node record will also indicate that either the node
+	 * or repmgr has note been properly configured.
 	 */

 	/* Retrieve record for this node from the local database */
-	(void) get_node_record(local_conn, config_file_options.node_id, &local_node_info);
+	record_status = get_node_record(local_conn, config_file_options.node_id, &local_node_info);

-	if (local_node_info.node_id == NODE_NOT_FOUND)
+	if (record_status != RECORD_FOUND)
 	{
 		log_error(_("no metadata record found for this node - terminating"));
 		log_hint(_("Check that 'repmgr (master|standby) register' was executed for this node"));