From f23a090a28b4f2b9f7a3a54b17d097b6b57de087 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Mon, 13 Jul 2026 02:40:45 +0200 Subject: [PATCH] Fix postgres readiness race in FSM transitions Three related fixes for a race where a keeper FSM transition could report a new state to the monitor while Postgres was still refusing TCP connections. 1. pg_setup_is_ready: accept PM_STATUS_STANDBY as "ready" PostgreSQL writes "standby" (not "ready") 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"; pg_setup_is_ready previously only accepted "ready", causing it to loop indefinitely for hot standby nodes. Update the while condition, the early-exit break, and the return value to treat POSTMASTER_STATUS_READY and POSTMASTER_STATUS_STANDBY identically. Also fix the same hardcoded check in the pg_setup_init short-circuit inside pg_setup_wait_until_is_ready. 2. local_postgres_wait_until_ready: always wait for PM_STATUS Previously this function short-circuited via pg_is_running() (= pg_ctl status, which exits 0 as soon as the PID file exists) and skipped pg_setup_wait_until_is_ready when the process was already present. The PID file appears before the postmaster writes its ready/standby status, so callers could see pgIsRunning=true while the postmaster was still in the "starting" phase. Drop the pg_is_running pre-check entirely. pg_setup_wait_until_is_ready handles the "already running" case correctly: its first loop terminates immediately when get_pgpid finds a live pid, then the second loop checks pm_status. Every caller wants "accepting connections", not "process has a pid", so the short-circuit was never correct. 3. fsm_prepare_for_secondary: guard with ensure_postgres_service_is_running The CATCHINGUP -> SECONDARY transition assumed Postgres was ready because the prior WAIT_STANDBY -> CATCHINGUP transition started it. But fix #2 above was the primary path for closing that window; this call is an explicit belt-and-suspenders guard: if Postgres is somehow still in startup when fsm_prepare_for_secondary runs, the keeper retries the transition rather than reporting SECONDARY to the monitor prematurely. --- src/bin/common/pgsetup.c | 26 +++++++++----- src/bin/pg_autoctl/fsm_transition.c | 16 ++++++++- src/bin/pg_autoctl/primary_standby.c | 51 ++++++++++++++-------------- 3 files changed, 58 insertions(+), 35 deletions(-) diff --git a/src/bin/common/pgsetup.c b/src/bin/common/pgsetup.c index c9e0b0ecf..a7bbd53ad 100644 --- a/src/bin/common/pgsetup.c +++ b/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++) { diff --git a/src/bin/pg_autoctl/fsm_transition.c b/src/bin/pg_autoctl/fsm_transition.c index 25698c790..c0bbf021e 100644 --- a/src/bin/pg_autoctl/fsm_transition.c +++ b/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 */ diff --git a/src/bin/pg_autoctl/primary_standby.c b/src/bin/pg_autoctl/primary_standby.c index cafcf8dc4..e9c0921ac 100644 --- a/src/bin/pg_autoctl/primary_standby.c +++ b/src/bin/pg_autoctl/primary_standby.c @@ -274,8 +274,16 @@ 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) @@ -283,33 +291,26 @@ 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;