Fix Postgres readiness race in FSM transitions

src/bin/common/pgsetup.c

@@ -907,8 +907,12 @@ pg_setup_is_running(PostgresSetup *pgSetup)
 
 
 /*
- * pg_setup_is_ready returns true when the postmaster.pid file has a "ready"
- * status in it, which we parse in pgSetup->pm_status.
+ * pg_setup_is_ready returns true when the postmaster is accepting connections.
+ * That means pm_status "ready" (primary or promoted standby) or "standby" (hot
+ * standby still in streaming recovery).  PostgreSQL writes "standby" to
+ * postmaster.pid when hot_standby=on and the instance is accepting read-only
+ * connections but has not yet been promoted; pg_ctl -w treats both statuses as
+ * "server started", and so do we.
  */
 bool
 pg_setup_is_ready(PostgresSetup *pgSetup, bool pgIsNotRunningIsOk)
@@ -935,7 +939,7 @@ pg_setup_is_ready(PostgresSetup *pgSetup, bool pgIsNotRunningIsOk)
 	/*
 	 * Sometimes `pg_ctl start` returns with success and Postgres is still
 	 * in crash recovery replaying WAL files, in the "starting" state
-	 * rather than the "ready" state.
+	 * rather than the "ready" or "standby" state.
 	 *
 	 * In that case, we wait until Postgres is ready for connections. The
 	 * whole pg_autoctl code is expecting to be able to connect to
@@ -945,7 +949,8 @@ pg_setup_is_ready(PostgresSetup *pgSetup, bool pgIsNotRunningIsOk)
 	 * ERROR Connection to database failed: FATAL: the database system is
 	 * starting up
 	 */
-	while (pgSetup->pm_status != POSTMASTER_STATUS_READY)
+	while (pgSetup->pm_status != POSTMASTER_STATUS_READY &&
+		   pgSetup->pm_status != POSTMASTER_STATUS_STANDBY)
 	{
 		int maxRetries = 5;
 
@@ -996,7 +1001,8 @@ pg_setup_is_ready(PostgresSetup *pgSetup, bool pgIsNotRunningIsOk)
 		}
 
 		/* avoid an extra wait if that's possible */
-		if (pgSetup->pm_status == POSTMASTER_STATUS_READY)
+		if (pgSetup->pm_status == POSTMASTER_STATUS_READY ||
+			pgSetup->pm_status == POSTMASTER_STATUS_STANDBY)
 		{
 			break;
 		}
@@ -1013,7 +1019,8 @@ pg_setup_is_ready(PostgresSetup *pgSetup, bool pgIsNotRunningIsOk)
 		log_trace("pg_setup_is_ready: %s", pmStatusToString(pgSetup->pm_status));
 	}
 
-	return pgSetup->pm_status == POSTMASTER_STATUS_READY;
+	return pgSetup->pm_status == POSTMASTER_STATUS_READY ||
+		   pgSetup->pm_status == POSTMASTER_STATUS_STANDBY;
 }
 
 
@@ -1091,14 +1098,15 @@ pg_setup_wait_until_is_ready(PostgresSetup *pgSetup, int timeout, int logLevel)
 		*pgSetup = newPgSetup;
 
 		/* avoid an extra pg_setup_is_ready call if we're all good already */
-		pgIsReady = pgSetup->pm_status == POSTMASTER_STATUS_READY;
+		pgIsReady = pgSetup->pm_status == POSTMASTER_STATUS_READY ||
+					pgSetup->pm_status == POSTMASTER_STATUS_STANDBY;
 	}
 
 	/*
 	 * Ok so we have a postmaster.pid file with a pid > 0 (not a standalone
 	 * backend, the service has started). Postgres might still be "starting"
-	 * rather than "ready" though, so let's continue our attempts and make sure
-	 * that Postgres is ready.
+	 * rather than "ready" or "standby" though, so let's continue our attempts
+	 * and make sure that Postgres is ready.
 	 */
 	for (; !pgIsReady; attempts++)
 	{

src/bin/pg_autoctl/fsm_transition.c

@@ -1059,7 +1059,21 @@ fsm_prepare_for_secondary(Keeper *keeper)
 {
 	LocalPostgresServer *postgres = &(keeper->postgres);
 
-	/* first. check that we're on the same timeline as the new primary */
+	/*
+	 * Verify that Postgres is accepting connections before proceeding.
+	 * Postgres was started in the WAIT_STANDBY -> CATCHINGUP transition, but
+	 * local_postgres_wait_until_ready may have returned as soon as the PID
+	 * file appeared, before the postmaster finished startup recovery and began
+	 * accepting connections.  Calling ensure_postgres_service_is_running here
+	 * re-checks readiness (via pg_setup_wait_until_is_ready) and closes that
+	 * race window before we attempt SQL against the local instance.
+	 */
+	if (!ensure_postgres_service_is_running(postgres))
+	{
+		return false;
+	}
+
+	/* check that we're on the same timeline as the new primary */
 	if (!standby_check_timeline_with_upstream(postgres))
 	{
 		/* errors have already been logged */

src/bin/pg_autoctl/primary_standby.c

@@ -274,42 +274,43 @@ local_postgres_update(LocalPostgresServer *postgres, bool postgresNotRunningIsOk
 
 
 /*
- * local_postgres_wait_until_ready waits until Postgres is running and updates
- * our failure tracking counters for the Postgres service accordingly.
+ * local_postgres_wait_until_ready waits until Postgres is accepting
+ * connections and updates our failure tracking counters accordingly.
+ *
+ * We always go through pg_setup_wait_until_is_ready rather than short-
+ * circuiting on pg_is_running().  pg_is_running() (= pg_ctl status) returns
+ * true as soon as the postmaster PID file exists, which can happen before the
+ * postmaster has written PM_STATUS_READY or PM_STATUS_STANDBY to that file.
+ * Skipping the wait when the process already existed was the source of a race
+ * where a FSM transition could report a new state to the monitor while
+ * Postgres was still refusing TCP connections.
  */
 static bool
 local_postgres_wait_until_ready(LocalPostgresServer *postgres)
 {
 	PostgresSetup *pgSetup = &(postgres->postgresSetup);
 
 	int timeout = 10;       /* wait for Postgres for 10s */
-	bool pgIsRunning = pg_is_running(pgSetup->pg_ctl, pgSetup->pgdata);
 
-	log_trace("local_postgres_wait_until_ready: Postgres %s in \"%s\"",
-			  pgIsRunning ? "is running" : "is not running", pgSetup->pgdata);
-
-	if (!pgIsRunning)
-	{
-		/* main logging is done in the Postgres controller sub-process */
-		pgIsRunning = pg_setup_wait_until_is_ready(pgSetup, timeout, LOG_DEBUG);
+	/* main logging is done in the Postgres controller sub-process */
+	bool pgIsRunning =
+		pg_setup_wait_until_is_ready(pgSetup, timeout, LOG_DEBUG);
 
-		/* update connection string for connection to postgres */
-		(void)
-		local_postgres_update_pg_failures_tracking(postgres, pgIsRunning);
+	/* update connection string for connection to postgres */
+	(void) local_postgres_update_pg_failures_tracking(postgres, pgIsRunning);
 
-		if (pgIsRunning)
-		{
-			/* update pgSetup cache with new Postgres pid and all */
-			local_postgres_init(postgres, pgSetup);
+	if (pgIsRunning)
+	{
+		/* update pgSetup cache with new Postgres pid and all */
+		local_postgres_init(postgres, pgSetup);
 
-			log_debug("local_postgres_wait_until_ready: Postgres is running "
-					  "with pid %d", pgSetup->pidFile.pid);
-		}
-		else
-		{
-			log_error("Failed to ensure that Postgres is running in \"%s\"",
-					  pgSetup->pgdata);
-		}
+		log_debug("local_postgres_wait_until_ready: Postgres is running "
+				  "with pid %d", pgSetup->pidFile.pid);
+	}
+	else
+	{
+		log_error("Failed to ensure that Postgres is running in \"%s\"",
+				  pgSetup->pgdata);
 	}
 
 	return pgIsRunning;