From cb0885b03e2c01bce7deeb2cf5dec0c07be416ba Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Mon, 27 Jul 2026 02:41:37 +0200 Subject: [PATCH 1/6] Fix #774: monitor can assign a primary an unreachable goal state during failover Two rules in group_state_machine.c could combine to assign the primary goal state demote_timeout while it is sitting at wait_primary. There is no KeeperFSM[] edge from wait_primary to demote_timeout, so the keeper fatals forever with "does not know how to reach state ... from ...". Rule 1 ("no healthy standby in the quorum"): reassigns the primary to wait_primary whenever secondaryQuorumNodesCount == 0. That count drops to zero the instant a failover candidate leaves SECONDARY (e.g. converges to prepare_promotion) -- even though the candidate is the failover in progress, not a lost standby. Guarded with !IsFailoverInProgress(). Rule 2 ("prepare_promotion -> stop_replication"): assigns the primary demote_timeout as soon as the candidate reports prepare_promotion, unconditionally. demote_timeout has a real KeeperFSM[] edge from primary/join_primary/apply_settings/draining -- but not from wait_primary. Guarded to only fire when the primary's reported state is actually one of those four. Adds a pg_regress test (failover_candidate_leaves_secondary) isolating each rule via direct state manufacture, following the same technique stale_primary_report.sql already uses for hard-to-reach states. Verified via authoritative `make installcheck` (pg_regress in Docker, all 13 monitor tests + isolation tests pass) and by empirically reproducing both bugs pre-fix and confirming both fixes independently load-bearing (temporarily reverting each and observing the failure reproduce). --- .../failover_candidate_leaves_secondary.out | 335 ++++++++++++++++++ src/monitor/group_state_machine.c | 33 +- src/monitor/regress_schedule | 1 + .../failover_candidate_leaves_secondary.sql | 225 ++++++++++++ 4 files changed, 592 insertions(+), 2 deletions(-) create mode 100644 src/monitor/expected/failover_candidate_leaves_secondary.out create mode 100644 src/monitor/sql/failover_candidate_leaves_secondary.sql diff --git a/src/monitor/expected/failover_candidate_leaves_secondary.out b/src/monitor/expected/failover_candidate_leaves_secondary.out new file mode 100644 index 000000000..cf4080fa1 --- /dev/null +++ b/src/monitor/expected/failover_candidate_leaves_secondary.out @@ -0,0 +1,335 @@ +-- Copyright (c) Microsoft Corporation. All rights reserved. +-- Licensed under the PostgreSQL License. +-- +-- Regression test for issue #774: a monitor rule can assign a node a goal +-- state its own KeeperFSM[] (src/bin/pg_autoctl/fsm.c) has no transition +-- path to reach from that node's current state, fataling the keeper +-- forever ("does not know how to reach state ... from ..."). +-- +-- Two independent rules in group_state_machine.c interact badly: +-- +-- Rule 1 (ProceedGroupStateForPrimaryNode, "no healthy standby in the +-- quorum" block): reassigns the primary to wait_primary whenever +-- secondaryQuorumNodesCount == 0. That count drops to zero the instant a +-- failover candidate leaves SECONDARY (e.g. converges to +-- prepare_promotion) -- even though the candidate *is* the failover in +-- progress, not a lost standby. Without a guard, the primary can be +-- reassigned wait_primary right as its own candidate is converging. +-- +-- Rule 2 ("prepare_promotion -> stop_replication" block): assigns the +-- primary demote_timeout as soon as the candidate reports +-- prepare_promotion, unconditionally. demote_timeout has a real +-- KeeperFSM[] edge from primary/join_primary/apply_settings/draining -- +-- but not from wait_primary. If Rule 1 already reassigned the primary to +-- wait_primary, Rule 2 hands out an unreachable demote_timeout, and the +-- keeper fatals on every retry. +-- +-- Fixed by: Rule 1 now skips its reassignment when IsFailoverInProgress() +-- is true (the candidate already being in prepare_promotion counts); +-- Rule 2 now only assigns demote_timeout when the primary's reported +-- state is actually one KeeperFSM[] has an edge to, deferring otherwise. +-- +-- Two scenarios below, each isolating one of the two guards. Neither +-- needs the isolation tester (pg_isolation_regress): both are pure +-- sequencing issues inside ProceedGroupStateFromContext, reproducible +-- deterministically with a straight-line sequence of node_active() calls +-- and (for the states that are otherwise hard to reach synchronously) a +-- direct UPDATE to manufacture the precondition -- the same technique +-- stale_primary_report.sql already uses for a similarly hard-to-reach +-- scenario (a primary that never gets to report its own demotion). +\x on +-- ═══════════════════════════════════════════════════════════════════════ +-- Scenario A (Rule 1 / Fix 1): a candidate already mid-promotion must not +-- be treated as "the standby is just gone" by the primary's own routine +-- report. +-- ═══════════════════════════════════════════════════════════════════════ +SELECT pgautofailover.create_formation('fclma_test', 'pgsql', 'postgres', true, 0); +-[ RECORD 1 ]----+-------------------------------- +create_formation | (fclma_test,pgsql,postgres,t,0) + +SELECT * + FROM pgautofailover.register_node('fclma_test', 'fclma_p', 5432, + 'postgres', 'fclma_p', 1); +-[ RECORD 1 ]---------------+-------- +assigned_node_id | 24 +assigned_group_id | 0 +assigned_group_state | single +assigned_candidate_priority | 100 +assigned_replication_quorum | t +assigned_node_name | fclma_p + +SELECT nodeid AS np FROM pgautofailover.node + WHERE formationid = 'fclma_test' AND nodename = 'fclma_p' \gset +SELECT * + FROM pgautofailover.register_node('fclma_test', 'fclma_s', 5432, + 'postgres', 'fclma_s', 1); +-[ RECORD 1 ]---------------+------------- +assigned_node_id | 25 +assigned_group_id | 0 +assigned_group_state | wait_standby +assigned_candidate_priority | 100 +assigned_replication_quorum | t +assigned_node_name | fclma_s + +SELECT nodeid AS ns FROM pgautofailover.node + WHERE formationid = 'fclma_test' AND nodename = 'fclma_s' \gset +-- bootstrap: p = primary, s = secondary +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :np, 0, + current_group_role => 'single'); +-[ RECORD 1 ]--------+------- +assigned_group_state | single + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :ns, 0, + current_group_role => 'wait_standby'); +-[ RECORD 1 ]--------+------------- +assigned_group_state | wait_standby + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :np, 0, + current_group_role => 'single', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+------------- +assigned_group_state | wait_primary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :np, 0, + current_group_role => 'wait_primary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+------------- +assigned_group_state | wait_primary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :ns, 0, + current_group_role => 'wait_standby'); +-[ RECORD 1 ]--------+----------- +assigned_group_state | catchingup + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :ns, 0, + current_group_role => 'catchingup', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+---------- +assigned_group_state | secondary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :ns, 0, + current_group_role => 'secondary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+---------- +assigned_group_state | secondary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :np, 0, + current_group_role => 'wait_primary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+-------- +assigned_group_state | primary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :np, 0, + current_group_role => 'primary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+-------- +assigned_group_state | primary + +SELECT nodename, reportedstate, goalstate + FROM pgautofailover.node + WHERE formationid = 'fclma_test' + ORDER BY nodename; +-[ RECORD 1 ]-+---------- +nodename | fclma_p +reportedstate | primary +goalstate | primary +-[ RECORD 2 ]-+---------- +nodename | fclma_s +reportedstate | secondary +goalstate | secondary + +-- The candidate is already fully converged at prepare_promotion (a +-- promotion is genuinely in progress -- this is what IsFailoverInProgress() +-- must recognize). This can be reached for real via perform_promotion() or +-- a multi-standby perform_failover(); manufactured directly here to isolate +-- Rule 1 from everything else. +UPDATE pgautofailover.node + SET reportedstate = 'prepare_promotion', + goalstate = 'prepare_promotion' + WHERE formationid = 'fclma_test' AND nodename = 'fclma_s'; +-- The primary's own next routine report must NOT be reassigned +-- wait_primary just because its one standby is no longer "secondary" -- +-- that standby is the active failover candidate, not a lost node. +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :np, 0, + current_group_role => 'primary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+-------- +assigned_group_state | primary + +-- ASSERT: fclma_p's goalstate is still 'primary'. +-- Pre-fix: Rule 1 reassigns it to 'wait_primary' here. +SELECT nodename, reportedstate, goalstate + FROM pgautofailover.node + WHERE formationid = 'fclma_test' + ORDER BY nodename; +-[ RECORD 1 ]-+------------------ +nodename | fclma_p +reportedstate | primary +goalstate | primary +-[ RECORD 2 ]-+------------------ +nodename | fclma_s +reportedstate | prepare_promotion +goalstate | prepare_promotion + +-- ═══════════════════════════════════════════════════════════════════════ +-- Scenario B (Rule 2 / Fix 2): demote_timeout must never be assigned to a +-- primary sitting at wait_primary -- there is no KeeperFSM[] edge from +-- wait_primary to demote_timeout, so that assignment fatals the keeper +-- forever. +-- ═══════════════════════════════════════════════════════════════════════ +SELECT pgautofailover.create_formation('fclmb_test', 'pgsql', 'postgres', true, 0); +-[ RECORD 1 ]----+-------------------------------- +create_formation | (fclmb_test,pgsql,postgres,t,0) + +SELECT * + FROM pgautofailover.register_node('fclmb_test', 'fclmb_p', 5432, + 'postgres', 'fclmb_p', 1); +-[ RECORD 1 ]---------------+-------- +assigned_node_id | 26 +assigned_group_id | 0 +assigned_group_state | single +assigned_candidate_priority | 100 +assigned_replication_quorum | t +assigned_node_name | fclmb_p + +SELECT nodeid AS np FROM pgautofailover.node + WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_p' \gset +SELECT * + FROM pgautofailover.register_node('fclmb_test', 'fclmb_s', 5432, + 'postgres', 'fclmb_s', 1); +-[ RECORD 1 ]---------------+------------- +assigned_node_id | 27 +assigned_group_id | 0 +assigned_group_state | wait_standby +assigned_candidate_priority | 100 +assigned_replication_quorum | t +assigned_node_name | fclmb_s + +SELECT nodeid AS ns FROM pgautofailover.node + WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_s' \gset +-- bootstrap: p = primary, s = secondary +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :np, 0, + current_group_role => 'single'); +-[ RECORD 1 ]--------+------- +assigned_group_state | single + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :ns, 0, + current_group_role => 'wait_standby'); +-[ RECORD 1 ]--------+------------- +assigned_group_state | wait_standby + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :np, 0, + current_group_role => 'single', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+------------- +assigned_group_state | wait_primary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :np, 0, + current_group_role => 'wait_primary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+------------- +assigned_group_state | wait_primary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :ns, 0, + current_group_role => 'wait_standby'); +-[ RECORD 1 ]--------+----------- +assigned_group_state | catchingup + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :ns, 0, + current_group_role => 'catchingup', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+---------- +assigned_group_state | secondary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :ns, 0, + current_group_role => 'secondary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+---------- +assigned_group_state | secondary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :np, 0, + current_group_role => 'wait_primary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+-------- +assigned_group_state | primary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :np, 0, + current_group_role => 'primary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+-------- +assigned_group_state | primary + +SELECT nodename, reportedstate, goalstate + FROM pgautofailover.node + WHERE formationid = 'fclmb_test' + ORDER BY nodename; +-[ RECORD 1 ]-+---------- +nodename | fclmb_p +reportedstate | primary +goalstate | primary +-[ RECORD 2 ]-+---------- +nodename | fclmb_s +reportedstate | secondary +goalstate | secondary + +-- Manufacture the state Rule 1 would have produced without Fix 1 above -- +-- the primary fully converged at wait_primary, whatever the exact prior +-- trigger. This isolates Rule 2's behaviour from Rule 1's. +UPDATE pgautofailover.node + SET reportedstate = 'wait_primary', + goalstate = 'wait_primary' + WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_p'; +-- The candidate is already fully converged at prepare_promotion, same as in +-- Scenario A -- IsCurrentState() requires goalstate == reportedstate == the +-- state being tested, so a node_active() call alone would only move +-- reportedstate and leave goalstate at 'secondary' from the bootstrap above, +-- never matching Rule 2's IsCurrentState(activeNode, PREPARE_PROMOTION) +-- check. Manufacture full convergence directly, then re-affirm via +-- node_active() to trigger the dispatch. +UPDATE pgautofailover.node + SET reportedstate = 'prepare_promotion', + goalstate = 'prepare_promotion' + WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_s'; +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :ns, 0, + current_group_role => 'prepare_promotion', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+------------------ +assigned_group_state | prepare_promotion + +-- ASSERT: fclmb_p's goalstate is still 'wait_primary', not 'demote_timeout'. +-- Pre-fix: Rule 2 assigns demote_timeout unconditionally here, producing +-- reportedstate=wait_primary / goalstate=demote_timeout -- no KeeperFSM[] +-- edge between them, so the keeper would fatal on every retry. +SELECT nodename, reportedstate, goalstate + FROM pgautofailover.node + WHERE formationid = 'fclmb_test' + ORDER BY nodename; +-[ RECORD 1 ]-+------------------ +nodename | fclmb_p +reportedstate | wait_primary +goalstate | wait_primary +-[ RECORD 2 ]-+------------------ +nodename | fclmb_s +reportedstate | prepare_promotion +goalstate | prepare_promotion + diff --git a/src/monitor/group_state_machine.c b/src/monitor/group_state_machine.c index 8927c916f..70efea510 100644 --- a/src/monitor/group_state_machine.c +++ b/src/monitor/group_state_machine.c @@ -847,10 +847,28 @@ ProceedGroupStateFromContext(GroupStateContext *ctx) * refrain from prepare_maintenance -> demote_timeout on the primary, which * might happen here when secondary has reached prepare_promotion before * primary has reached prepare_maintenance. + * + * Also require the primary to actually be in one of the states + * demote_timeout has a real KeeperFSM[] edge from (primary, join_primary, + * apply_settings, or draining -- verified against fsm.c) before assigning + * it: this rule fires as soon as the candidate reports prepare_promotion, + * which routinely happens while the primary is still reporting "primary" + * itself (it hasn't locally converged to draining yet) -- that's the + * normal case and demote_timeout is reachable from primary too, so it's + * allowed. What's not reachable is wait_primary: a separate rule can + * reassign the primary to wait_primary in the same window (issue #774), + * and wait_primary has no FSM edge to demote_timeout at all, which fatals + * the node forever. If the primary is at wait_primary (or anything else + * with no edge to demote_timeout) when this fires, defer instead: do + * nothing this tick and let the next node_active() round re-evaluate. */ if (IsCurrentState(activeNode, REPLICATION_STATE_PREPARE_PROMOTION) && primaryNode && - !IsInMaintenance(primaryNode)) + !IsInMaintenance(primaryNode) && + (primaryNode->reportedState == REPLICATION_STATE_PRIMARY || + primaryNode->reportedState == REPLICATION_STATE_JOIN_PRIMARY || + primaryNode->reportedState == REPLICATION_STATE_APPLY_SETTINGS || + primaryNode->reportedState == REPLICATION_STATE_DRAINING)) { char message[BUFSIZE]; @@ -1319,9 +1337,20 @@ ProceedGroupStateForPrimaryNode(GroupStateContext *ctx, * the two defective standby nodes is available again. */ if (!IsCurrentState(primaryNode, REPLICATION_STATE_WAIT_PRIMARY) && - secondaryQuorumNodesCount == 0) + secondaryQuorumNodesCount == 0 && + !IsFailoverInProgress(ctx->groupNodeList)) { /* + * Do not second-guess an already-started failover: a candidate + * that just converged to prepare_promotion (or later stages) has + * left SECONDARY, dropping secondaryQuorumNodesCount to zero even + * though it *is* the failover candidate. Without this guard the + * primary can be reassigned wait_primary right as its own + * candidate is converging, landing it on wait_primary while a + * later rule still expects to find it in draining and assigns + * demote_timeout -- an assignment with no FSM edge from + * wait_primary (issue #774). + * * Allow wait_primary when number_sync_standbys = 0, otherwise * block writes on the primary. */ diff --git a/src/monitor/regress_schedule b/src/monitor/regress_schedule index d09d29af6..c44e50f93 100644 --- a/src/monitor/regress_schedule +++ b/src/monitor/regress_schedule @@ -40,6 +40,7 @@ test: drop_node test: stale_primary_report test: lock_and_fetch_migration test: timeline_fork_detection +test: failover_candidate_leaves_secondary test: dummy_update test: drop_extension test: upgrade diff --git a/src/monitor/sql/failover_candidate_leaves_secondary.sql b/src/monitor/sql/failover_candidate_leaves_secondary.sql new file mode 100644 index 000000000..0ef4c9ddb --- /dev/null +++ b/src/monitor/sql/failover_candidate_leaves_secondary.sql @@ -0,0 +1,225 @@ +-- Copyright (c) Microsoft Corporation. All rights reserved. +-- Licensed under the PostgreSQL License. +-- +-- Regression test for issue #774: a monitor rule can assign a node a goal +-- state its own KeeperFSM[] (src/bin/pg_autoctl/fsm.c) has no transition +-- path to reach from that node's current state, fataling the keeper +-- forever ("does not know how to reach state ... from ..."). +-- +-- Two independent rules in group_state_machine.c interact badly: +-- +-- Rule 1 (ProceedGroupStateForPrimaryNode, "no healthy standby in the +-- quorum" block): reassigns the primary to wait_primary whenever +-- secondaryQuorumNodesCount == 0. That count drops to zero the instant a +-- failover candidate leaves SECONDARY (e.g. converges to +-- prepare_promotion) -- even though the candidate *is* the failover in +-- progress, not a lost standby. Without a guard, the primary can be +-- reassigned wait_primary right as its own candidate is converging. +-- +-- Rule 2 ("prepare_promotion -> stop_replication" block): assigns the +-- primary demote_timeout as soon as the candidate reports +-- prepare_promotion, unconditionally. demote_timeout has a real +-- KeeperFSM[] edge from primary/join_primary/apply_settings/draining -- +-- but not from wait_primary. If Rule 1 already reassigned the primary to +-- wait_primary, Rule 2 hands out an unreachable demote_timeout, and the +-- keeper fatals on every retry. +-- +-- Fixed by: Rule 1 now skips its reassignment when IsFailoverInProgress() +-- is true (the candidate already being in prepare_promotion counts); +-- Rule 2 now only assigns demote_timeout when the primary's reported +-- state is actually one KeeperFSM[] has an edge to, deferring otherwise. +-- +-- Two scenarios below, each isolating one of the two guards. Neither +-- needs the isolation tester (pg_isolation_regress): both are pure +-- sequencing issues inside ProceedGroupStateFromContext, reproducible +-- deterministically with a straight-line sequence of node_active() calls +-- and (for the states that are otherwise hard to reach synchronously) a +-- direct UPDATE to manufacture the precondition -- the same technique +-- stale_primary_report.sql already uses for a similarly hard-to-reach +-- scenario (a primary that never gets to report its own demotion). + +\x on + +-- ═══════════════════════════════════════════════════════════════════════ +-- Scenario A (Rule 1 / Fix 1): a candidate already mid-promotion must not +-- be treated as "the standby is just gone" by the primary's own routine +-- report. +-- ═══════════════════════════════════════════════════════════════════════ + +SELECT pgautofailover.create_formation('fclma_test', 'pgsql', 'postgres', true, 0); + +SELECT * + FROM pgautofailover.register_node('fclma_test', 'fclma_p', 5432, + 'postgres', 'fclma_p', 1); + +SELECT nodeid AS np FROM pgautofailover.node + WHERE formationid = 'fclma_test' AND nodename = 'fclma_p' \gset + +SELECT * + FROM pgautofailover.register_node('fclma_test', 'fclma_s', 5432, + 'postgres', 'fclma_s', 1); + +SELECT nodeid AS ns FROM pgautofailover.node + WHERE formationid = 'fclma_test' AND nodename = 'fclma_s' \gset + +-- bootstrap: p = primary, s = secondary +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :np, 0, + current_group_role => 'single'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :ns, 0, + current_group_role => 'wait_standby'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :np, 0, + current_group_role => 'single', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :np, 0, + current_group_role => 'wait_primary', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :ns, 0, + current_group_role => 'wait_standby'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :ns, 0, + current_group_role => 'catchingup', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :ns, 0, + current_group_role => 'secondary', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :np, 0, + current_group_role => 'wait_primary', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :np, 0, + current_group_role => 'primary', + current_lsn => '0/5000'); + +SELECT nodename, reportedstate, goalstate + FROM pgautofailover.node + WHERE formationid = 'fclma_test' + ORDER BY nodename; + +-- The candidate is already fully converged at prepare_promotion (a +-- promotion is genuinely in progress -- this is what IsFailoverInProgress() +-- must recognize). This can be reached for real via perform_promotion() or +-- a multi-standby perform_failover(); manufactured directly here to isolate +-- Rule 1 from everything else. +UPDATE pgautofailover.node + SET reportedstate = 'prepare_promotion', + goalstate = 'prepare_promotion' + WHERE formationid = 'fclma_test' AND nodename = 'fclma_s'; + +-- The primary's own next routine report must NOT be reassigned +-- wait_primary just because its one standby is no longer "secondary" -- +-- that standby is the active failover candidate, not a lost node. +SELECT assigned_group_state + FROM pgautofailover.node_active('fclma_test', :np, 0, + current_group_role => 'primary', + current_lsn => '0/5000'); + +-- ASSERT: fclma_p's goalstate is still 'primary'. +-- Pre-fix: Rule 1 reassigns it to 'wait_primary' here. +SELECT nodename, reportedstate, goalstate + FROM pgautofailover.node + WHERE formationid = 'fclma_test' + ORDER BY nodename; + +-- ═══════════════════════════════════════════════════════════════════════ +-- Scenario B (Rule 2 / Fix 2): demote_timeout must never be assigned to a +-- primary sitting at wait_primary -- there is no KeeperFSM[] edge from +-- wait_primary to demote_timeout, so that assignment fatals the keeper +-- forever. +-- ═══════════════════════════════════════════════════════════════════════ + +SELECT pgautofailover.create_formation('fclmb_test', 'pgsql', 'postgres', true, 0); + +SELECT * + FROM pgautofailover.register_node('fclmb_test', 'fclmb_p', 5432, + 'postgres', 'fclmb_p', 1); + +SELECT nodeid AS np FROM pgautofailover.node + WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_p' \gset + +SELECT * + FROM pgautofailover.register_node('fclmb_test', 'fclmb_s', 5432, + 'postgres', 'fclmb_s', 1); + +SELECT nodeid AS ns FROM pgautofailover.node + WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_s' \gset + +-- bootstrap: p = primary, s = secondary +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :np, 0, + current_group_role => 'single'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :ns, 0, + current_group_role => 'wait_standby'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :np, 0, + current_group_role => 'single', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :np, 0, + current_group_role => 'wait_primary', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :ns, 0, + current_group_role => 'wait_standby'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :ns, 0, + current_group_role => 'catchingup', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :ns, 0, + current_group_role => 'secondary', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :np, 0, + current_group_role => 'wait_primary', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :np, 0, + current_group_role => 'primary', + current_lsn => '0/5000'); + +SELECT nodename, reportedstate, goalstate + FROM pgautofailover.node + WHERE formationid = 'fclmb_test' + ORDER BY nodename; + +-- Manufacture the state Rule 1 would have produced without Fix 1 above -- +-- the primary fully converged at wait_primary, whatever the exact prior +-- trigger. This isolates Rule 2's behaviour from Rule 1's. +UPDATE pgautofailover.node + SET reportedstate = 'wait_primary', + goalstate = 'wait_primary' + WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_p'; + +-- The candidate is already fully converged at prepare_promotion, same as in +-- Scenario A -- IsCurrentState() requires goalstate == reportedstate == the +-- state being tested, so a node_active() call alone would only move +-- reportedstate and leave goalstate at 'secondary' from the bootstrap above, +-- never matching Rule 2's IsCurrentState(activeNode, PREPARE_PROMOTION) +-- check. Manufacture full convergence directly, then re-affirm via +-- node_active() to trigger the dispatch. +UPDATE pgautofailover.node + SET reportedstate = 'prepare_promotion', + goalstate = 'prepare_promotion' + WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_s'; + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmb_test', :ns, 0, + current_group_role => 'prepare_promotion', + current_lsn => '0/5000'); + +-- ASSERT: fclmb_p's goalstate is still 'wait_primary', not 'demote_timeout'. +-- Pre-fix: Rule 2 assigns demote_timeout unconditionally here, producing +-- reportedstate=wait_primary / goalstate=demote_timeout -- no KeeperFSM[] +-- edge between them, so the keeper would fatal on every retry. +SELECT nodename, reportedstate, goalstate + FROM pgautofailover.node + WHERE formationid = 'fclmb_test' + ORDER BY nodename; From cd3ba604112d55a4be5c9af922fa4ae0cd687021 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Mon, 27 Jul 2026 04:30:13 +0200 Subject: [PATCH 2/6] Fix a second #774 gap: candidate stalls forever if the primary died before ever reaching PRIMARY Found via the actual CI failure (pgaftest multi-async, test_014_003_ restart_node1): when a primary is killed while still at wait_primary (never got a healthy secondary counted before being promoted the rest of the way), GetPrimaryOrDemotedNodeInGroupFromList still resolves it as primaryNode (its goal state, wait_primary, is writable). The previous fix's Rule 2 guard correctly refuses to assign it demote_timeout (no KeeperFSM[] edge from wait_primary), but the primaryNode == NULL fallback never fires either, since primaryNode is non-NULL -- the candidate is left stuck at prepare_promotion forever. Broadened the fallback rule to also fire whenever primaryNode exists but is not in a state with a real demote_timeout edge (mirroring the guard's own restricted set): there is no live primary to hand off to either way, so promote the candidate directly, exactly as when primaryNode is NULL. Updated failover_candidate_leaves_secondary.sql's Scenario B to assert the corrected behavior: the candidate now reaches wait_primary directly instead of deferring. Verified via: - `make installcheck` (pg_regress in Docker): all 13 monitor tests + isolation tests pass. - Reproduced the CI hang locally with own-tagged Docker images (pgaf774dbg:pg17 / pgaf774dbg:pgaftest) running the actual multi-async pgaftest schedule; confirmed test_014_003_restart_node1 now passes (previously hung for the full 180s timeout). --- .../failover_candidate_leaves_secondary.out | 12 ++++++---- src/monitor/group_state_machine.c | 22 ++++++++++++++++--- .../failover_candidate_leaves_secondary.sql | 6 ++++- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/monitor/expected/failover_candidate_leaves_secondary.out b/src/monitor/expected/failover_candidate_leaves_secondary.out index cf4080fa1..dc07967de 100644 --- a/src/monitor/expected/failover_candidate_leaves_secondary.out +++ b/src/monitor/expected/failover_candidate_leaves_secondary.out @@ -313,10 +313,14 @@ SELECT assigned_group_state FROM pgautofailover.node_active('fclmb_test', :ns, 0, current_group_role => 'prepare_promotion', current_lsn => '0/5000'); --[ RECORD 1 ]--------+------------------ -assigned_group_state | prepare_promotion +-[ RECORD 1 ]--------+------------- +assigned_group_state | wait_primary --- ASSERT: fclmb_p's goalstate is still 'wait_primary', not 'demote_timeout'. +-- ASSERT: fclmb_p's goalstate is still 'wait_primary', not 'demote_timeout', +-- and fclmb_s is promoted directly to 'wait_primary' -- fclmb_p is stuck at +-- wait_primary with no path to become a live primary, so it is treated the +-- same as "no primary at all" (the primaryNode == NULL case) rather than +-- waiting forever for a demote handshake that can never happen. -- Pre-fix: Rule 2 assigns demote_timeout unconditionally here, producing -- reportedstate=wait_primary / goalstate=demote_timeout -- no KeeperFSM[] -- edge between them, so the keeper would fatal on every retry. @@ -331,5 +335,5 @@ goalstate | wait_primary -[ RECORD 2 ]-+------------------ nodename | fclmb_s reportedstate | prepare_promotion -goalstate | prepare_promotion +goalstate | wait_primary diff --git a/src/monitor/group_state_machine.c b/src/monitor/group_state_machine.c index 70efea510..3628934b6 100644 --- a/src/monitor/group_state_machine.c +++ b/src/monitor/group_state_machine.c @@ -892,11 +892,27 @@ ProceedGroupStateFromContext(GroupStateContext *ctx) } /* - * when primary node has been removed and we are promoting one standby - * prepare_promotion -> stop_replication + * when primary node has been removed, or never actually reached a state + * with a real demote_timeout edge, and we are promoting one standby + * prepare_promotion -> wait_primary + * + * This is the complement of the rule above: primaryNode can be non-NULL + * here and still have no live primary to hand off to -- e.g. a primary + * candidate that reached wait_primary (a writable goal state, so it's + * still found by GetPrimaryOrDemotedNodeInGroupFromList) but died before + * ever getting a healthy secondary counted and being promoted the rest + * of the way to primary. wait_primary has no KeeperFSM[] edge to + * demote_timeout, so waiting for the rule above to fire would wait + * forever (issue #774): there is no live primary to demote, so just + * promote the candidate directly, exactly as when primaryNode is NULL. */ if (IsCurrentState(activeNode, REPLICATION_STATE_PREPARE_PROMOTION) && - primaryNode == NULL) + (primaryNode == NULL || + (!IsInMaintenance(primaryNode) && + primaryNode->reportedState != REPLICATION_STATE_PRIMARY && + primaryNode->reportedState != REPLICATION_STATE_JOIN_PRIMARY && + primaryNode->reportedState != REPLICATION_STATE_APPLY_SETTINGS && + primaryNode->reportedState != REPLICATION_STATE_DRAINING))) { char message[BUFSIZE] = { 0 }; diff --git a/src/monitor/sql/failover_candidate_leaves_secondary.sql b/src/monitor/sql/failover_candidate_leaves_secondary.sql index 0ef4c9ddb..d19f0d9b6 100644 --- a/src/monitor/sql/failover_candidate_leaves_secondary.sql +++ b/src/monitor/sql/failover_candidate_leaves_secondary.sql @@ -215,7 +215,11 @@ SELECT assigned_group_state current_group_role => 'prepare_promotion', current_lsn => '0/5000'); --- ASSERT: fclmb_p's goalstate is still 'wait_primary', not 'demote_timeout'. +-- ASSERT: fclmb_p's goalstate is still 'wait_primary', not 'demote_timeout', +-- and fclmb_s is promoted directly to 'wait_primary' -- fclmb_p is stuck at +-- wait_primary with no path to become a live primary, so it is treated the +-- same as "no primary at all" (the primaryNode == NULL case) rather than +-- waiting forever for a demote handshake that can never happen. -- Pre-fix: Rule 2 assigns demote_timeout unconditionally here, producing -- reportedstate=wait_primary / goalstate=demote_timeout -- no KeeperFSM[] -- edge between them, so the keeper would fatal on every retry. From ffb72a176c2e07a5ac1af3ebd1b0a3ae5b5b6c14 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Mon, 27 Jul 2026 14:14:04 +0200 Subject: [PATCH 3/6] Guard both prepare_promotion rules on the primary being "settled" (converged or unhealthy), not just its reportedState The two rules that decide what to do with a candidate converged at prepare_promotion both act on primaryNode's reportedState to choose between the demote_timeout path and the direct-promotion fallback. Neither previously checked whether primaryNode was actually done changing: a primary that is healthy and legitimately still converging toward a real "primary" (goalState already PRIMARY, e.g. after a replication-quorum change, but reportedState still lagging one step behind at a real predecessor like apply_settings) could have its lagging reportedState misread as "no live primary", triggering the fallback and promoting a second candidate straight to wait_primary while the first primary was seconds away from completing its own promotion on its own -- two simultaneously-writable nodes, which is worse than the unreachable-goal-state bug (#774) these rules exist to prevent. Both rules now require primaryNode to be "settled" before acting on its reportedState at all: either primaryNode == NULL, or it has fully converged to its own currently assigned goal (reportedState == goalState), or it is confirmed unhealthy (NodeIsUnhealthy) and therefore cannot make further progress toward that goal regardless. If primaryNode is healthy but still mid-transition, neither rule fires this tick; the next node_active() round re-evaluates once it either converges or is confirmed unhealthy. Also fixed a maintenance-handling regression introduced while merging the two rules' guards: a primaryNode in maintenance must defer entirely (as before), not fall through to the direct-promotion fallback just because it doesn't match the demote_timeout-reachable state set. Added Scenario C to failover_candidate_leaves_secondary.sql, manufacturing exactly the healthy-but-unconverged primary case and asserting the candidate's goal is left untouched at prepare_promotion (deferred) rather than hastily promoted. Verified via `make installcheck` (pg_regress in Docker): all 13 monitor tests (including the new Scenario C) + isolation tests pass. Ran citus_indent (docker run citus/stylechecker:no-py) and reformatted accordingly; re-verified full suite passes after reformatting. --- .../failover_candidate_leaves_secondary.out | 160 ++++++++++++++++++ src/monitor/group_state_machine.c | 160 ++++++++++-------- .../failover_candidate_leaves_secondary.sql | 105 ++++++++++++ 3 files changed, 352 insertions(+), 73 deletions(-) diff --git a/src/monitor/expected/failover_candidate_leaves_secondary.out b/src/monitor/expected/failover_candidate_leaves_secondary.out index dc07967de..91138fd73 100644 --- a/src/monitor/expected/failover_candidate_leaves_secondary.out +++ b/src/monitor/expected/failover_candidate_leaves_secondary.out @@ -337,3 +337,163 @@ nodename | fclmb_s reportedstate | prepare_promotion goalstate | wait_primary +-- ═══════════════════════════════════════════════════════════════════════ +-- Scenario C (the "settled" guard): neither Rule 2's demote_timeout path nor +-- its wait_primary fallback may fire while the primary is healthy but has +-- not yet converged to its own assigned goal -- it might still complete +-- that transition (e.g. reach a real primary) on its own, and deciding +-- based on a reportedState that's just lagging behind an in-progress, +-- healthy convergence risks promoting a second candidate while the first +-- primary is still legitimately alive and progressing, producing two +-- simultaneously-writable nodes. +-- ═══════════════════════════════════════════════════════════════════════ +SELECT pgautofailover.create_formation('fclmc_test', 'pgsql', 'postgres', true, 0); +-[ RECORD 1 ]----+-------------------------------- +create_formation | (fclmc_test,pgsql,postgres,t,0) + +SELECT * + FROM pgautofailover.register_node('fclmc_test', 'fclmc_p', 5432, + 'postgres', 'fclmc_p', 1); +-[ RECORD 1 ]---------------+-------- +assigned_node_id | 28 +assigned_group_id | 0 +assigned_group_state | single +assigned_candidate_priority | 100 +assigned_replication_quorum | t +assigned_node_name | fclmc_p + +SELECT nodeid AS np FROM pgautofailover.node + WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_p' \gset +SELECT * + FROM pgautofailover.register_node('fclmc_test', 'fclmc_s', 5432, + 'postgres', 'fclmc_s', 1); +-[ RECORD 1 ]---------------+------------- +assigned_node_id | 29 +assigned_group_id | 0 +assigned_group_state | wait_standby +assigned_candidate_priority | 100 +assigned_replication_quorum | t +assigned_node_name | fclmc_s + +SELECT nodeid AS ns FROM pgautofailover.node + WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_s' \gset +-- bootstrap: p = primary, s = secondary +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :np, 0, + current_group_role => 'single'); +-[ RECORD 1 ]--------+------- +assigned_group_state | single + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :ns, 0, + current_group_role => 'wait_standby'); +-[ RECORD 1 ]--------+------------- +assigned_group_state | wait_standby + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :np, 0, + current_group_role => 'single', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+------------- +assigned_group_state | wait_primary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :np, 0, + current_group_role => 'wait_primary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+------------- +assigned_group_state | wait_primary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :ns, 0, + current_group_role => 'wait_standby'); +-[ RECORD 1 ]--------+----------- +assigned_group_state | catchingup + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :ns, 0, + current_group_role => 'catchingup', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+---------- +assigned_group_state | secondary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :ns, 0, + current_group_role => 'secondary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+---------- +assigned_group_state | secondary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :np, 0, + current_group_role => 'wait_primary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+-------- +assigned_group_state | primary + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :np, 0, + current_group_role => 'primary', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+-------- +assigned_group_state | primary + +SELECT nodename, reportedstate, goalstate + FROM pgautofailover.node + WHERE formationid = 'fclmc_test' + ORDER BY nodename; +-[ RECORD 1 ]-+---------- +nodename | fclmc_p +reportedstate | primary +goalstate | primary +-[ RECORD 2 ]-+---------- +nodename | fclmc_s +reportedstate | secondary +goalstate | secondary + +-- Manufacture the primary mid-transition toward its own goal: goalstate is +-- already 'primary' (assigned by the monitor, e.g. after a replication +-- property change), but the keeper has only reported back as far as +-- 'apply_settings' -- a real, in-progress, healthy predecessor of primary +-- (apply_settings -> primary is a genuine KeeperFSM[] edge), not a dead +-- node. Recency of the last report (from the node_active() calls just +-- above) keeps it "healthy" per NodeIsUnhealthy()'s reportTime check. +UPDATE pgautofailover.node + SET reportedstate = 'apply_settings', + goalstate = 'primary' + WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_p'; +-- Manufacture the candidate fully converged at prepare_promotion, same +-- technique as Scenario A/B. +UPDATE pgautofailover.node + SET reportedstate = 'prepare_promotion', + goalstate = 'prepare_promotion' + WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_s'; +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :ns, 0, + current_group_role => 'prepare_promotion', + current_lsn => '0/5000'); +-[ RECORD 1 ]--------+------------------ +assigned_group_state | prepare_promotion + +-- ASSERT: fclmc_s's goalstate is still 'prepare_promotion' (deferred, no +-- action taken this tick), and fclmc_p's goalstate is untouched at +-- 'primary' -- neither Rule 2 branch may fire while the primary is healthy +-- and still converging toward its own goal. +-- Before the "settled" guard: this would have incorrectly matched the +-- direct-promotion fallback (reportedState 'apply_settings' is not in the +-- demote_timeout-reachable set), promoting fclmc_s to wait_primary right +-- as fclmc_p was legitimately about to reach primary on its own -- two +-- simultaneously-writable nodes. +SELECT nodename, reportedstate, goalstate + FROM pgautofailover.node + WHERE formationid = 'fclmc_test' + ORDER BY nodename; +-[ RECORD 1 ]-+------------------ +nodename | fclmc_p +reportedstate | apply_settings +goalstate | primary +-[ RECORD 2 ]-+------------------ +nodename | fclmc_s +reportedstate | prepare_promotion +goalstate | prepare_promotion + diff --git a/src/monitor/group_state_machine.c b/src/monitor/group_state_machine.c index 3628934b6..a647ad4c7 100644 --- a/src/monitor/group_state_machine.c +++ b/src/monitor/group_state_machine.c @@ -843,91 +843,105 @@ ProceedGroupStateFromContext(GroupStateContext *ctx) /* * when node is seeing no more writes: * prepare_promotion -> stop_replication + * (or, when there's no live primary to hand off to: -> wait_primary) * - * refrain from prepare_maintenance -> demote_timeout on the primary, which - * might happen here when secondary has reached prepare_promotion before - * primary has reached prepare_maintenance. - * - * Also require the primary to actually be in one of the states - * demote_timeout has a real KeeperFSM[] edge from (primary, join_primary, - * apply_settings, or draining -- verified against fsm.c) before assigning - * it: this rule fires as soon as the candidate reports prepare_promotion, - * which routinely happens while the primary is still reporting "primary" - * itself (it hasn't locally converged to draining yet) -- that's the - * normal case and demote_timeout is reachable from primary too, so it's - * allowed. What's not reachable is wait_primary: a separate rule can - * reassign the primary to wait_primary in the same window (issue #774), - * and wait_primary has no FSM edge to demote_timeout at all, which fatals - * the node forever. If the primary is at wait_primary (or anything else - * with no edge to demote_timeout) when this fires, defer instead: do - * nothing this tick and let the next node_active() round re-evaluate. + * Both of the two rules below act on primaryNode's reported state to + * decide what to do with it, so neither may fire until primaryNode is + * "settled": either it has fully converged to its own currently assigned + * goal (reportedState == goalState), or it's confirmed unhealthy (not + * responding, so it cannot make further progress toward that goal right + * now regardless). If primaryNode is healthy but still mid-transition + * toward its own goal, defer instead: it might complete that transition + * (e.g. actually reach primary) on its own before we ever need to hand + * off to a different candidate, and deciding based on a reportedState + * that's just lagging behind an in-progress, healthy convergence risks + * racing ahead of it -- producing two simultaneously-writable nodes, + * which is strictly worse than the unreachable-goal-state bug (#774) + * these two rules exist to prevent in the first place. */ if (IsCurrentState(activeNode, REPLICATION_STATE_PREPARE_PROMOTION) && - primaryNode && - !IsInMaintenance(primaryNode) && - (primaryNode->reportedState == REPLICATION_STATE_PRIMARY || - primaryNode->reportedState == REPLICATION_STATE_JOIN_PRIMARY || - primaryNode->reportedState == REPLICATION_STATE_APPLY_SETTINGS || - primaryNode->reportedState == REPLICATION_STATE_DRAINING)) + (primaryNode == NULL || + (!IsInMaintenance(primaryNode) && + (IsCurrentState(primaryNode, primaryNode->goalState) || + NodeIsUnhealthy(primaryNode, ctx))))) { - char message[BUFSIZE]; + /* + * refrain from prepare_maintenance -> demote_timeout on the primary + * (excluded via IsInMaintenance above), which might happen here when + * secondary has reached prepare_promotion before primary has reached + * prepare_maintenance -- deferring entirely rather than falling + * through to the direct-promotion branch below, since a primary + * that's deliberately in maintenance is not "gone", just paused. + * + * Also require the primary to actually be in one of the states + * demote_timeout has a real KeeperFSM[] edge from (primary, + * join_primary, apply_settings, or draining -- verified against + * fsm.c) before assigning it: this rule fires as soon as the + * candidate reports prepare_promotion, which routinely happens + * while the primary is still reporting "primary" itself (it hasn't + * locally converged to draining yet) -- that's the normal case and + * demote_timeout is reachable from primary too, so it's allowed. + * What's not reachable is wait_primary: a separate rule can + * reassign the primary to wait_primary in the same window (issue + * #774), and wait_primary has no FSM edge to demote_timeout at all, + * which fatals the node forever. + */ + if (primaryNode != NULL && + (primaryNode->reportedState == REPLICATION_STATE_PRIMARY || + primaryNode->reportedState == REPLICATION_STATE_JOIN_PRIMARY || + primaryNode->reportedState == REPLICATION_STATE_APPLY_SETTINGS || + primaryNode->reportedState == REPLICATION_STATE_DRAINING)) + { + char message[BUFSIZE]; - LogAndNotifyMessage( - message, BUFSIZE, - "Setting goal state of " NODE_FORMAT - " to demote_timeout and " NODE_FORMAT - " to stop_replication after " NODE_FORMAT - " converged to prepare_promotion.", - NODE_FORMAT_ARGS(primaryNode), - NODE_FORMAT_ARGS(activeNode), - NODE_FORMAT_ARGS(activeNode)); + LogAndNotifyMessage( + message, BUFSIZE, + "Setting goal state of " NODE_FORMAT + " to demote_timeout and " NODE_FORMAT + " to stop_replication after " NODE_FORMAT + " converged to prepare_promotion.", + NODE_FORMAT_ARGS(primaryNode), + NODE_FORMAT_ARGS(activeNode), + NODE_FORMAT_ARGS(activeNode)); - /* perform promotion to stop replication */ - AssignGoalState(activeNode, REPLICATION_STATE_STOP_REPLICATION, message); + /* perform promotion to stop replication */ + AssignGoalState(activeNode, REPLICATION_STATE_STOP_REPLICATION, + message); - /* wait for possibly-alive primary to kill itself */ - AssignGoalState(primaryNode, REPLICATION_STATE_DEMOTE_TIMEOUT, message); + /* wait for possibly-alive primary to kill itself */ + AssignGoalState(primaryNode, REPLICATION_STATE_DEMOTE_TIMEOUT, + message); - return true; - } + return true; + } - /* - * when primary node has been removed, or never actually reached a state - * with a real demote_timeout edge, and we are promoting one standby - * prepare_promotion -> wait_primary - * - * This is the complement of the rule above: primaryNode can be non-NULL - * here and still have no live primary to hand off to -- e.g. a primary - * candidate that reached wait_primary (a writable goal state, so it's - * still found by GetPrimaryOrDemotedNodeInGroupFromList) but died before - * ever getting a healthy secondary counted and being promoted the rest - * of the way to primary. wait_primary has no KeeperFSM[] edge to - * demote_timeout, so waiting for the rule above to fire would wait - * forever (issue #774): there is no live primary to demote, so just - * promote the candidate directly, exactly as when primaryNode is NULL. - */ - if (IsCurrentState(activeNode, REPLICATION_STATE_PREPARE_PROMOTION) && - (primaryNode == NULL || - (!IsInMaintenance(primaryNode) && - primaryNode->reportedState != REPLICATION_STATE_PRIMARY && - primaryNode->reportedState != REPLICATION_STATE_JOIN_PRIMARY && - primaryNode->reportedState != REPLICATION_STATE_APPLY_SETTINGS && - primaryNode->reportedState != REPLICATION_STATE_DRAINING))) - { - char message[BUFSIZE] = { 0 }; + /* + * primaryNode is either NULL (removed from the group entirely), or + * settled (per the outer guard above) in a state with no real + * demote_timeout edge -- e.g. a primary candidate that reached + * wait_primary (a writable goal state, so it's still found by + * GetPrimaryOrDemotedNodeInGroupFromList) but died before ever + * getting a healthy secondary counted and being promoted the rest + * of the way to primary. Waiting for the block above to fire would + * wait forever (issue #774): there is no live primary to demote, so + * just promote the candidate directly, exactly as when primaryNode + * is NULL. + */ + { + char message[BUFSIZE] = { 0 }; - LogAndNotifyMessage( - message, BUFSIZE, - "Setting goal state of " NODE_FORMAT - " to wait_primary after " NODE_FORMAT - " converged to prepare_promotion.", - NODE_FORMAT_ARGS(activeNode), - NODE_FORMAT_ARGS(activeNode)); + LogAndNotifyMessage( + message, BUFSIZE, + "Setting goal state of " NODE_FORMAT + " to wait_primary after " NODE_FORMAT + " converged to prepare_promotion.", + NODE_FORMAT_ARGS(activeNode), + NODE_FORMAT_ARGS(activeNode)); - /* perform promotion to stop replication */ - AssignGoalState(activeNode, REPLICATION_STATE_WAIT_PRIMARY, message); + AssignGoalState(activeNode, REPLICATION_STATE_WAIT_PRIMARY, message); - return true; + return true; + } } /* diff --git a/src/monitor/sql/failover_candidate_leaves_secondary.sql b/src/monitor/sql/failover_candidate_leaves_secondary.sql index d19f0d9b6..4bea89833 100644 --- a/src/monitor/sql/failover_candidate_leaves_secondary.sql +++ b/src/monitor/sql/failover_candidate_leaves_secondary.sql @@ -227,3 +227,108 @@ SELECT nodename, reportedstate, goalstate FROM pgautofailover.node WHERE formationid = 'fclmb_test' ORDER BY nodename; + +-- ═══════════════════════════════════════════════════════════════════════ +-- Scenario C (the "settled" guard): neither Rule 2's demote_timeout path nor +-- its wait_primary fallback may fire while the primary is healthy but has +-- not yet converged to its own assigned goal -- it might still complete +-- that transition (e.g. reach a real primary) on its own, and deciding +-- based on a reportedState that's just lagging behind an in-progress, +-- healthy convergence risks promoting a second candidate while the first +-- primary is still legitimately alive and progressing, producing two +-- simultaneously-writable nodes. +-- ═══════════════════════════════════════════════════════════════════════ + +SELECT pgautofailover.create_formation('fclmc_test', 'pgsql', 'postgres', true, 0); + +SELECT * + FROM pgautofailover.register_node('fclmc_test', 'fclmc_p', 5432, + 'postgres', 'fclmc_p', 1); + +SELECT nodeid AS np FROM pgautofailover.node + WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_p' \gset + +SELECT * + FROM pgautofailover.register_node('fclmc_test', 'fclmc_s', 5432, + 'postgres', 'fclmc_s', 1); + +SELECT nodeid AS ns FROM pgautofailover.node + WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_s' \gset + +-- bootstrap: p = primary, s = secondary +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :np, 0, + current_group_role => 'single'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :ns, 0, + current_group_role => 'wait_standby'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :np, 0, + current_group_role => 'single', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :np, 0, + current_group_role => 'wait_primary', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :ns, 0, + current_group_role => 'wait_standby'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :ns, 0, + current_group_role => 'catchingup', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :ns, 0, + current_group_role => 'secondary', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :np, 0, + current_group_role => 'wait_primary', + current_lsn => '0/5000'); +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :np, 0, + current_group_role => 'primary', + current_lsn => '0/5000'); + +SELECT nodename, reportedstate, goalstate + FROM pgautofailover.node + WHERE formationid = 'fclmc_test' + ORDER BY nodename; + +-- Manufacture the primary mid-transition toward its own goal: goalstate is +-- already 'primary' (assigned by the monitor, e.g. after a replication +-- property change), but the keeper has only reported back as far as +-- 'apply_settings' -- a real, in-progress, healthy predecessor of primary +-- (apply_settings -> primary is a genuine KeeperFSM[] edge), not a dead +-- node. Recency of the last report (from the node_active() calls just +-- above) keeps it "healthy" per NodeIsUnhealthy()'s reportTime check. +UPDATE pgautofailover.node + SET reportedstate = 'apply_settings', + goalstate = 'primary' + WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_p'; + +-- Manufacture the candidate fully converged at prepare_promotion, same +-- technique as Scenario A/B. +UPDATE pgautofailover.node + SET reportedstate = 'prepare_promotion', + goalstate = 'prepare_promotion' + WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_s'; + +SELECT assigned_group_state + FROM pgautofailover.node_active('fclmc_test', :ns, 0, + current_group_role => 'prepare_promotion', + current_lsn => '0/5000'); + +-- ASSERT: fclmc_s's goalstate is still 'prepare_promotion' (deferred, no +-- action taken this tick), and fclmc_p's goalstate is untouched at +-- 'primary' -- neither Rule 2 branch may fire while the primary is healthy +-- and still converging toward its own goal. +-- Before the "settled" guard: this would have incorrectly matched the +-- direct-promotion fallback (reportedState 'apply_settings' is not in the +-- demote_timeout-reachable set), promoting fclmc_s to wait_primary right +-- as fclmc_p was legitimately about to reach primary on its own -- two +-- simultaneously-writable nodes. +SELECT nodename, reportedstate, goalstate + FROM pgautofailover.node + WHERE formationid = 'fclmc_test' + ORDER BY nodename; From abacf0fac2cb081708e47f23f144202f20657fb4 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Mon, 27 Jul 2026 15:04:09 +0200 Subject: [PATCH 4/6] Revert to Fix 1 only: the "unreachable demote_timeout" restriction and everything built on it were solving a self-inflicted regression, not the real #774 bug Built and ran the exact CI reproduction against a fresh, completely unmodified origin/main worktree: all 27 multi-async tests pass, including the "primary dies at wait_primary" scenario this whole series of fixes was chasing (test_014_003_restart_node1 takes ~35s via the pre-existing drain-timeout escape hatch, then test_014_004 resolves cleanly with no split-brain). Root cause of the confusion: main already has a working, purely-time-based mechanism for this exact case (NodeIsDrainTimeExpired + the "demote timeout expired" rule) that reassigns a stuck-at-wait_primary primary to demoted -- a state genuinely reachable from wait_primary in KeeperFSM[] (added in #707, before #774 was even filed). The second commit in this branch ("Fix #774: monitor can assign a primary an unreachable goal state") restricted the demote_timeout-assigning rule to require the primary's reportedState be in a specific reachable set. That inadvertently prevented the primary's goal from ever becoming demote_timeout in the wait_primary case at all -- which is the precondition NodeIsDrainTimeExpired checks (node->goalState != DEMOTE_TIMEOUT_STATE => false). That broke the escape hatch's trigger, producing the "candidate hangs at prepare_promotion forever" bug the next two commits in this branch were built to fix -- via a new fallback path that promotes the candidate directly but never reconciles the old primary's own row, producing an actual split-brain (two simultaneously-writable nodes) that does not exist on main. Re-reading the original #774 issue thread confirms the actual reported bug is a *race*, not a permanent unreachable-state hang: a manual perform_failover can have the "no healthy standby in the quorum" rule (Rule 1) reassign the primary to wait_primary in the same window a separate rule assigns it demote_timeout, exactly matching the issue reporter's own diagnosis ("the monitor should not continue with the usual failover transition" when it just reassigned wait_primary). Fix 1 (IsFailoverInProgress guard on Rule 1, kept) directly prevents this race. The keeper's FATAL-and-retry behavior for a genuinely unreachable assignment is not itself a crash (log_fatal is a severity label only, per src/bin/lib/log/src/log.h -- the keeper loops and retries harmlessly) and should stay as-is: the monitor must never hand out an unreachable transition, full stop, and the keeper is right to refuse loudly if it ever does. Reverted group_state_machine.c's prepare_promotion handling back to origin/main's version, then reapplied only Fix 1 (the IsFailoverInProgress guard in ProceedGroupStateForPrimaryNode) -- diffed against origin/main to confirm this is now the only change. Trimmed failover_candidate_leaves_secondary.sql back to Scenario A (Fix 1's test) only; removed Scenarios B/C, which tested the now-reverted code. Verified via: - `make installcheck` (pg_regress in Docker): all 13 monitor tests + isolation tests pass. - citus_indent (docker run citus/stylechecker:no-py): clean. - Full multi-async pgaftest schedule against this exact code (own local image tags): all 27 tests pass, matching origin/main's baseline behavior exactly (including the ~35s drain-timeout path for test_014_003, and a clean test_014_004 with no split-brain). --- .../failover_candidate_leaves_secondary.out | 377 ++---------------- src/monitor/group_state_machine.c | 126 ++---- .../failover_candidate_leaves_secondary.sql | 267 ++----------- 3 files changed, 101 insertions(+), 669 deletions(-) diff --git a/src/monitor/expected/failover_candidate_leaves_secondary.out b/src/monitor/expected/failover_candidate_leaves_secondary.out index 91138fd73..d69cf152d 100644 --- a/src/monitor/expected/failover_candidate_leaves_secondary.out +++ b/src/monitor/expected/failover_candidate_leaves_secondary.out @@ -3,40 +3,41 @@ -- -- Regression test for issue #774: a monitor rule can assign a node a goal -- state its own KeeperFSM[] (src/bin/pg_autoctl/fsm.c) has no transition --- path to reach from that node's current state, fataling the keeper --- forever ("does not know how to reach state ... from ..."). +-- path to reach from that node's current state. -- --- Two independent rules in group_state_machine.c interact badly: --- --- Rule 1 (ProceedGroupStateForPrimaryNode, "no healthy standby in the --- quorum" block): reassigns the primary to wait_primary whenever --- secondaryQuorumNodesCount == 0. That count drops to zero the instant a --- failover candidate leaves SECONDARY (e.g. converges to --- prepare_promotion) -- even though the candidate *is* the failover in --- progress, not a lost standby. Without a guard, the primary can be --- reassigned wait_primary right as its own candidate is converging. --- --- Rule 2 ("prepare_promotion -> stop_replication" block): assigns the --- primary demote_timeout as soon as the candidate reports --- prepare_promotion, unconditionally. demote_timeout has a real --- KeeperFSM[] edge from primary/join_primary/apply_settings/draining -- --- but not from wait_primary. If Rule 1 already reassigned the primary to --- wait_primary, Rule 2 hands out an unreachable demote_timeout, and the --- keeper fatals on every retry. +-- Root cause: two independent rules in group_state_machine.c could race. +-- Rule 1 (ProceedGroupStateForPrimaryNode, "no healthy standby in the +-- quorum" block) reassigns the primary to wait_primary whenever +-- secondaryQuorumNodesCount == 0. That count drops to zero the instant a +-- failover candidate leaves SECONDARY (e.g. converges to prepare_promotion) +-- -- even though the candidate *is* the failover in progress, not a lost +-- standby. Without a guard, the primary could be reassigned wait_primary +-- right as its own candidate is converging, landing it on wait_primary +-- right when a later rule still expects to find it in draining and assigns +-- demote_timeout -- an assignment with no KeeperFSM[] edge from +-- wait_primary, which the keeper cannot execute. -- -- Fixed by: Rule 1 now skips its reassignment when IsFailoverInProgress() --- is true (the candidate already being in prepare_promotion counts); --- Rule 2 now only assigns demote_timeout when the primary's reported --- state is actually one KeeperFSM[] has an edge to, deferring otherwise. +-- is true (the candidate already being in prepare_promotion counts). This +-- prevents the race from happening in the first place, matching the root +-- cause described by the original issue reporter. -- --- Two scenarios below, each isolating one of the two guards. Neither --- needs the isolation tester (pg_isolation_regress): both are pure --- sequencing issues inside ProceedGroupStateFromContext, reproducible --- deterministically with a straight-line sequence of node_active() calls --- and (for the states that are otherwise hard to reach synchronously) a --- direct UPDATE to manufacture the precondition -- the same technique --- stale_primary_report.sql already uses for a similarly hard-to-reach --- scenario (a primary that never gets to report its own demotion). +-- Note: if a live primary is nonetheless ever handed an unreachable goal +-- state by some other path, the keeper is expected to loudly refuse (fatal) +-- rather than silently proceed -- that is a deliberate invariant, not a bug +-- to be softened. The monitor's own drain-timeout mechanism +-- (NodeIsDrainTimeExpired, purely time-based) already reconciles a primary +-- stuck unresponsive at wait_primary by reassigning it to demoted (a state +-- genuinely reachable from wait_primary), independently of this fix. +-- +-- This scenario isolates Rule 1's guard. It doesn't need the isolation +-- tester (pg_isolation_regress): it's a pure sequencing issue inside +-- ProceedGroupStateFromContext, reproducible deterministically with a +-- straight-line sequence of node_active() calls and a direct UPDATE to +-- manufacture the otherwise hard-to-reach precondition -- the same +-- technique stale_primary_report.sql already uses for a similarly +-- hard-to-reach scenario (a primary that never gets to report its own +-- demotion). \x on -- ═══════════════════════════════════════════════════════════════════════ -- Scenario A (Rule 1 / Fix 1): a candidate already mid-promotion must not @@ -181,319 +182,3 @@ nodename | fclma_s reportedstate | prepare_promotion goalstate | prepare_promotion --- ═══════════════════════════════════════════════════════════════════════ --- Scenario B (Rule 2 / Fix 2): demote_timeout must never be assigned to a --- primary sitting at wait_primary -- there is no KeeperFSM[] edge from --- wait_primary to demote_timeout, so that assignment fatals the keeper --- forever. --- ═══════════════════════════════════════════════════════════════════════ -SELECT pgautofailover.create_formation('fclmb_test', 'pgsql', 'postgres', true, 0); --[ RECORD 1 ]----+-------------------------------- -create_formation | (fclmb_test,pgsql,postgres,t,0) - -SELECT * - FROM pgautofailover.register_node('fclmb_test', 'fclmb_p', 5432, - 'postgres', 'fclmb_p', 1); --[ RECORD 1 ]---------------+-------- -assigned_node_id | 26 -assigned_group_id | 0 -assigned_group_state | single -assigned_candidate_priority | 100 -assigned_replication_quorum | t -assigned_node_name | fclmb_p - -SELECT nodeid AS np FROM pgautofailover.node - WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_p' \gset -SELECT * - FROM pgautofailover.register_node('fclmb_test', 'fclmb_s', 5432, - 'postgres', 'fclmb_s', 1); --[ RECORD 1 ]---------------+------------- -assigned_node_id | 27 -assigned_group_id | 0 -assigned_group_state | wait_standby -assigned_candidate_priority | 100 -assigned_replication_quorum | t -assigned_node_name | fclmb_s - -SELECT nodeid AS ns FROM pgautofailover.node - WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_s' \gset --- bootstrap: p = primary, s = secondary -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :np, 0, - current_group_role => 'single'); --[ RECORD 1 ]--------+------- -assigned_group_state | single - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :ns, 0, - current_group_role => 'wait_standby'); --[ RECORD 1 ]--------+------------- -assigned_group_state | wait_standby - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :np, 0, - current_group_role => 'single', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+------------- -assigned_group_state | wait_primary - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :np, 0, - current_group_role => 'wait_primary', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+------------- -assigned_group_state | wait_primary - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :ns, 0, - current_group_role => 'wait_standby'); --[ RECORD 1 ]--------+----------- -assigned_group_state | catchingup - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :ns, 0, - current_group_role => 'catchingup', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+---------- -assigned_group_state | secondary - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :ns, 0, - current_group_role => 'secondary', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+---------- -assigned_group_state | secondary - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :np, 0, - current_group_role => 'wait_primary', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+-------- -assigned_group_state | primary - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :np, 0, - current_group_role => 'primary', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+-------- -assigned_group_state | primary - -SELECT nodename, reportedstate, goalstate - FROM pgautofailover.node - WHERE formationid = 'fclmb_test' - ORDER BY nodename; --[ RECORD 1 ]-+---------- -nodename | fclmb_p -reportedstate | primary -goalstate | primary --[ RECORD 2 ]-+---------- -nodename | fclmb_s -reportedstate | secondary -goalstate | secondary - --- Manufacture the state Rule 1 would have produced without Fix 1 above -- --- the primary fully converged at wait_primary, whatever the exact prior --- trigger. This isolates Rule 2's behaviour from Rule 1's. -UPDATE pgautofailover.node - SET reportedstate = 'wait_primary', - goalstate = 'wait_primary' - WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_p'; --- The candidate is already fully converged at prepare_promotion, same as in --- Scenario A -- IsCurrentState() requires goalstate == reportedstate == the --- state being tested, so a node_active() call alone would only move --- reportedstate and leave goalstate at 'secondary' from the bootstrap above, --- never matching Rule 2's IsCurrentState(activeNode, PREPARE_PROMOTION) --- check. Manufacture full convergence directly, then re-affirm via --- node_active() to trigger the dispatch. -UPDATE pgautofailover.node - SET reportedstate = 'prepare_promotion', - goalstate = 'prepare_promotion' - WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_s'; -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :ns, 0, - current_group_role => 'prepare_promotion', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+------------- -assigned_group_state | wait_primary - --- ASSERT: fclmb_p's goalstate is still 'wait_primary', not 'demote_timeout', --- and fclmb_s is promoted directly to 'wait_primary' -- fclmb_p is stuck at --- wait_primary with no path to become a live primary, so it is treated the --- same as "no primary at all" (the primaryNode == NULL case) rather than --- waiting forever for a demote handshake that can never happen. --- Pre-fix: Rule 2 assigns demote_timeout unconditionally here, producing --- reportedstate=wait_primary / goalstate=demote_timeout -- no KeeperFSM[] --- edge between them, so the keeper would fatal on every retry. -SELECT nodename, reportedstate, goalstate - FROM pgautofailover.node - WHERE formationid = 'fclmb_test' - ORDER BY nodename; --[ RECORD 1 ]-+------------------ -nodename | fclmb_p -reportedstate | wait_primary -goalstate | wait_primary --[ RECORD 2 ]-+------------------ -nodename | fclmb_s -reportedstate | prepare_promotion -goalstate | wait_primary - --- ═══════════════════════════════════════════════════════════════════════ --- Scenario C (the "settled" guard): neither Rule 2's demote_timeout path nor --- its wait_primary fallback may fire while the primary is healthy but has --- not yet converged to its own assigned goal -- it might still complete --- that transition (e.g. reach a real primary) on its own, and deciding --- based on a reportedState that's just lagging behind an in-progress, --- healthy convergence risks promoting a second candidate while the first --- primary is still legitimately alive and progressing, producing two --- simultaneously-writable nodes. --- ═══════════════════════════════════════════════════════════════════════ -SELECT pgautofailover.create_formation('fclmc_test', 'pgsql', 'postgres', true, 0); --[ RECORD 1 ]----+-------------------------------- -create_formation | (fclmc_test,pgsql,postgres,t,0) - -SELECT * - FROM pgautofailover.register_node('fclmc_test', 'fclmc_p', 5432, - 'postgres', 'fclmc_p', 1); --[ RECORD 1 ]---------------+-------- -assigned_node_id | 28 -assigned_group_id | 0 -assigned_group_state | single -assigned_candidate_priority | 100 -assigned_replication_quorum | t -assigned_node_name | fclmc_p - -SELECT nodeid AS np FROM pgautofailover.node - WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_p' \gset -SELECT * - FROM pgautofailover.register_node('fclmc_test', 'fclmc_s', 5432, - 'postgres', 'fclmc_s', 1); --[ RECORD 1 ]---------------+------------- -assigned_node_id | 29 -assigned_group_id | 0 -assigned_group_state | wait_standby -assigned_candidate_priority | 100 -assigned_replication_quorum | t -assigned_node_name | fclmc_s - -SELECT nodeid AS ns FROM pgautofailover.node - WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_s' \gset --- bootstrap: p = primary, s = secondary -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :np, 0, - current_group_role => 'single'); --[ RECORD 1 ]--------+------- -assigned_group_state | single - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :ns, 0, - current_group_role => 'wait_standby'); --[ RECORD 1 ]--------+------------- -assigned_group_state | wait_standby - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :np, 0, - current_group_role => 'single', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+------------- -assigned_group_state | wait_primary - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :np, 0, - current_group_role => 'wait_primary', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+------------- -assigned_group_state | wait_primary - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :ns, 0, - current_group_role => 'wait_standby'); --[ RECORD 1 ]--------+----------- -assigned_group_state | catchingup - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :ns, 0, - current_group_role => 'catchingup', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+---------- -assigned_group_state | secondary - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :ns, 0, - current_group_role => 'secondary', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+---------- -assigned_group_state | secondary - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :np, 0, - current_group_role => 'wait_primary', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+-------- -assigned_group_state | primary - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :np, 0, - current_group_role => 'primary', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+-------- -assigned_group_state | primary - -SELECT nodename, reportedstate, goalstate - FROM pgautofailover.node - WHERE formationid = 'fclmc_test' - ORDER BY nodename; --[ RECORD 1 ]-+---------- -nodename | fclmc_p -reportedstate | primary -goalstate | primary --[ RECORD 2 ]-+---------- -nodename | fclmc_s -reportedstate | secondary -goalstate | secondary - --- Manufacture the primary mid-transition toward its own goal: goalstate is --- already 'primary' (assigned by the monitor, e.g. after a replication --- property change), but the keeper has only reported back as far as --- 'apply_settings' -- a real, in-progress, healthy predecessor of primary --- (apply_settings -> primary is a genuine KeeperFSM[] edge), not a dead --- node. Recency of the last report (from the node_active() calls just --- above) keeps it "healthy" per NodeIsUnhealthy()'s reportTime check. -UPDATE pgautofailover.node - SET reportedstate = 'apply_settings', - goalstate = 'primary' - WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_p'; --- Manufacture the candidate fully converged at prepare_promotion, same --- technique as Scenario A/B. -UPDATE pgautofailover.node - SET reportedstate = 'prepare_promotion', - goalstate = 'prepare_promotion' - WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_s'; -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :ns, 0, - current_group_role => 'prepare_promotion', - current_lsn => '0/5000'); --[ RECORD 1 ]--------+------------------ -assigned_group_state | prepare_promotion - --- ASSERT: fclmc_s's goalstate is still 'prepare_promotion' (deferred, no --- action taken this tick), and fclmc_p's goalstate is untouched at --- 'primary' -- neither Rule 2 branch may fire while the primary is healthy --- and still converging toward its own goal. --- Before the "settled" guard: this would have incorrectly matched the --- direct-promotion fallback (reportedState 'apply_settings' is not in the --- demote_timeout-reachable set), promoting fclmc_s to wait_primary right --- as fclmc_p was legitimately about to reach primary on its own -- two --- simultaneously-writable nodes. -SELECT nodename, reportedstate, goalstate - FROM pgautofailover.node - WHERE formationid = 'fclmc_test' - ORDER BY nodename; --[ RECORD 1 ]-+------------------ -nodename | fclmc_p -reportedstate | apply_settings -goalstate | primary --[ RECORD 2 ]-+------------------ -nodename | fclmc_s -reportedstate | prepare_promotion -goalstate | prepare_promotion - diff --git a/src/monitor/group_state_machine.c b/src/monitor/group_state_machine.c index a647ad4c7..42a7125e5 100644 --- a/src/monitor/group_state_machine.c +++ b/src/monitor/group_state_machine.c @@ -843,105 +843,57 @@ ProceedGroupStateFromContext(GroupStateContext *ctx) /* * when node is seeing no more writes: * prepare_promotion -> stop_replication - * (or, when there's no live primary to hand off to: -> wait_primary) * - * Both of the two rules below act on primaryNode's reported state to - * decide what to do with it, so neither may fire until primaryNode is - * "settled": either it has fully converged to its own currently assigned - * goal (reportedState == goalState), or it's confirmed unhealthy (not - * responding, so it cannot make further progress toward that goal right - * now regardless). If primaryNode is healthy but still mid-transition - * toward its own goal, defer instead: it might complete that transition - * (e.g. actually reach primary) on its own before we ever need to hand - * off to a different candidate, and deciding based on a reportedState - * that's just lagging behind an in-progress, healthy convergence risks - * racing ahead of it -- producing two simultaneously-writable nodes, - * which is strictly worse than the unreachable-goal-state bug (#774) - * these two rules exist to prevent in the first place. + * refrain from prepare_maintenance -> demote_timeout on the primary, which + * might happen here when secondary has reached prepare_promotion before + * primary has reached prepare_maintenance. */ if (IsCurrentState(activeNode, REPLICATION_STATE_PREPARE_PROMOTION) && - (primaryNode == NULL || - (!IsInMaintenance(primaryNode) && - (IsCurrentState(primaryNode, primaryNode->goalState) || - NodeIsUnhealthy(primaryNode, ctx))))) + primaryNode && + !IsInMaintenance(primaryNode)) { - /* - * refrain from prepare_maintenance -> demote_timeout on the primary - * (excluded via IsInMaintenance above), which might happen here when - * secondary has reached prepare_promotion before primary has reached - * prepare_maintenance -- deferring entirely rather than falling - * through to the direct-promotion branch below, since a primary - * that's deliberately in maintenance is not "gone", just paused. - * - * Also require the primary to actually be in one of the states - * demote_timeout has a real KeeperFSM[] edge from (primary, - * join_primary, apply_settings, or draining -- verified against - * fsm.c) before assigning it: this rule fires as soon as the - * candidate reports prepare_promotion, which routinely happens - * while the primary is still reporting "primary" itself (it hasn't - * locally converged to draining yet) -- that's the normal case and - * demote_timeout is reachable from primary too, so it's allowed. - * What's not reachable is wait_primary: a separate rule can - * reassign the primary to wait_primary in the same window (issue - * #774), and wait_primary has no FSM edge to demote_timeout at all, - * which fatals the node forever. - */ - if (primaryNode != NULL && - (primaryNode->reportedState == REPLICATION_STATE_PRIMARY || - primaryNode->reportedState == REPLICATION_STATE_JOIN_PRIMARY || - primaryNode->reportedState == REPLICATION_STATE_APPLY_SETTINGS || - primaryNode->reportedState == REPLICATION_STATE_DRAINING)) - { - char message[BUFSIZE]; + char message[BUFSIZE]; - LogAndNotifyMessage( - message, BUFSIZE, - "Setting goal state of " NODE_FORMAT - " to demote_timeout and " NODE_FORMAT - " to stop_replication after " NODE_FORMAT - " converged to prepare_promotion.", - NODE_FORMAT_ARGS(primaryNode), - NODE_FORMAT_ARGS(activeNode), - NODE_FORMAT_ARGS(activeNode)); + LogAndNotifyMessage( + message, BUFSIZE, + "Setting goal state of " NODE_FORMAT + " to demote_timeout and " NODE_FORMAT + " to stop_replication after " NODE_FORMAT + " converged to prepare_promotion.", + NODE_FORMAT_ARGS(primaryNode), + NODE_FORMAT_ARGS(activeNode), + NODE_FORMAT_ARGS(activeNode)); - /* perform promotion to stop replication */ - AssignGoalState(activeNode, REPLICATION_STATE_STOP_REPLICATION, - message); + /* perform promotion to stop replication */ + AssignGoalState(activeNode, REPLICATION_STATE_STOP_REPLICATION, message); - /* wait for possibly-alive primary to kill itself */ - AssignGoalState(primaryNode, REPLICATION_STATE_DEMOTE_TIMEOUT, - message); + /* wait for possibly-alive primary to kill itself */ + AssignGoalState(primaryNode, REPLICATION_STATE_DEMOTE_TIMEOUT, message); - return true; - } + return true; + } - /* - * primaryNode is either NULL (removed from the group entirely), or - * settled (per the outer guard above) in a state with no real - * demote_timeout edge -- e.g. a primary candidate that reached - * wait_primary (a writable goal state, so it's still found by - * GetPrimaryOrDemotedNodeInGroupFromList) but died before ever - * getting a healthy secondary counted and being promoted the rest - * of the way to primary. Waiting for the block above to fire would - * wait forever (issue #774): there is no live primary to demote, so - * just promote the candidate directly, exactly as when primaryNode - * is NULL. - */ - { - char message[BUFSIZE] = { 0 }; + /* + * when primary node has been removed and we are promoting one standby + * prepare_promotion -> stop_replication + */ + if (IsCurrentState(activeNode, REPLICATION_STATE_PREPARE_PROMOTION) && + primaryNode == NULL) + { + char message[BUFSIZE] = { 0 }; - LogAndNotifyMessage( - message, BUFSIZE, - "Setting goal state of " NODE_FORMAT - " to wait_primary after " NODE_FORMAT - " converged to prepare_promotion.", - NODE_FORMAT_ARGS(activeNode), - NODE_FORMAT_ARGS(activeNode)); + LogAndNotifyMessage( + message, BUFSIZE, + "Setting goal state of " NODE_FORMAT + " to wait_primary after " NODE_FORMAT + " converged to prepare_promotion.", + NODE_FORMAT_ARGS(activeNode), + NODE_FORMAT_ARGS(activeNode)); - AssignGoalState(activeNode, REPLICATION_STATE_WAIT_PRIMARY, message); + /* perform promotion to stop replication */ + AssignGoalState(activeNode, REPLICATION_STATE_WAIT_PRIMARY, message); - return true; - } + return true; } /* diff --git a/src/monitor/sql/failover_candidate_leaves_secondary.sql b/src/monitor/sql/failover_candidate_leaves_secondary.sql index 4bea89833..8a587e4cd 100644 --- a/src/monitor/sql/failover_candidate_leaves_secondary.sql +++ b/src/monitor/sql/failover_candidate_leaves_secondary.sql @@ -3,40 +3,41 @@ -- -- Regression test for issue #774: a monitor rule can assign a node a goal -- state its own KeeperFSM[] (src/bin/pg_autoctl/fsm.c) has no transition --- path to reach from that node's current state, fataling the keeper --- forever ("does not know how to reach state ... from ..."). +-- path to reach from that node's current state. -- --- Two independent rules in group_state_machine.c interact badly: --- --- Rule 1 (ProceedGroupStateForPrimaryNode, "no healthy standby in the --- quorum" block): reassigns the primary to wait_primary whenever --- secondaryQuorumNodesCount == 0. That count drops to zero the instant a --- failover candidate leaves SECONDARY (e.g. converges to --- prepare_promotion) -- even though the candidate *is* the failover in --- progress, not a lost standby. Without a guard, the primary can be --- reassigned wait_primary right as its own candidate is converging. --- --- Rule 2 ("prepare_promotion -> stop_replication" block): assigns the --- primary demote_timeout as soon as the candidate reports --- prepare_promotion, unconditionally. demote_timeout has a real --- KeeperFSM[] edge from primary/join_primary/apply_settings/draining -- --- but not from wait_primary. If Rule 1 already reassigned the primary to --- wait_primary, Rule 2 hands out an unreachable demote_timeout, and the --- keeper fatals on every retry. +-- Root cause: two independent rules in group_state_machine.c could race. +-- Rule 1 (ProceedGroupStateForPrimaryNode, "no healthy standby in the +-- quorum" block) reassigns the primary to wait_primary whenever +-- secondaryQuorumNodesCount == 0. That count drops to zero the instant a +-- failover candidate leaves SECONDARY (e.g. converges to prepare_promotion) +-- -- even though the candidate *is* the failover in progress, not a lost +-- standby. Without a guard, the primary could be reassigned wait_primary +-- right as its own candidate is converging, landing it on wait_primary +-- right when a later rule still expects to find it in draining and assigns +-- demote_timeout -- an assignment with no KeeperFSM[] edge from +-- wait_primary, which the keeper cannot execute. -- -- Fixed by: Rule 1 now skips its reassignment when IsFailoverInProgress() --- is true (the candidate already being in prepare_promotion counts); --- Rule 2 now only assigns demote_timeout when the primary's reported --- state is actually one KeeperFSM[] has an edge to, deferring otherwise. +-- is true (the candidate already being in prepare_promotion counts). This +-- prevents the race from happening in the first place, matching the root +-- cause described by the original issue reporter. -- --- Two scenarios below, each isolating one of the two guards. Neither --- needs the isolation tester (pg_isolation_regress): both are pure --- sequencing issues inside ProceedGroupStateFromContext, reproducible --- deterministically with a straight-line sequence of node_active() calls --- and (for the states that are otherwise hard to reach synchronously) a --- direct UPDATE to manufacture the precondition -- the same technique --- stale_primary_report.sql already uses for a similarly hard-to-reach --- scenario (a primary that never gets to report its own demotion). +-- Note: if a live primary is nonetheless ever handed an unreachable goal +-- state by some other path, the keeper is expected to loudly refuse (fatal) +-- rather than silently proceed -- that is a deliberate invariant, not a bug +-- to be softened. The monitor's own drain-timeout mechanism +-- (NodeIsDrainTimeExpired, purely time-based) already reconciles a primary +-- stuck unresponsive at wait_primary by reassigning it to demoted (a state +-- genuinely reachable from wait_primary), independently of this fix. +-- +-- This scenario isolates Rule 1's guard. It doesn't need the isolation +-- tester (pg_isolation_regress): it's a pure sequencing issue inside +-- ProceedGroupStateFromContext, reproducible deterministically with a +-- straight-line sequence of node_active() calls and a direct UPDATE to +-- manufacture the otherwise hard-to-reach precondition -- the same +-- technique stale_primary_report.sql already uses for a similarly +-- hard-to-reach scenario (a primary that never gets to report its own +-- demotion). \x on @@ -126,209 +127,3 @@ SELECT nodename, reportedstate, goalstate FROM pgautofailover.node WHERE formationid = 'fclma_test' ORDER BY nodename; - --- ═══════════════════════════════════════════════════════════════════════ --- Scenario B (Rule 2 / Fix 2): demote_timeout must never be assigned to a --- primary sitting at wait_primary -- there is no KeeperFSM[] edge from --- wait_primary to demote_timeout, so that assignment fatals the keeper --- forever. --- ═══════════════════════════════════════════════════════════════════════ - -SELECT pgautofailover.create_formation('fclmb_test', 'pgsql', 'postgres', true, 0); - -SELECT * - FROM pgautofailover.register_node('fclmb_test', 'fclmb_p', 5432, - 'postgres', 'fclmb_p', 1); - -SELECT nodeid AS np FROM pgautofailover.node - WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_p' \gset - -SELECT * - FROM pgautofailover.register_node('fclmb_test', 'fclmb_s', 5432, - 'postgres', 'fclmb_s', 1); - -SELECT nodeid AS ns FROM pgautofailover.node - WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_s' \gset - --- bootstrap: p = primary, s = secondary -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :np, 0, - current_group_role => 'single'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :ns, 0, - current_group_role => 'wait_standby'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :np, 0, - current_group_role => 'single', - current_lsn => '0/5000'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :np, 0, - current_group_role => 'wait_primary', - current_lsn => '0/5000'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :ns, 0, - current_group_role => 'wait_standby'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :ns, 0, - current_group_role => 'catchingup', - current_lsn => '0/5000'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :ns, 0, - current_group_role => 'secondary', - current_lsn => '0/5000'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :np, 0, - current_group_role => 'wait_primary', - current_lsn => '0/5000'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :np, 0, - current_group_role => 'primary', - current_lsn => '0/5000'); - -SELECT nodename, reportedstate, goalstate - FROM pgautofailover.node - WHERE formationid = 'fclmb_test' - ORDER BY nodename; - --- Manufacture the state Rule 1 would have produced without Fix 1 above -- --- the primary fully converged at wait_primary, whatever the exact prior --- trigger. This isolates Rule 2's behaviour from Rule 1's. -UPDATE pgautofailover.node - SET reportedstate = 'wait_primary', - goalstate = 'wait_primary' - WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_p'; - --- The candidate is already fully converged at prepare_promotion, same as in --- Scenario A -- IsCurrentState() requires goalstate == reportedstate == the --- state being tested, so a node_active() call alone would only move --- reportedstate and leave goalstate at 'secondary' from the bootstrap above, --- never matching Rule 2's IsCurrentState(activeNode, PREPARE_PROMOTION) --- check. Manufacture full convergence directly, then re-affirm via --- node_active() to trigger the dispatch. -UPDATE pgautofailover.node - SET reportedstate = 'prepare_promotion', - goalstate = 'prepare_promotion' - WHERE formationid = 'fclmb_test' AND nodename = 'fclmb_s'; - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmb_test', :ns, 0, - current_group_role => 'prepare_promotion', - current_lsn => '0/5000'); - --- ASSERT: fclmb_p's goalstate is still 'wait_primary', not 'demote_timeout', --- and fclmb_s is promoted directly to 'wait_primary' -- fclmb_p is stuck at --- wait_primary with no path to become a live primary, so it is treated the --- same as "no primary at all" (the primaryNode == NULL case) rather than --- waiting forever for a demote handshake that can never happen. --- Pre-fix: Rule 2 assigns demote_timeout unconditionally here, producing --- reportedstate=wait_primary / goalstate=demote_timeout -- no KeeperFSM[] --- edge between them, so the keeper would fatal on every retry. -SELECT nodename, reportedstate, goalstate - FROM pgautofailover.node - WHERE formationid = 'fclmb_test' - ORDER BY nodename; - --- ═══════════════════════════════════════════════════════════════════════ --- Scenario C (the "settled" guard): neither Rule 2's demote_timeout path nor --- its wait_primary fallback may fire while the primary is healthy but has --- not yet converged to its own assigned goal -- it might still complete --- that transition (e.g. reach a real primary) on its own, and deciding --- based on a reportedState that's just lagging behind an in-progress, --- healthy convergence risks promoting a second candidate while the first --- primary is still legitimately alive and progressing, producing two --- simultaneously-writable nodes. --- ═══════════════════════════════════════════════════════════════════════ - -SELECT pgautofailover.create_formation('fclmc_test', 'pgsql', 'postgres', true, 0); - -SELECT * - FROM pgautofailover.register_node('fclmc_test', 'fclmc_p', 5432, - 'postgres', 'fclmc_p', 1); - -SELECT nodeid AS np FROM pgautofailover.node - WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_p' \gset - -SELECT * - FROM pgautofailover.register_node('fclmc_test', 'fclmc_s', 5432, - 'postgres', 'fclmc_s', 1); - -SELECT nodeid AS ns FROM pgautofailover.node - WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_s' \gset - --- bootstrap: p = primary, s = secondary -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :np, 0, - current_group_role => 'single'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :ns, 0, - current_group_role => 'wait_standby'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :np, 0, - current_group_role => 'single', - current_lsn => '0/5000'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :np, 0, - current_group_role => 'wait_primary', - current_lsn => '0/5000'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :ns, 0, - current_group_role => 'wait_standby'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :ns, 0, - current_group_role => 'catchingup', - current_lsn => '0/5000'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :ns, 0, - current_group_role => 'secondary', - current_lsn => '0/5000'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :np, 0, - current_group_role => 'wait_primary', - current_lsn => '0/5000'); -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :np, 0, - current_group_role => 'primary', - current_lsn => '0/5000'); - -SELECT nodename, reportedstate, goalstate - FROM pgautofailover.node - WHERE formationid = 'fclmc_test' - ORDER BY nodename; - --- Manufacture the primary mid-transition toward its own goal: goalstate is --- already 'primary' (assigned by the monitor, e.g. after a replication --- property change), but the keeper has only reported back as far as --- 'apply_settings' -- a real, in-progress, healthy predecessor of primary --- (apply_settings -> primary is a genuine KeeperFSM[] edge), not a dead --- node. Recency of the last report (from the node_active() calls just --- above) keeps it "healthy" per NodeIsUnhealthy()'s reportTime check. -UPDATE pgautofailover.node - SET reportedstate = 'apply_settings', - goalstate = 'primary' - WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_p'; - --- Manufacture the candidate fully converged at prepare_promotion, same --- technique as Scenario A/B. -UPDATE pgautofailover.node - SET reportedstate = 'prepare_promotion', - goalstate = 'prepare_promotion' - WHERE formationid = 'fclmc_test' AND nodename = 'fclmc_s'; - -SELECT assigned_group_state - FROM pgautofailover.node_active('fclmc_test', :ns, 0, - current_group_role => 'prepare_promotion', - current_lsn => '0/5000'); - --- ASSERT: fclmc_s's goalstate is still 'prepare_promotion' (deferred, no --- action taken this tick), and fclmc_p's goalstate is untouched at --- 'primary' -- neither Rule 2 branch may fire while the primary is healthy --- and still converging toward its own goal. --- Before the "settled" guard: this would have incorrectly matched the --- direct-promotion fallback (reportedState 'apply_settings' is not in the --- demote_timeout-reachable set), promoting fclmc_s to wait_primary right --- as fclmc_p was legitimately about to reach primary on its own -- two --- simultaneously-writable nodes. -SELECT nodename, reportedstate, goalstate - FROM pgautofailover.node - WHERE formationid = 'fclmc_test' - ORDER BY nodename; From 654e88cdf351655f9964d9a499dcf29de5f2dc36 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Mon, 27 Jul 2026 16:00:14 +0200 Subject: [PATCH 5/6] tests: harden transient-connectivity retries for post-restart queries - has_needed_replication_slots(): bump retry_timeout 5s -> 15s, the existing 0.5s-backoff retry loop wasn't always covering a keeper- triggered Postgres restart under a loaded CI runner (test_multi_ifdown.py::test_003_add_standby saw a bare Connection refused). - test_citus_multi_standbys.py: use the existing run_sql_query_retry() helper for worker2c instead of a bare run_sql_query(), which had no retry at all (test_003_002_all_workers_have_data saw the same). --- tests/pgautofailover_utils.py | 2 +- tests/test_citus_multi_standbys.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/pgautofailover_utils.py b/tests/pgautofailover_utils.py index b1a11c185..d95e6b47c 100644 --- a/tests/pgautofailover_utils.py +++ b/tests/pgautofailover_utils.py @@ -1848,7 +1848,7 @@ def list_replication_slot_names(self): self.print_debug_logs() raise e - def has_needed_replication_slots(self, retry_timeout=5): + def has_needed_replication_slots(self, retry_timeout=15): """ Each node is expected to maintain a slot for each of the other nodes the primary through streaming replication, the secondary(s) manually diff --git a/tests/test_citus_multi_standbys.py b/tests/test_citus_multi_standbys.py index db8c53a00..1137ed782 100644 --- a/tests/test_citus_multi_standbys.py +++ b/tests/test_citus_multi_standbys.py @@ -192,7 +192,7 @@ def test_003_002_all_workers_have_data(): results = worker2b.run_sql_query(q1) eq_(results, r2) - results = worker2c.run_sql_query(q1) + results = worker2c.run_sql_query_retry(q1) eq_(results, r2) From ed2de650314ac77c33436b8ed66448e1b76f1d40 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Mon, 27 Jul 2026 18:08:07 +0200 Subject: [PATCH 6/6] Regenerate expected.out for failover_candidate_leaves_secondary after rebase origin/main merged PR #1161 (timeline_fork_detection), which now runs before this test in regress_schedule and consumes 6 more ids from the shared node-id sequence. This test's expected.out hardcoded the literal assigned_node_id values (24/25) from register_node's return row, per the schedule file's own documented caveat about every test needing the whole node-id sequence to itself in a fixed order. Regenerated against the actual node ids now assigned (31/32); no other part of the test changed. Verified: all 14 pg_regress + 6 isolation tests pass locally via 'make installcheck'. --- src/monitor/expected/failover_candidate_leaves_secondary.out | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/monitor/expected/failover_candidate_leaves_secondary.out b/src/monitor/expected/failover_candidate_leaves_secondary.out index d69cf152d..10798ed36 100644 --- a/src/monitor/expected/failover_candidate_leaves_secondary.out +++ b/src/monitor/expected/failover_candidate_leaves_secondary.out @@ -52,7 +52,7 @@ SELECT * FROM pgautofailover.register_node('fclma_test', 'fclma_p', 5432, 'postgres', 'fclma_p', 1); -[ RECORD 1 ]---------------+-------- -assigned_node_id | 24 +assigned_node_id | 31 assigned_group_id | 0 assigned_group_state | single assigned_candidate_priority | 100 @@ -65,7 +65,7 @@ SELECT * FROM pgautofailover.register_node('fclma_test', 'fclma_s', 5432, 'postgres', 'fclma_s', 1); -[ RECORD 1 ]---------------+------------- -assigned_node_id | 25 +assigned_node_id | 32 assigned_group_id | 0 assigned_group_state | wait_standby assigned_candidate_priority | 100