Fix residual synchronized_standby_slots race during primary promotion

src/monitor/expected/check_fsm_reachability.out

@@ -18,7 +18,7 @@
 SELECT count(*) AS total_edge_count FROM pgautofailover.dump_fsm_edges();
  total_edge_count 
 ------------------
-              178
+              179
 (1 row)
 
 -- pos 301 ("converged secondary, reportedTLI not an ancestor of reference
@@ -72,7 +72,7 @@ SELECT count(*) AS missing_with_empty_keeper_edges
   FROM pgautofailover.check_fsm_reachability('[]'::jsonb);
  missing_with_empty_keeper_edges 
 ---------------------------------
-                             178
+                             179
 (1 row)
 
 -- Providing exactly pos 301's own edge, plus one of pos 343's two edges

src/monitor/expected/fsm.out

@@ -385,7 +385,7 @@ pos                          | 302
 section                      | reporting_node
 section_path                 | reporting_node.from_context
 active_node_current_state    | secondary
-other_node_current_state     | primary
+other_node_current_state     | primary, wait_primary
 candidate_node_current_state | 
 active_node_conditions       | candidateEligible=true, previouslyReportedState=catchingup
 other_node_conditions        | 
@@ -394,7 +394,7 @@ group_conditions             |
 active_node_assigned_state   | 
 other_node_assigned_state    | apply_settings
 has_extra_action             | f
-comment                      | catchingup -> secondary, primary converged primary -> apply_settings
+comment                      | catchingup -> secondary, primary converged primary or mid-promotion to it -> apply_settings
 -[ RECORD 26 ]---------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 pos                          | 303
 section                      | reporting_node

src/monitor/expected/keeper_fsm_edges.out

@@ -43,7 +43,14 @@ SELECT DISTINCT
        END AS current_state,
        (edge ->> 'assigned')::pgautofailover.replication_state AS assigned_state
   FROM jsonb_array_elements(:'keeper_json'::jsonb) AS edge;
-SELECT * FROM keeper_fsm_edges ORDER BY current_state, assigned_state;
+-- current_state is plain text (its own comment above explains why: the
+-- 'any' sentinel isn't a legal pgautofailover.replication_state value), so
+-- unlike assigned_state (the enum, sorted by declaration order regardless
+-- of locale) its ORDER BY position depends on the database's collation --
+-- explicit COLLATE "C" pins it to the same byte-wise order this file's own
+-- expected/keeper_fsm_edges.out was committed with, independent of
+-- whatever locale initdb happened to pick on a given machine or in CI.
+SELECT * FROM keeper_fsm_edges ORDER BY current_state COLLATE "C", assigned_state;
     current_state    |   assigned_state    
 ---------------------+---------------------
  any                 | dropped
@@ -177,6 +184,12 @@ SELECT * FROM keeper_fsm_edges ORDER BY current_state, assigned_state;
 --
 -- Expected result: empty. Every MonitorFSM[] rule currently has a matching
 -- KeeperFSM[] row for every current_state it can assign a transition from.
+--
+-- No COLLATE "C" needed on this query's own ORDER BY below, unlike the
+-- first query above and step 2b's below it: e.current_state here is
+-- dump_fsm_edges()'s own pgautofailover.replication_state return column
+-- (the enum, sorted by declaration order), not keeper_fsm_edges' plain-text
+-- column of the same name -- collation-independent already.
 SELECT e.pos AS rule, count(*) AS n, e.current_state, e.assigned_state, f.comment
   FROM pgautofailover.dump_fsm_edges() e
   JOIN pgautofailover.fsm f ON f.pos = e.pos
@@ -215,6 +228,8 @@ SELECT e.pos AS rule, count(*) AS n, e.current_state, e.assigned_state, f.commen
 -- deliberately excludes (see its own comment), so it can never appear
 -- here. Same "investigate before assuming which" caveat as the rest of
 -- this file applies to every other row below.
+-- Same COLLATE "C" reasoning as the first query above: k.current_state is
+-- keeper_fsm_edges' own plain-text column.
 SELECT k.current_state, k.assigned_state
   FROM keeper_fsm_edges k
  WHERE NOT EXISTS (
@@ -223,7 +238,7 @@ SELECT k.current_state, k.assigned_state
           WHERE e.assigned_state = k.assigned_state
             AND (k.current_state = 'any' OR k.current_state = e.current_state::text)
        )
- ORDER BY k.current_state, k.assigned_state;
+ ORDER BY k.current_state COLLATE "C", k.assigned_state;
     current_state    |   assigned_state    
 ---------------------+---------------------
  any                 | dropped
@@ -248,7 +263,6 @@ SELECT k.current_state, k.assigned_state
  secondary           | maintenance
  secondary           | wait_maintenance
  wait_primary        | join_primary
- wait_primary        | apply_settings
-(23 rows)
+(22 rows)
 
 DROP TABLE keeper_fsm_edges;

src/monitor/expected/synchronized_standby_slots_promoting_primary.out

@@ -0,0 +1,228 @@
+-- Copyright (c) Microsoft Corporation. All rights reserved.
+-- Licensed under the PostgreSQL License.
+--
+-- Regression test for the residual race left by the fix in
+-- synchronized_standby_slots.sql / pos 302 (group_state_machine.c):
+-- that fix forces the primary through apply_settings when a standby
+-- finishes catching up *after* the primary has already converged to
+-- steady "primary" -- but a second, narrower window remained open when
+-- a standby instead finishes catching up *while the primary itself is
+-- still mid-promotion* (reportedState=wait_primary, goalState already
+-- primary, i.e. between the monitor assigning goal=primary and the
+-- keeper's own next heartbeat reporting it). fsm_enable_sync_rep()'s own
+-- WAIT_PRIMARY -> PRIMARY keeper transition fetches
+-- synchronized_standby_slots fresh on every retry, driven by LOCAL
+-- pg_stat_replication catch-up rather than the monitor's own
+-- reportedState column, so that fetch can complete off a single
+-- caught-up standby before a second standby's own catchingup -> secondary
+-- transition has landed on the monitor -- and since that transition is a
+-- one-time event per standby, pos 302's original strict
+-- FSM_STATE(PRIMARY) gate (reportedState == goalState == primary) could
+-- never retroactively fire for it afterwards, leaving the primary's own
+-- goalState (and, in the real keeper, its locally-applied
+-- synchronized_standby_slots GUC) stuck on whatever shorter list it had
+-- already fetched.
+--
+-- This test drives that exact interleaving entirely through
+-- pgautofailover.node_active() calls, without relying on timing. Both
+-- standbys are assigned catchingup concurrently, while the primary is
+-- still steady wait_primary (same as an ordinary two-standby startup);
+-- one standby (s1) finishes first, which is what nudges the primary into
+-- promoting -- and the *other* standby (s2) only reports finishing
+-- (catchingup -> secondary) once the primary is already mid-promotion
+-- (reportedState=wait_primary, goalState=primary): the real keeper's own
+-- retry window inside fsm_enable_sync_rep(). The regression check is on
+-- the primary's own goalState immediately after s2's transition: it must
+-- become apply_settings (forcing the next re-fetch), not silently stay
+-- primary.
+\x on
+-- ── formation and node registration ─────────────────────────────────────────
+SELECT pgautofailover.create_formation('sss2_test', 'pgsql', 'postgres', true, 1);
+-[ RECORD 1 ]----+-------------------------------
+create_formation | (sss2_test,pgsql,postgres,t,1)
+
+SELECT assigned_group_state
+  FROM pgautofailover.register_node('sss2_test', 'sss2_p', 5432,
+                                    'postgres', 'sss2_p', 1);
+-[ RECORD 1 ]--------+-------
+assigned_group_state | single
+
+SELECT nodeid AS np FROM pgautofailover.node
+ WHERE formationid = 'sss2_test' AND nodename = 'sss2_p' \gset
+SELECT assigned_group_state
+  FROM pgautofailover.register_node('sss2_test', 'sss2_s1', 5432,
+                                    'postgres', 'sss2_s1', 1);
+-[ RECORD 1 ]--------+-------------
+assigned_group_state | wait_standby
+
+SELECT nodeid AS ns1 FROM pgautofailover.node
+ WHERE formationid = 'sss2_test' AND nodename = 'sss2_s1' \gset
+SELECT assigned_group_state
+  FROM pgautofailover.register_node('sss2_test', 'sss2_s2', 5432,
+                                    'postgres', 'sss2_s2', 1);
+-[ RECORD 1 ]--------+-------------
+assigned_group_state | wait_standby
+
+SELECT nodeid AS ns2 FROM pgautofailover.node
+ WHERE formationid = 'sss2_test' AND nodename = 'sss2_s2' \gset
+-- ── bootstrap: p single -> wait_primary, both standbys join as wait_standby ─
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :np, 0,
+                                  current_group_role => 'single');
+-[ RECORD 1 ]--------+-------
+assigned_group_state | single
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :ns1, 0,
+                                  current_group_role => 'wait_standby');
+-[ RECORD 1 ]--------+-------------
+assigned_group_state | wait_standby
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :ns2, 0,
+                                  current_group_role => 'wait_standby');
+-[ RECORD 1 ]--------+-------------
+assigned_group_state | wait_standby
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :np, 0,
+                                  current_group_role => 'single',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+-------------
+assigned_group_state | wait_primary
+
+-- p: single -> wait_primary (confirm); steady, no secondary yet.
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :np, 0,
+                                  current_group_role => 'wait_primary',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+-------------
+assigned_group_state | wait_primary
+
+-- Both standbys are assigned catchingup concurrently, while p is still
+-- steady wait_primary -- an ordinary two-standby startup so far.
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :ns1, 0,
+                                  current_group_role => 'wait_standby');
+-[ RECORD 1 ]--------+-----------
+assigned_group_state | catchingup
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :ns2, 0,
+                                  current_group_role => 'wait_standby');
+-[ RECORD 1 ]--------+-----------
+assigned_group_state | catchingup
+
+-- Both standbys report sufficient LSN and get assigned goalState=secondary
+-- (still reporting catchingup themselves at this point -- the actual
+-- catchingup -> secondary *transition* only happens below, on each one's
+-- next call reporting secondary itself).
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :ns1, 0,
+                                  current_group_role => 'catchingup',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+----------
+assigned_group_state | secondary
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :ns2, 0,
+                                  current_group_role => 'catchingup',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+----------
+assigned_group_state | secondary
+
+-- s1 finishes first: catchingup -> secondary (the actual transition). p is
+-- still steady wait_primary at this exact moment -- pos 302 must not fire
+-- here, same as the ordinary one-standby case in node_active_protocol.sql.
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :ns1, 0,
+                                  current_group_role => 'secondary',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+----------
+assigned_group_state | secondary
+
+-- p: wait_primary (own heartbeat) -- *now* the monitor sees a quorum
+-- secondary (s1) and assigns goalState=primary, while p's own
+-- reportedState is still wait_primary: this is the mid-promotion window
+-- the real keeper's fsm_enable_sync_rep() retry loop would be sitting
+-- inside, before its own local pg_stat_replication check lets it report
+-- reportedState=primary.
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :np, 0,
+                                  current_group_role => 'wait_primary',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+--------
+assigned_group_state | primary
+
+SELECT nodename, goalstate, reportedstate
+  FROM pgautofailover.node
+ WHERE formationid = 'sss2_test' AND nodename = 'sss2_p';
+-[ RECORD 1 ]-+-------------
+nodename      | sss2_p
+goalstate     | primary
+reportedstate | wait_primary
+
+-- ── the race: s2 finishes catching up while p is still mid-promotion ───────
+-- s2: catchingup -> secondary, with p still reportedState=wait_primary /
+-- goalState=primary from the step above. This is a one-time transition
+-- per standby (gated on previouslyReportedState=catchingup) -- if it
+-- doesn't force p through apply_settings now, nothing else ever will.
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :ns2, 0,
+                                  current_group_role => 'secondary',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+----------
+assigned_group_state | secondary
+
+-- Regression check: p's goalState must now be apply_settings, not left at
+-- primary -- this is the exact bug symptom (goalState silently staying
+-- primary here is what let the real keeper's locally-applied
+-- synchronized_standby_slots GUC get stuck on a one-slot list forever).
+SELECT nodename, goalstate, reportedstate
+  FROM pgautofailover.node
+ WHERE formationid = 'sss2_test' AND nodename = 'sss2_p';
+-[ RECORD 1 ]-+---------------
+nodename      | sss2_p
+goalstate     | apply_settings
+reportedstate | wait_primary
+
+-- ── drive p the rest of the way, as the keeper would ────────────────────────
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :np, 0,
+                                  current_group_role => 'apply_settings',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+--------
+assigned_group_state | primary
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('sss2_test', :np, 0,
+                                  current_group_role => 'primary',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+--------
+assigned_group_state | primary
+
+SELECT nodename, goalstate, reportedstate, candidatepriority
+  FROM pgautofailover.node
+ WHERE formationid = 'sss2_test'
+ ORDER BY nodename;
+-[ RECORD 1 ]-----+----------
+nodename          | sss2_p
+goalstate         | primary
+reportedstate     | primary
+candidatepriority | 100
+-[ RECORD 2 ]-----+----------
+nodename          | sss2_s1
+goalstate         | secondary
+reportedstate     | secondary
+candidatepriority | 100
+-[ RECORD 3 ]-----+----------
+nodename          | sss2_s2
+goalstate         | secondary
+reportedstate     | secondary
+candidatepriority | 100
+
+-- Both standbys are caught-up candidates: both slot names.
+SELECT pgautofailover.synchronized_standby_slots('sss2_test', 0);
+-[ RECORD 1 ]--------------+----------------------------------------------------
+synchronized_standby_slots | pgautofailover_standby_43,pgautofailover_standby_44
+