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..10798ed36 --- /dev/null +++ b/src/monitor/expected/failover_candidate_leaves_secondary.out @@ -0,0 +1,184 @@ +-- 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. +-- +-- 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). This +-- prevents the race from happening in the first place, matching the root +-- cause described by the original issue reporter. +-- +-- 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 +-- 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 | 31 +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 | 32 +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 + diff --git a/src/monitor/group_state_machine.c b/src/monitor/group_state_machine.c index 8927c916f..42a7125e5 100644 --- a/src/monitor/group_state_machine.c +++ b/src/monitor/group_state_machine.c @@ -1319,9 +1319,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..8a587e4cd --- /dev/null +++ b/src/monitor/sql/failover_candidate_leaves_secondary.sql @@ -0,0 +1,129 @@ +-- 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. +-- +-- 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). This +-- prevents the race from happening in the first place, matching the root +-- cause described by the original issue reporter. +-- +-- 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 +-- 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; 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)