Fix 3-DC split-brain: monitor assigns wait_primary on replication stall (issue #997)

src/bin/Makefile

@@ -1,15 +1,21 @@
 # Copyright (c) Microsoft Corporation. All rights reserved.
 # Licensed under the PostgreSQL License.
 
-all: common pg_autoctl pgaftest ;
+COMMON_LIB = common/libpgaf_common.a
 
-common:
+all: pg_autoctl pgaftest ;
+
+# Build the shared archive once, serially, before the two binaries run in
+# parallel.  Both pg_autoctl and pgaftest include Makefile.common which
+# defines compile rules for common/*.c; without this serialisation a
+# parallel make -j would race to write the same .o files simultaneously.
+$(COMMON_LIB):
 	$(MAKE) -C common
 
-pg_autoctl: common
+pg_autoctl: $(COMMON_LIB)
 	$(MAKE) -C pg_autoctl pg_autoctl
 
-pgaftest: common
+pgaftest: $(COMMON_LIB)
 	$(MAKE) -C pgaftest pgaftest
 
 clean:
@@ -21,4 +27,4 @@ install: pg_autoctl pgaftest
 	$(MAKE) -C pg_autoctl install
 	$(MAKE) -C pgaftest install
 
-.PHONY: all common pg_autoctl pgaftest install clean
+.PHONY: all pg_autoctl pgaftest install clean

src/bin/common/Makefile.common

@@ -31,9 +31,7 @@ DEFAULT_CFLAGS += -I $(shell $(PG_CONFIG) --pkgincludedir)/internal
 DEFAULT_CFLAGS += $(shell $(PG_CONFIG) --cflags)
 DEFAULT_CFLAGS += -Wformat
 DEFAULT_CFLAGS += -Wall
-DEFAULT_CFLAGS += -Werror=implicit-int
-DEFAULT_CFLAGS += -Werror=implicit-function-declaration
-DEFAULT_CFLAGS += -Werror=return-type
+DEFAULT_CFLAGS += -Werror
 DEFAULT_CFLAGS += -Wno-declaration-after-statement
 DEFAULT_CFLAGS += -D_WANT_SEMUN
 DEFAULT_CFLAGS += -Wno-missing-braces

src/bin/pg_autoctl/cli_common.c

@@ -407,6 +407,14 @@ cli_common_keeper_getopts(int argc, char **argv,
 				break;
 			}
 
+			case 'G':
+			{
+				/* { "region", required_argument, NULL, 'G' } */
+				strlcpy(LocalOptionConfig.region, optarg, NAMEDATALEN);
+				log_trace("--region %s", LocalOptionConfig.region);
+				break;
+			}
+
 			case 'V':
 			{
 				/* keeper_cli_print_version prints version and exits. */

src/bin/pg_autoctl/cli_create_node.c

@@ -343,6 +343,7 @@ cli_create_postgres_getopts(int argc, char **argv)
 		{ "candidate-priority", required_argument, NULL, 'P' },
 		{ "replication-quorum", required_argument, NULL, 'r' },
 		{ "maximum-backup-rate", required_argument, NULL, 'R' },
+		{ "region", required_argument, NULL, 'G' },
 		{ "run", no_argument, NULL, 'x' },
 		{ "no-ssl", no_argument, NULL, 'N' },
 		{ "ssl-self-signed", no_argument, NULL, 's' },
@@ -356,7 +357,7 @@ cli_create_postgres_getopts(int argc, char **argv)
 
 	int optind =
 		cli_create_node_getopts(argc, argv, long_options,
-								"C:D:H:p:l:U:A:SLd:a:n:f:m:MI:W:w:RVvqhP:r:xsN",
+								"C:D:H:p:l:U:A:SLd:a:n:f:m:MI:W:w:RGVvqhP:r:xsN",
 								&options);
 
 	/* publish our option parsing in the global variable */
@@ -441,6 +442,7 @@ cli_create_coordinator_getopts(int argc, char **argv)
 		{ "citus-cluster", required_argument, NULL, 'Z' },
 		{ "candidate-priority", required_argument, NULL, 'P' },
 		{ "replication-quorum", required_argument, NULL, 'r' },
+		{ "region", required_argument, NULL, 'G' },
 		{ "run", no_argument, NULL, 'x' },
 		{ "no-ssl", no_argument, NULL, 'N' },
 		{ "ssl-self-signed", no_argument, NULL, 's' },
@@ -454,7 +456,7 @@ cli_create_coordinator_getopts(int argc, char **argv)
 
 	int optind =
 		cli_create_node_getopts(argc, argv, long_options,
-								"C:D:H:p:l:U:A:SLd:a:n:f:m:MRVvqhzZ:P:r:xsN",
+								"C:D:H:p:l:U:A:SLd:a:n:f:m:MRGVvqhzZ:P:r:xsN",
 								&options);
 
 	options.groupId = 0;
@@ -551,6 +553,7 @@ cli_create_worker_getopts(int argc, char **argv)
 		{ "citus-cluster", required_argument, NULL, 'Z' },
 		{ "candidate-priority", required_argument, NULL, 'P' },
 		{ "replication-quorum", required_argument, NULL, 'r' },
+		{ "region", required_argument, NULL, 'G' },
 		{ "run", no_argument, NULL, 'x' },
 		{ "no-ssl", no_argument, NULL, 'N' },
 		{ "ssl-self-signed", no_argument, NULL, 's' },
@@ -564,7 +567,7 @@ cli_create_worker_getopts(int argc, char **argv)
 
 	int optind =
 		cli_create_node_getopts(argc, argv, long_options,
-								"C:D:H:p:l:y:zZ:U:A:SLd:a:n:f:m:MRVvqhzP:r:xsN",
+								"C:D:H:p:l:y:zZ:U:A:SLd:a:n:f:m:MRGVvqhzP:r:xsN",
 								&options);
 
 	if (options.groupId == 0)

src/bin/pg_autoctl/cli_watch.c

@@ -43,6 +43,8 @@
 static int cli_watch_getopts(int argc, char **argv);
 static void cli_watch(int argc, char **argv);
 
+static bool watchWaitForMonitor = false;
+
 CommandLine watch_command =
 	make_command("watch",
 				 "Display a dashboard to watch monitor's events and state",
@@ -51,7 +53,8 @@ CommandLine watch_command =
 				 "  --monitor     show the monitor uri\n"
 				 "  --formation   formation to query, defaults to 'default' \n"
 				 "  --group       group to query formation, defaults to all \n"
-				 "  --json        output data in the JSON format\n",
+				 "  --json        output data in the JSON format\n"
+				 "  --wait        wait for the monitor to be ready before starting\n",
 				 cli_watch_getopts,
 				 cli_watch);
 
@@ -67,6 +70,7 @@ cli_watch_getopts(int argc, char **argv)
 		{ "monitor", required_argument, NULL, 'm' },
 		{ "formation", required_argument, NULL, 'f' },
 		{ "group", required_argument, NULL, 'g' },
+		{ "wait", no_argument, NULL, 'w' },
 		{ "version", no_argument, NULL, 'V' },
 		{ "verbose", no_argument, NULL, 'v' },
 		{ "quiet", no_argument, NULL, 'q' },
@@ -84,7 +88,7 @@ cli_watch_getopts(int argc, char **argv)
 
 	optind = 0;
 
-	while ((c = getopt_long(argc, argv, "D:f:g:n:Vvqh",
+	while ((c = getopt_long(argc, argv, "D:f:g:n:wVvqh",
 							long_options, &option_index)) != -1)
 	{
 		switch (c)
@@ -128,6 +132,13 @@ cli_watch_getopts(int argc, char **argv)
 				break;
 			}
 
+			case 'w':
+			{
+				watchWaitForMonitor = true;
+				log_trace("--wait");
+				break;
+			}
+
 			case 'V':
 			{
 				/* keeper_cli_print_version prints version and exits. */
@@ -238,6 +249,12 @@ cli_watch(int argc, char **argv)
 
 	(void) cli_monitor_init_from_option_or_config(&(context.monitor), &config);
 
+	if (watchWaitForMonitor)
+	{
+		pgsql_set_monitor_interactive_retry_policy(
+			&(context.monitor.pgsql.retryPolicy));
+	}
+
 	strlcpy(context.formation, config.formation, sizeof(context.formation));
 	context.groupId = config.groupId;
 

src/bin/pg_autoctl/defaults.h

@@ -19,7 +19,7 @@
 #define PG_AUTOCTL_VERSION GIT_VERSION
 
 /* version of the extension that we requite to talk to on the monitor */
-#define PG_AUTOCTL_EXTENSION_VERSION "2.2"
+#define PG_AUTOCTL_EXTENSION_VERSION "2.3"
 
 /* environment variable to use to make DEBUG facilities available */
 #define PG_AUTOCTL_DEBUG "PG_AUTOCTL_DEBUG"

src/bin/pg_autoctl/keeper.c

@@ -1670,6 +1670,7 @@ keeper_register_and_init(Keeper *keeper, NodeState initialState)
 								  config->pgSetup.settings.candidatePriority,
 								  config->pgSetup.settings.replicationQuorum,
 								  config->pgSetup.citusClusterName,
+								  config->region,
 								  &mayRetry,
 								  &assignedState))
 		{
@@ -1875,6 +1876,7 @@ keeper_register_again(Keeper *keeper)
 								  config->pgSetup.settings.candidatePriority,
 								  config->pgSetup.settings.replicationQuorum,
 								  DEFAULT_CITUS_CLUSTER_NAME,
+								  config->region,
 								  &mayRetry,
 								  &assignedState))
 		{

src/bin/pg_autoctl/keeper_config.c

@@ -61,6 +61,10 @@
 	make_strbuf_option("pg_autoctl", "nodekind", NULL, false, NAMEDATALEN, \
 					   config->nodeKind)
 
+#define OPTION_AUTOCTL_REGION(config) \
+	make_strbuf_option_default("pg_autoctl", "region", "region", \
+							   false, NAMEDATALEN, config->region, "")
+
 #define OPTION_POSTGRESQL_PGDATA(config) \
 	make_strbuf_option("postgresql", "pgdata", "pgdata", true, MAXPGPATH, \
 					   config->pgSetup.pgdata)
@@ -227,6 +231,7 @@
 		OPTION_AUTOCTL_HOSTNAME(config), \
 		OPTION_AUTOCTL_NODENAME(config), \
 		OPTION_AUTOCTL_NODEKIND(config), \
+		OPTION_AUTOCTL_REGION(config), \
 		OPTION_POSTGRESQL_PGDATA(config), \
 		OPTION_POSTGRESQL_PG_CTL(config), \
 		OPTION_POSTGRESQL_USERNAME(config), \

src/bin/pg_autoctl/keeper_config.h

@@ -46,6 +46,7 @@ typedef struct KeeperConfig
 	char name[_POSIX_HOST_NAME_MAX];
 	char hostname[_POSIX_HOST_NAME_MAX];
 	char nodeKind[NAMEDATALEN];
+	char region[NAMEDATALEN];
 
 	/* PostgreSQL setup */
 	PostgresSetup pgSetup;

src/bin/pg_autoctl/monitor.c

@@ -851,19 +851,20 @@ monitor_register_node(Monitor *monitor, char *formation,
 					  NodeState initialState,
 					  PgInstanceKind kind, int candidatePriority, bool quorum,
 					  char *citusClusterName,
+					  char *region,
 					  bool *mayRetry,
 					  MonitorAssignedState *assignedState)
 {
 	PGSQL *pgsql = &monitor->pgsql;
 	const char *sql =
 		"SELECT * FROM pgautofailover.register_node($1, $2, $3, $4, $5, $6, $7, "
-		"$8, $9::pgautofailover.replication_state, $10, $11, $12, $13)";
-	int paramCount = 13;
-	Oid paramTypes[13] = {
+		"$8, $9::pgautofailover.replication_state, $10, $11, $12, $13, $14)";
+	int paramCount = 14;
+	Oid paramTypes[14] = {
 		TEXTOID, TEXTOID, INT4OID, NAMEOID, TEXTOID, INT8OID,
-		INT8OID, INT4OID, TEXTOID, TEXTOID, INT4OID, BOOLOID, TEXTOID
+		INT8OID, INT4OID, TEXTOID, TEXTOID, INT4OID, BOOLOID, TEXTOID, TEXTOID
 	};
-	const char *paramValues[13];
+	const char *paramValues[14];
 	MonitorAssignedStateParseContext parseContext =
 	{ { 0 }, assignedState, false };
 	const char *nodeStateString = NodeStateToString(initialState);
@@ -889,6 +890,10 @@ monitor_register_node(Monitor *monitor, char *formation,
 		IS_EMPTY_STRING_BUFFER(citusClusterName)
 		? DEFAULT_CITUS_CLUSTER_NAME
 		: citusClusterName;
+	paramValues[13] =
+		IS_EMPTY_STRING_BUFFER(region)
+		? "default"
+		: region;
 
 	if (!pgsql_execute_with_params(pgsql, sql,
 								   paramCount, paramTypes, paramValues,
@@ -1940,7 +1945,7 @@ monitor_get_current_state(Monitor *monitor, char *formation, int group,
 				"         current_group_state, assigned_group_state, "
 				"         candidate_priority, replication_quorum, "
 				"         reported_tli, reported_lsn, health, nodecluster, "
-				"         healthlag, reportlag"
+				"         noderegion, healthlag, reportlag"
 				"    FROM pgautofailover.current_state($1) cs "
 				"    JOIN ("
 				"          select nodeid, "
@@ -1966,7 +1971,7 @@ monitor_get_current_state(Monitor *monitor, char *formation, int group,
 				"         current_group_state, assigned_group_state, "
 				"         candidate_priority, replication_quorum, "
 				"         reported_tli, reported_lsn, health, nodecluster, "
-				"         healthlag, reportlag"
+				"         noderegion, healthlag, reportlag"
 				"    FROM pgautofailover.current_state($1, $2) cs "
 				"    JOIN ("
 				"          select nodeid, "
@@ -2019,7 +2024,7 @@ parseCurrentNodeState(PGresult *result, int rowNumber,
 	int errors = 0;
 
 	/* we don't expect any of the column to be NULL */
-	for (colNumber = 0; colNumber < 16; colNumber++)
+	for (colNumber = 0; colNumber < 17; colNumber++)
 	{
 		if (PQgetisnull(result, rowNumber, 0))
 		{
@@ -2044,8 +2049,9 @@ parseCurrentNodeState(PGresult *result, int rowNumber,
 	 * 11 - OUT reported_lsn         pg_lsn,
 	 * 12 - OUT health               integer
 	 * 13 - OUT nodecluster          text
-	 * 14 -     healthlag            int (extract epoch from interval)
-	 * 15 -     reportlag            int (extract epoch from interval)
+	 * 14 - OUT noderegion           text
+	 * 15 -     healthlag            int (extract epoch from interval)
+	 * 16 -     reportlag            int (extract epoch from interval)
 	 *
 	 * We need the groupId to parse the formation kind into a nodeKind, so we
 	 * begin at column 1 and get back to column 0 later, after column 4.
@@ -2181,14 +2187,24 @@ parseCurrentNodeState(PGresult *result, int rowNumber,
 	}
 
 	value = PQgetvalue(result, rowNumber, 14);
+	length = strlcpy(nodeState->region, value, NAMEDATALEN);
+	if (length >= NAMEDATALEN)
+	{
+		log_error("Region \"%s\" returned by monitor is %d characters, "
+				  "the maximum supported by pg_autoctl is %d",
+				  value, length, NAMEDATALEN - 1);
+		++errors;
+	}
+
+	value = PQgetvalue(result, rowNumber, 15);
 
 	if (!stringToDouble(value, &(nodeState->healthLag)))
 	{
 		log_error("Invalid health lag \"%s\" returned by monitor", value);
 		++errors;
 	}
 
-	value = PQgetvalue(result, rowNumber, 15);
+	value = PQgetvalue(result, rowNumber, 16);
 
 	if (!stringToDouble(value, &(nodeState->reportLag)))
 	{
@@ -2221,10 +2237,10 @@ parseCurrentNodeStateArray(CurrentNodeStateArray *nodesArray, PGresult *result)
 		return false;
 	}
 
-	/* pgautofailover.current_state returns 11 columns */
-	if (PQnfields(result) != 16)
+	/* pgautofailover.current_state returns 17 columns (including noderegion) */
+	if (PQnfields(result) != 17)
 	{
-		log_error("Query returned %d columns, expected 16", PQnfields(result));
+		log_error("Query returned %d columns, expected 17", PQnfields(result));
 		return false;
 	}
 

src/bin/pg_autoctl/monitor.h

@@ -140,6 +140,7 @@ bool monitor_register_node(Monitor *monitor,
 						   int candidatePriority,
 						   bool quorum,
 						   char *citusClusterName,
+						   char *region,
 						   bool *mayRetry,
 						   MonitorAssignedState *assignedState);
 bool monitor_node_active(Monitor *monitor,

src/bin/pg_autoctl/nodespec.c

@@ -114,6 +114,9 @@ nodespec_read(const char *path, NodeSpec *spec)
 		make_strbuf_option_default("settings", "replication_quorum", NULL, false,
 								   sizeof(replicationQuorumStr),
 								   replicationQuorumStr, "true"),
+		make_strbuf_option_default("settings", "region", NULL, false,
+								   sizeof(spec->region), spec->region,
+								   ""),
 
 		/* [options] — immutable, used only at create time */
 		make_strbuf_option_default("options", "ssl", NULL, false,
@@ -674,6 +677,13 @@ nodespec_create_argv(const NodeSpec *spec,
 			PUSH("false");
 		}
 
+		/* region label (omit when empty — monitor defaults to "default") */
+		if (!IS_EMPTY_STRING_BUFFER(spec->region))
+		{
+			PUSH("--region");
+			PUSH(spec->region);
+		}
+
 		/* Citus secondary/read-replica cluster settings */
 		if (spec->citusSecondary)
 		{

src/bin/pg_autoctl/nodespec.h

@@ -71,6 +71,7 @@ typedef struct NodeSpec
 	/* [settings]  — mutable; applied on SIGHUP / file change */
 	int candidate_priority;      /* 0-100, default 50 */
 	bool replication_quorum;     /* sync quorum participant, default true */
+	char region[NAMEDATALEN];    /* data-centre / availability zone label */
 
 	/* [options]    — immutable; used only at pg_autoctl create time */
 	char ssl[32];                /* self-signed | verify-ca | verify-full | off */

src/bin/pg_autoctl/nodestate_utils.h

@@ -25,6 +25,7 @@ typedef struct CurrentNodeState
 
 	char formation[NAMEDATALEN];
 	char citusClusterName[NAMEDATALEN];
+	char region[NAMEDATALEN];
 	int groupId;
 	PgInstanceKind pgKind;