pg_autoctl: retry perform_failover on transient monitor deadlocks (#1004)

src/bin/pg_autoctl/cli_perform.c

@@ -300,31 +300,60 @@ cli_perform_failover(int argc, char **argv)
 		exit(EXIT_CODE_MONITOR);
 	}
 
-	bool performOk;
+	/*
+	 * perform_failover() on the monitor can hit a genuine but transient
+	 * Postgres deadlock racing a concurrent node_active()/health-check
+	 * write (see #1004): retry it the same way enable/disable maintenance
+	 * already do for their own monitor calls, rather than failing the
+	 * whole command on the first transient error.
+	 */
+	ConnectionRetryPolicy retryPolicy = { 0 };
+
+	(void) pgsql_set_monitor_interactive_retry_policy(&retryPolicy);
 
-	if (keeperOptions.allowDataLoss)
+	while (!pgsql_retry_policy_expired(&retryPolicy))
 	{
-		performOk = monitor_perform_failover_allow_data_loss(
-			&monitor, config.formation, config.groupId);
+		bool performOk;
+		bool mayRetry = false;
 
-		if (!performOk)
+		if (keeperOptions.allowDataLoss)
 		{
-			log_fatal("Failed to perform failover with --allow-data-loss, "
-					  "see above for details");
-			exit(EXIT_CODE_MONITOR);
+			performOk = monitor_perform_failover_allow_data_loss(
+				&monitor, config.formation, config.groupId, &mayRetry);
+		}
+		else
+		{
+			performOk = monitor_perform_failover(
+				&monitor, config.formation, config.groupId, &mayRetry);
 		}
-	}
-	else
-	{
-		performOk = monitor_perform_failover(
-			&monitor, config.formation, config.groupId);
 
-		if (!performOk)
+		if (performOk)
 		{
-			log_fatal("Failed to perform failover/switchover, "
-					  "see above for details");
+			break;
+		}
+
+		if (!mayRetry)
+		{
+			log_fatal("Failed to perform failover%s, see above for details",
+					  keeperOptions.allowDataLoss ? " with --allow-data-loss" : "");
 			exit(EXIT_CODE_MONITOR);
 		}
+
+		int sleepTimeMs = pgsql_compute_connection_retry_sleep_time(&retryPolicy);
+
+		log_warn("Failed to perform failover%s, retrying in %d ms.",
+				 keeperOptions.allowDataLoss ? " with --allow-data-loss" : "",
+				 sleepTimeMs);
+
+		/* we have milliseconds, pg_usleep() wants microseconds */
+		(void) pg_usleep(sleepTimeMs * 1000);
+	}
+
+	if (pgsql_retry_policy_expired(&retryPolicy))
+	{
+		log_fatal("Failed to perform failover/switchover, "
+				  "see above for details");
+		exit(EXIT_CODE_MONITOR);
 	}
 
 	/* process state changes notification until we have a new primary */

src/bin/pg_autoctl/demoapp.c

@@ -440,7 +440,9 @@ demoapp_process_perform_switchover(DemoAppOptions *demoAppOptions)
 			continue;
 		}
 
-		if (!monitor_perform_failover(&monitor, formation, groupId))
+		bool mayRetry = false;
+
+		if (!monitor_perform_failover(&monitor, formation, groupId, &mayRetry))
 		{
 			log_fatal("Failed to perform failover/switchover, "
 					  "see above for details");

src/bin/pg_autoctl/monitor.c

@@ -1904,29 +1904,55 @@ monitor_get_groupId_from_name(Monitor *monitor, char *formation, char *name,
 /*
  * monitor_perform_failover calls the pgautofailover.monitor_perform_failover
  * function on the monitor.
+ *
+ * perform_failover() takes the same LockFormation()/LockNodeGroup() advisory
+ * locks every other node-mutating entry point does, so it can race a
+ * concurrent node_active()/health-check write and hit a genuine Postgres
+ * deadlock (see #1004) -- transient by nature, and already covered by
+ * monitor_retryable_error() for every other entry point that can hit it
+ * (monitor_start_maintenance, monitor_stop_maintenance,
+ * monitor_register_node, ...) except this one. *mayRetry follows that same
+ * convention: set it and return false so the caller can apply its own retry
+ * policy, exactly as for start_maintenance/stop_maintenance.
  */
 bool
-monitor_perform_failover(Monitor *monitor, char *formation, int group)
+monitor_perform_failover(Monitor *monitor, char *formation, int group,
+						 bool *mayRetry)
 {
 	PGSQL *pgsql = &monitor->pgsql;
 	const char *sql = "SELECT pgautofailover.perform_failover($1, $2)";
 	int paramCount = 2;
 	Oid paramTypes[2] = { TEXTOID, INT4OID };
 	const char *paramValues[2];
 	IntString groupString = intToString(group);
+	AbstractResultContext context = {
+		{ 0 }
+	};
 
 	paramValues[0] = formation;
 	paramValues[1] = groupString.strValue;
 
 	/*
-	 * pgautofailover.perform_failover() returns VOID.
+	 * pgautofailover.perform_failover() returns VOID: we only need the
+	 * context to capture the SQLSTATE on failure, there is nothing to parse
+	 * on success.
 	 */
 	if (!pgsql_execute_with_params(pgsql, sql,
 								   paramCount, paramTypes, paramValues,
-								   NULL, NULL))
+								   &context, NULL))
 	{
-		log_error("Failed to perform failover for formation %s and group %d",
-				  formation, group);
+		if (monitor_retryable_error(context.sqlstate))
+		{
+			*mayRetry = true;
+		}
+		else
+		{
+			/* when we may retry then it's up to the caller to handle errors */
+			log_error("Failed to perform failover for formation %s "
+					  "and group %d",
+					  formation, group);
+		}
+
 		return false;
 	}
 
@@ -1938,18 +1964,26 @@ monitor_perform_failover(Monitor *monitor, char *formation, int group)
  * monitor_perform_failover_allow_data_loss runs perform_failover in a
  * transaction with guard_data_loss disabled, accepting the risk of data
  * loss when quorum nodes have not reported their LSN.
+ *
+ * See monitor_perform_failover's docstring for why *mayRetry is needed here
+ * too: the underlying perform_failover() call is the same SQL function and
+ * can hit the exact same transient deadlock.
  */
 bool
 monitor_perform_failover_allow_data_loss(Monitor *monitor,
 										 char *formation,
-										 int group)
+										 int group,
+										 bool *mayRetry)
 {
 	PGSQL *pgsql = &monitor->pgsql;
 	const char *sql = "SELECT pgautofailover.perform_failover($1, $2)";
 	int paramCount = 2;
 	Oid paramTypes[2] = { TEXTOID, INT4OID };
 	const char *paramValues[2];
 	IntString groupString = intToString(group);
+	AbstractResultContext context = {
+		{ 0 }
+	};
 
 	paramValues[0] = formation;
 	paramValues[1] = groupString.strValue;
@@ -1970,12 +2004,21 @@ monitor_perform_failover_allow_data_loss(Monitor *monitor,
 
 	if (!pgsql_execute_with_params(pgsql, sql,
 								   paramCount, paramTypes, paramValues,
-								   NULL, NULL))
+								   &context, NULL))
 	{
-		log_error("Failed to perform failover with --allow-data-loss "
-				  "for formation %s and group %d",
-				  formation, group);
 		(void) pgsql_rollback(pgsql);
+
+		if (monitor_retryable_error(context.sqlstate))
+		{
+			*mayRetry = true;
+		}
+		else
+		{
+			log_error("Failed to perform failover with --allow-data-loss "
+					  "for formation %s and group %d",
+					  formation, group);
+		}
+
 		return false;
 	}
 

src/bin/pg_autoctl/monitor.h

@@ -187,10 +187,12 @@ bool monitor_get_groupId_from_name(Monitor *monitor,
 								   char *formation, char *name,
 								   int *groupId);
 
-bool monitor_perform_failover(Monitor *monitor, char *formation, int group);
+bool monitor_perform_failover(Monitor *monitor, char *formation, int group,
+							  bool *mayRetry);
 bool monitor_perform_failover_allow_data_loss(Monitor *monitor,
 											  char *formation,
-											  int group);
+											  int group,
+											  bool *mayRetry);
 bool monitor_perform_promotion(Monitor *monitor, char *formation, char *name);
 
 bool monitor_get_current_state(Monitor *monitor, char *formation, int group,