Replace the monitor's hand-written FSM dispatch with a declarative MonitorFSM[] table + keeper reachability cross-check - #1185
Merged
Conversation
Working, fully-tested implementation (14/14 regress, 6/6 isolation, 49/49 + 101/101 pgaftest, all in Docker) but deviates from the design doc at /Users/dim/dev/temp/monitor-fsm-data-driven-refactor-prompt.md in two ways the doc explicitly argues against: - three separate arrays (MonitorFSM_EarlyChecks[], MonitorFSM_FromContext[], MonitorFSM_ForPrimaryNode[]) instead of one MonitorFSM[] with named section-boundary constants. - a looping dispatch site (resume from index+1 when a row's extraAction returns false) instead of the doc's single-shot driver + bounded named jump inside extraAction. This is the continuesDispatch mechanism the doc's own design phase tried and reverted -- and it reproduced exactly the double-invocation failure mode the doc predicted, fixed here by merging three sibling rows into one instead of adopting the doc's named-boundary jump. Keeping this commit as a known-good reference point before reworking towards the doc's actual single-array/single-shot-dispatch design.
Reworks the previous checkpoint (9c9c9b9) to match /Users/dim/dev/temp/monitor-fsm-data-driven-refactor-prompt.md's actual design, which that commit deviated from in two ways the doc explicitly argues against: - Merges MonitorFSM_EarlyChecks[]/FromContext[]/ForPrimaryNode[] into one MonitorFSM[] array (48 rows), with named boundary constants (MonitorFSM_FromContextStart=6, MonitorFSM_MSFailoverClusterStart=9, MonitorFSM_PrimaryNodeSectionStart=37) instead of three separate arrays standing in for section boundaries. - Replaces the looping dispatch site (extraAction returning bool, resuming from index+1 on decline -- functionally the continuesDispatch mechanism the doc's own design phase tried and reverted) with the doc's single-shot driver: ProceedGroupStateFromContext now makes at most a small, fixed number of straight-line lookups and dispatches at most one row per call. MonitorExtraActionFunction is void again; ActionRunMultiStandbyFailoverCascade and ActionRunPrimaryNodeTransition each do one bounded, explicitly-named nested search+dispatch instead of signaling the driver to keep scanning. One deliberate deviation from the doc's own top-level driver snippet, kept and documented in ProceedGroupStateFromContext's comment: the six early-check rows are always tried first via their own lookup, rather than being skipped via 'startIndex = isInPrimaryState ? PrimaryNode SectionStart : 0'. The literal doc snippet would skip them whenever activeNode is already primary-role, silently reintroducing the drop_node regression (a primary that just lost its only standby must still reach SINGLE via those checks before any primary-role redirect). ProceedGroupStateForPrimaryNode is removed as a separate function -- folded into the top-level driver's primary-role branch and ActionRunPrimaryNodeTransition, both now querying MonitorFSM[] directly via the shared FindAndDispatchMonitorFSMRule helper. Verified: 14/14 regress + 6/6 isolation, both locally and in the Docker-based installcheck (pgaf-base:bookworm, ephemeral pg_virtualenv cluster) -- same as the three-array checkpoint, confirming this is a structural realignment with the doc, not a behavior change.
FindAndDispatchMonitorFSMRule's own doc comment miscounted ProceedGroupStateFromContext's straight-line lookups as three; it's two (early checks, then either the primary-role section or the rest of the FromContext range). The third lookup I'd been counting belongs to a different function entirely -- the conditional nested search inside ActionRunMultiStandbyFailoverCascade's extraAction, which only runs when that row's own MS-failover cascade declines. No functional change, comment only.
…umeStart Reusing the design doc's constant name was misleading: in the doc, MonitorFSM_MSFailoverClusterStart marks the start of a real, converted section -- roughly ten declarative rows (a fourth candidateNode role, conditions like candidatePromotionInProgress and mostAdvancedCandidate WithinPromoteThreshold, an otherNodesFn hook) that partially replace BuildCandidateList/SelectFailoverCandidateNode/PromoteSelectedNode. No such section exists in this table. ProceedGroupStateForMSFailover() and everything it calls stayed one opaque hand-written function, called wholesale from ActionRunMultiStandbyFailoverCascade -- exactly the scope reduction already disclosed, but the borrowed name implied a conversion that didn't happen. The renamed constant just marks 'resume scanning ordinary FromContext rows from here' when that hand-written call declines -- a narrower thing than what the doc's identically- shaped constant was for. Comments at both definition and both use sites now say this explicitly. No functional change.
…dump_fsm()
Builds on 4768b05 to complete the declarative dispatch table design
(/Users/dim/dev/temp/monitor-fsm-data-driven-refactor-prompt.md), so the
monitor's own FSM can be introspected and cross-checked against the
keeper's KeeperFSM[] (src/bin/pg_autoctl/fsm.h).
Operator-triggered (API_TRIGGERED) section:
- New MONITOR_FSM_SECTION_API_TRIGGERED (15 rows, pos 101-129), a new
MonitorApiFunction enum, ApiTriggerPattern/API_TRIGGER() matching
mechanism, and ProceedGroupStateForApiTrigger() entry point.
- Converts all 7 operator-triggered SQL functions (remove_node,
perform_failover, start_maintenance, stop_maintenance,
set_node_candidate_priority, set_node_replication_quorum,
set_formation_number_sync_standbys) in node_active_protocol.c/
formation_metadata.c to dispatch through the table instead of
hand-written AssignGoalState calls.
- Found and fixed three real bugs while converting: a dropped-primary
fan-out row that could skip DROPPED under first-match-wins; two rows
using NODE_STATE_NOT_ASSIGNED where NODE_STATE_NOT_STABLE was the
actual guard; both confirmed via regress failures, not by inspection.
MS-failover / candidate-selection cluster (pos 363-379):
- Added candidateNode as a third NodeStatus role, plus the MS-failover
cluster's own facts (activeNodeAllWalSourcesUnhealthy,
candidatePromotionInProgress, mostAdvancedCandidateWithinPromoteThreshold,
guardDataLossEnabled) and a new inMSFailoverCluster marker.
- TryMSFailoverDeclarativeRow wires the retry-reset and join_secondary
transitions; TryFanOutReportLsnRow wires BuildCandidateList's own
fan-out to REPORT_LSN (4 rows, one per fromState shape);
DispatchMonitorFSMRuleByPos wires PromoteSelectedNode's
PREPARE_PROMOTION/FAST_FORWARD choice (2 rows sharing identical
conditions, disambiguated only by PromoteSelectedNode's own internal
LSN comparison, not by any BoolPattern). BuildCandidateList/
SelectFailoverCandidateNode/PromoteSelectedNode/ProceedWithMSFailover
themselves stay hand-written C; only their tail-end AssignGoalState
calls dispatch through the table, each falling back to the original
call on no match.
- inMSFailoverCluster fixed a real bug: extending the MS-failover
cluster's own upper bound also exposed the new fan-out rows to the
ordinary top-level dispatch (same shared bound), so they briefly
hijacked ordinary 2-node heartbeats. Caught via a 9-test regress
regression, fixed by gating every MS-failover-only row on a marker
that's true only when built by BuildMSFailoverNodeActiveContext.
dump_fsm()/pgautofailover.fsm (introspection view):
- Exposes active/other/candidate current (reported) state,
per-role and group-level BoolPattern conditions (health foremost,
including a goal-state precondition for NODE_STATE_ASSIGNED/
NOT_ASSIGNED rows that would otherwise render as entirely blank),
assigned states, and whether a row has an extraAction.
- section folds in the API function for API_TRIGGERED rows as plain
text ("api_triggered: remove_node") instead of a separate column.
- rule_pos/rule_section on pgautofailover.event (set from
DispatchMonitorFSMRule) let a query join an actual event back to the
exact row that produced it.
Tests: fsm.sql (plain \x on dump of all 72 rows, one regression diff
per row added/removed/edited) and cluster_init_failover_rule_attribution.sql
(end-to-end bootstrap + manual failover, joining pgautofailover.event to
pgautofailover.fsm on rule_pos to show which row produced each transition).
Verified: 16/16 regress + 6/6 isolation (Docker installcheck, pgaf-base:
bookworm), plus live pgaftest against real multi-standby Docker clusters
(multi_standbys 27/27, multi_alternate 16/16, multi_ifdown 12/12,
multi_maintenance 22/22).
Builds the other half of the cross-check dump_fsm() was always meant to
support (design doc's "Exposing both tables for the cross-check", never
previously implemented -- confirmed by the doc's own "Implementation
readiness" section: "check_fsm_reachability() and pg_autoctl do fsm
check... have never actually run once").
Monitor side (src/monitor/group_state_machine.c, pgautofailover.sql):
- pgautofailover.dump_fsm_edges(): a new SRF resolving every MonitorFSM[]
row into concrete (pos, current_state, assigned_state) edges, via a new
NodeStatePatternResolveFromStates() helper (handles all 7
NodeStatePatternKind values, including complement resolution for
NOT_STABLE and full-universe resolution for ANY/ASSIGNED/NOT_ASSIGNED).
Deliberately excludes two categories of edges, both confirmed by tracing
a live run's mismatches back to root cause rather than assuming:
reflexive (current == assigned) edges -- keeper_fsm_reach_assigned_state()
(fsm.c) returns before ever consulting KeeperFSM[] when
current_role == assigned_role, so a self-loop can never need a keeper
edge -- and the whole MONITOR_FSM_SECTION_API_TRIGGERED section, whose
rows resolve activeNode to a specific role (almost always the primary)
via hand-written C before dispatch, so their own NodeStatePattern was
never meant to double as a full reachability precondition.
- pgautofailover.check_fsm_reachability(keeper_edges jsonb): anti-joins
dump_fsm_edges() against a keeper's own edges (jsonb array of
{"current","assigned"} objects), returning every monitor edge with no
matching keeper entry. Casts keeper_edges' state names to
pgautofailover.replication_state, so an unrecognized name fails loudly
rather than silently never matching.
- check_fsm_reachability.sql: regress test for the SQL mechanism itself
(synthetic keeper-edge inputs), not the real KeeperFSM[] table.
Keeper side (src/bin/pg_autoctl/fsm.c, monitor.c, cli_do_fsm.c):
- KeeperFSMToJSON(): serializes KeeperFSM[] to the same edge shape,
expanding ANY_STATE (the "drop node from any state" wildcard) into all
21 concrete states -- found and fixed via the first live run, which
otherwise sent the literal string "#any state#" and failed the
monitor's own enum cast.
- monitor_check_fsm_reachability(): new monitor RPC sending that JSON to
check_fsm_reachability(), following the existing
monitor_get_node_region/monitor_report_timeline_history call patterns.
- "pg_autoctl inspect fsm check": new read-only CLI command wiring the
above together, mirroring cli_do_monitor_get_primary_node's shape
(unlike fsm list/gv, this one genuinely needs a monitor connection).
Also fixes a real, unrelated bug this feature's own large JSON parameter
exposed: pgsql_execute_with_params' debug-parameter trace (src/bin/common/
pgsql.c) used a fixed 1024-byte buffer and logged a "BUG:" line whenever a
parameter didn't fit -- cosmetic (the real query always used the full,
untruncated parameter), but the first caller to ever pass more than ~1KB.
Fixed by capping each parameter's own printed representation.
Verified against a live monitor+keeper pair (not just Docker installcheck):
first run found 23 mismatched MonitorFSM rows; tracing each one down to
its real KeeperFSM[] cause is what led to the reflexive-edge and
api_triggered exclusions above, bringing it down to 9 remaining
candidates, none matching a known GitHub issue -- see follow-up commits.
ActionRunMultiStandbyFailoverCascade contained two raw, hand-written AssignGoalState(primaryNode, DRAINING/MAINTENANCE, ...) calls -- invisible to dump_fsm()/dump_fsm_edges() and the "systematic visibility" guarantee the rest of this refactor gives every other transition. Fix: add two new declarative rows at pos 381 (DRAINING) and 383 (MAINTENANCE) to MonitorFSM[], in MONITOR_FSM_SECTION_REPORTING_NODE, right after the existing MS-failover cluster (pos 379). Both rows reuse pos 305's own gating conditions verbatim (primaryNode.isUnhealthy, groupHasMoreThanTwoNodes) rather than a new marker boolean: the ordinary top-level scan can only ever reach pos 381/383 by first reaching pos 305, which unconditionally dispatches through its own extraAction (ActionRunMultiStandbyFailoverCascade) before the scan could resume past it -- so pos 305 always intercepts first whenever the shared gate holds, and if it doesn't, 381/383 wouldn't match either. The function body now tries FindAndDispatchMonitorFSMRule() over the new bounded range first, falling back to the original hand-written if/else-if only if neither row matches (should never happen). The fallback reuses nac->atLeastOneHealthyCandidate (already computed in BuildFromContextNodeActiveContext under the identical isUnhealthy/ groupNodeCount>2 gate) instead of recomputing candidatesCount locally -- it turned out to be the exact same AutoFailoverOtherNodesListInState + CountHealthyCandidates computation. Reviewed every other AssignGoalState call site in group_state_machine.c for the same gap: ActionCatchupUnhealthySecondaries and ActionFanOutReportLsnOnPrimaryRemoval are genuine fan-outs over a dynamic node list (not expressible as a single row, already flagged as such in their callers' row comments); everything else already follows the established try-declarative-first-fallback pattern. This was the only remaining case. MonitorFSM_SIZE: 72 -> 74. MonitorFSM_PrimaryNodeSectionStart: 61 -> 63. Regenerated fsm.out/check_fsm_reachability.out for the two new rows (total_edge_count 231 -> 251). Verified: full regress+isolation Docker installcheck clean, including concurrent_second_primary_death_report/concurrent_health_check_and_report (the isolation tests that historically caught the "3 separate rows" version of this bug class); live pgaftest multi_standbys.pgaf (27/27, including test_012_fail_primary which exercises this exact cascade).
Gives "pg_autoctl inspect fsm list --json" (KeeperFSMToJSON()) a way to run with genuinely zero setup, and uses it to commit a reviewable, git-tracked snapshot of the real KeeperFSM[] table that a regress test compares against the monitor's own MonitorFSM[]/dump_fsm_edges(). cli_getopt_pgdata_or_json (cli_common.c/h): a new getopt function used only by fsm_list's own CommandLine, parsing the identical option set as the shared cli_getopt_pgdata but skipping prepare_keeper_options()'s unconditional config-file-existence check when --json is set. Needed because cli_getopt_pgdata (used, unconditionally, by every other terminal command) calls prepare_keeper_options() before the command handler ever runs, regardless of which subcommand or flags were parsed -- so cli_do_fsm_list's own outputJSON-short-circuit (added earlier this branch) was previously unreachable without a real, pre-existing node config. cli_getopt_pgdata itself is untouched, so every other command using it keeps requiring a real config exactly as before. keeper_fsm_edges.json: generated via "pg_autoctl inspect fsm list --json | python3 -m json.tool" (pretty- printed for git-diff review; KeeperFSMToJSON() itself stays compact, since it's also the payload for the live monitor_check_fsm_reachability() RPC, and payload size was exactly what src/bin/common/pgsql.c's debug-log buffer fix, earlier this branch, had to guard against). Must be regenerated by hand whenever KeeperFSM[] (src/bin/pg_autoctl/fsm.c) changes. sql/keeper_fsm_edges.sql: a two-step regress test. Step 1 loads the fixture client-side (psql's own backtick file embedding -- server-side pg_read_file() is superuser-gated and resolves paths against $PGDATA, not this test's own directory) into a real table, one row per distinct keeper edge (DISTINCT: KeeperFSMToJSON()'s ANY_STATE expansion can make two different KeeperFSM[] rows resolve to the same edge, and the JSON carries no per-row provenance to tell them apart by), then SELECTs from it so expected/keeper_fsm_edges.out shows the whole keeper FSM line by line, human-reviewable. Step 2 anti-joins that table against pgautofailover.dump_fsm_edges() for the real, actionable gap list -- unlike check_fsm_reachability.sql's own synthetic-input test, which only exercises the comparison mechanism itself. The real gap list (134 rows, all per-state expansions of pos 209, 211, 303, 325, 333, 339, 347, 349, 351, and now also 381) matches this session's own earlier finding: dump_fsm_edges() doesn't (and structurally can't, from a NodeStatePattern alone) narrow a `.primaryNode` role match down the way isInPrimaryState would in a full BoolPattern evaluation, so these rows over-report reachability requirements the keeper's real FSM was never expected to need. Pos 381 (this branch's own new ActionRunMultiStandbyFailoverCascade row) falls into the exact same category for the identical reason, not a new gap. Verified: full regress+isolation Docker installcheck clean (18/18 regress including the new keeper_fsm_edges, 6/6 isolation).
pgautofailover.last_events() (all three overloads) declares RETURNS
SETOF pgautofailover.event, but its own SELECT list never included the
rule_pos/rule_section columns added earlier this branch -- so every
call errored at parse time ("Final statement returns too few
columns"), before ever running. This broke "pg_autoctl show events"
and "pg_autoctl watch" entirely. Never caught because nothing in the
regress suite ever called last_events() -- added minimal coverage in
monitor.sql (all three overloads, count(*) >= 0 checks so the
expected output stays stable regardless of exact event counts
elsewhere in the file).
While fixing this, found the formation+group overload was additionally
missing nodename/nodehost/nodeport entirely (13 columns instead of
16) -- a separate, pre-existing bug that would have broken
"pg_autoctl show events --group N" specifically, also never caught for
the same reason. Fixed by aligning both overloads to select the same
column set.
Also fixes two comments (pgautofailover.sql's pgautofailover.event
table, and notifications.h's CurrentMonitorFSMRulePos declaration --
same wrong claim duplicated in both places) that said rule_pos is NULL
for operator-triggered SQL functions and for
ProceedGroupStateForMSFailover's own hand-written internals. Verified
against the actual dispatch code this is wrong: operator-triggered
functions dispatch via ProceedGroupStateForApiTrigger, which itself
calls DispatchMonitorFSMRule (sets the global); MS-failover's
internals are only ever invoked from inside an outer row's own
extraAction, so the global is already non-zero by the time they call
AssignGoalState directly. Both get a real, non-NULL rule_pos --
attributed to the OUTER triggering row, not one of their own, since
MS-failover's candidate-selection internals were never decomposed into
declarative rows. Corrected both comments to describe this accurately.
Verified: full regress+isolation Docker installcheck clean (18/18
regress including the new last_events coverage, 6/6 isolation); live
psql calls against a real monitor confirming last_events() now returns
rule_pos/rule_section correctly for both the all-formations and
specific-group cases.
MonitorEvent gains rulePos/ruleSection fields (0/"" meaning "no rule
attributed", matching the monitor-side CurrentMonitorFSMRulePos
convention).
monitor_get_last_events() (backs "pg_autoctl watch"'s events pane):
adds rule_pos/rule_section to its SQL and parsing. This also fixes the
pre-existing missing-nodename/nodehost/nodeport bug in its
formation+group SQL branch (see the previous commit -- same file, same
underlying last_events() overload).
monitor_print_last_events()/printLastEvents (backs plain "pg_autoctl
show events"): adds a "Rule" column showing rule_pos, positioned before
Comment; blank when NULL.
monitor_print_last_events_as_json() ("show events --json"): needed no
changes at all -- it already does row_to_json(event), which picks up
every column of the event row type generically, including rule_pos/
rule_section, once last_events() itself returns them. While here,
fixed an unrelated, pre-existing SQL typo in this same function's
formation+group branch: "FROM * FROM pgautofailover.last_events(...)"
(a stray duplicate FROM, invalid syntax) -- broke "show events --json"
for any specific group, for a third, independent reason from the two
fixed in the previous commit.
watch_colspecs.h: new EVENT_COLUMN_TYPE_RULE_POS, added to the
"verbose" event column policy only (between Name and Description) --
watch.c wires it into compute_event_column_size/print_event following
the exact pattern the existing columns already use (dynamic max-width
tracking against both the data and the "Rule" header's own length).
Verified: clean compile (monitor extension + pg_autoctl), and end to
end against a live monitor+node in Docker -- "show events" (plain and
--json) and the formation+group-specific query path all correctly show
rule_pos 209/"early_checks" for an FSM-dispatched event and blank/null
for a health-check-triggered one. The interactive "watch" TUI itself
couldn't be exercised headlessly (needs a real tty), but its own query
is the same shape independently verified via psql and "show events".
Adds an "event summary" query at the tail of every regress test that
actually registers nodes and drives state transitions: workers,
node_active_protocol (both formations it uses), guard_data_loss,
fast_forward, drop_node, stale_primary_report, lock_and_fetch_migration,
timeline_fork_detection (both formations), and
failover_candidate_leaves_secondary. Skipped: create_extension, fsm,
check_fsm_reachability, keeper_fsm_edges (no formation/node activity at
all), dummy_update/drop_extension/upgrade (extension-version lifecycle
tests, not FSM scenarios), and cluster_init_failover_rule_attribution
(already has its own, more detailed rule-attribution query at its own
tail).
Each summary is:
SELECT reportedstate, goalstate, rule_pos, rule_section, description
FROM pgautofailover.last_events('<this test's own formation>',
count => 100);
Filtering by the test's own uniquely-named formation (gdl_test, ff_test,
dn_test, etc.) isolates each summary from every other test sharing the
same pgautofailover.event table across this schedule's serial run --
verified safe regardless of position, since nodeid/event rows carry no
FK to pgautofailover.node (dropping a node doesn't cascade-delete its
history). eventid and eventtime are deliberately never selected: eventid
is a database-wide sequence shared by the whole schedule (see
regress_schedule's own comment) and eventtime is a live timestamp --
neither is a value this file's own expected output could ever pin
stably.
This gives pgautofailover.last_events() -- broken for a long time until
the previous two commits, entirely unexercised until this one -- real
coverage against genuine multi-step scenarios (17-30 events each),
not just the synthetic smoke test in monitor.sql. Reviewed every
generated summary for sanity (correct node names, no cross-test
bleed, sensible rule_pos/rule_section per event) before promoting.
pg19-specific handling: expected/pg19/expected/{workers,
node_active_protocol,guard_data_loss}.out are symlinks to the main
expected file, so needed no separate edit. fast_forward.out and
timeline_fork_detection.out are real, separate files (pg_lsn display
format differs on PG19) -- appended the identical new tail content to
both, since none of the summary's own columns touch lsn/timestamp
fields.
Verified: full regress+isolation Docker installcheck clean (18/18
regress including all nine modified tests, 6/6 isolation).
otherNodeAssignedState's real target was always nac->primaryNode.node, hardcoded directly in DispatchMonitorFSMRule -- meaning "the other node in this transition" and "the group's primary" were the same concept by construction, with no way for a row to say otherwise. Decouples them: - NodeActiveContext gains otherNode (a NodeStatus, same shape as activeNode/primaryNode/candidateNode). BuildFromContextNodeActiveContext and BuildApiTriggerNodeActiveContext -- the only two builders that ever populate a real primaryNode -- now also set nac->otherNode = nac->primaryNode right after, so otherNode.node == primaryNode.node everywhere, exactly as before. (BuildForPrimaryNode NodeActiveContext and BuildMSFailoverNodeActiveContext leave .primaryNode at its memset-zero default already, same as before.) - MonitorFSMTransition gains a matching otherNode NodeStatusPattern field, wired into RuleMatches. Every existing row omits it, which NodeStatusPattern's own "omitted means don't-care" default already makes a no-op -- zero risk to any of the 74 existing rows, same guarantee this array's other additive fields (apiFunction, inMSFailoverCluster, etc.) already established when each was added. - DispatchMonitorFSMRule now assigns otherNodeAssignedState to nac->otherNode.node instead of nac->primaryNode.node directly -- behaviorally identical today (same pointer), but the dispatch mechanism's own vocabulary is now genuinely activeNode/otherNode, with primaryNode/candidateNode as separate, monitor-domain-specific roles a row can still constrain independently. This is purely additive groundwork, not yet used by any row: it decouples "who otherNodeAssignedState targets" from "the primary" at the type level, which is the prerequisite for a future otherNodesFn mechanism (see the design doc's own proposal) to resolve otherNode to something else entirely -- e.g. a dynamically selected failover candidate in the MS-failover cluster -- without having to rename or restructure every existing primaryNode-shaped row's own conditions. dump_fsm_edges()'s own edge resolution (NodeStatePatternResolveFromStates called against rule->primaryNode.statePattern) deliberately still reads primaryNode, not otherNode: every existing row's real reachability constraint lives on .primaryNode (.otherNode defaults to NODE_STATE_ANY, unset everywhere), so switching that reference now would make every resolved edge look reachable from any reported state -- a real regression. Flagged in-place as a follow-up for whenever a row first sets .otherNode.statePattern to something .primaryNode doesn't already say. Verified: clean compile, full regress+isolation Docker installcheck (18/18 regress, 6/6 isolation) with zero expected-output changes -- confirms this is fully behavior-neutral, as designed.
Replace the 6 hand-maintained array-index boundary constants (MonitorFSM_EarlyChecksStart, MonitorFSM_FromContextStart, etc.) with a hierarchical MonitorFSMSectionPath carried on each row itself, matched via prefix containment (SectionPathIsUnderPrefix) instead of index ranges -- a row's membership no longer depends on where it happens to sit in the array. - Expand MonitorFSMSection in place with a NONE sentinel and fine-grained leaf values (reporting_node.ms_failover.* sub-sections); only sectionPath[0] is ever one of the original 4 SQL-visible values, so pgautofailover.fsm_section and rule_section are completely unaffected. - Add IntPattern (EXACTLY/AT_LEAST/AT_MOST) alongside the existing BoolPattern, and make ProceedGroupStateForMSFailover's three counting gates (missingNodesCount/candidateCount/quorumCandidateCount) declarative rows in MonitorFSM[], dispatched via a dedicated BuildMSFailoverCandidateGateNodeActiveContext and a new inMSFailoverCandidateGate guard (keeps them from being reachable through the existing MS-failover cluster's own broader scans). The decline-vs-continue control flow itself stays hand-written C; only the message text and conditions move into the table. - Add pgautofailover.fsm's new section_path column (ltree, cast at the view level only -- dump_fsm() itself just builds the dotted text) and the ltree control-file dependency. - Add candidate_count_gate.sql: dedicated regression coverage for the candidateCount == 0 gate, the one counting gate no existing test named explicitly. Full regress (19) + isolation (6) suites pass in Docker (PG17).
…ck; otherNodesFn; Assert fallbacks Three follow-ups from review of the section-path/IntPattern work: - Reformat every row's .sectionPath as one element per physical line (citus_indent-stable), instead of one long line per row -- easier to review diffs when a leaf changes. - keeper_fsm_edges.sql: add the reverse cross-check (step 2b) alongside the existing monitor-not-in-keeper one (step 2a) -- every keeper edge no MonitorFSM[] row can produce. - Implement the otherNodesFn mechanism the design doc's own comments gestured at but never built: MonitorFSMTransition gains an otherNodesFn field (GroupStateContext, NodeActiveContext) -> List *; when set, DispatchMonitorFSMRule loops the resolved list and calls AssignDeclaredGoalState per node instead of targeting the single nac->otherNode.node, giving every fanned-out node the same rule_pos/ rule_section attribution any other row's assignment already gets. Converts ActionCatchupUnhealthySecondaries (pos 403-419, 8 rows) and ActionFanOutReportLsnOnPrimaryRemoval (pos 101) from hand-written extraActions with zero dump_fsm() visibility into real otherNodeAssignedState-bearing rows. dump_fsm_edges() skips otherNodesFn rows explicitly: their target's current-state precondition isn't a NodeStatePattern, so resolving one would fabricate bogus edges from the NODE_STATE_ANY default. - Add Assert(false) to the 7 remaining raw AssignGoalState fallback sites (the "row should always match here" defensive branches): grepped every fallback's own message text against the full expected/*.out corpus and confirmed zero hits, so none has ever fired. Assertions are compiled out in production but catch a future row/call-site divergence immediately in any assert-enabled build instead of silently reverting to un-attributed pre-refactor behavior forever. Full regress (19) + isolation (6) suites pass in Docker (PG17).
Every raw AssignGoalState call outside AssignDeclaredGoalState is gone now (only the forward declaration, the one call inside AssignDeclaredGoalState itself, and the definition remain): - ActionRunMultiStandbyFailoverCascade's DRAINING/MAINTENANCE fallback: this one's "no match" case is a legitimate, expected no-op (the original hand-written if/else-if had no final else -- "neither applies" is a real, common outcome, not a bug), so last commit's Assert(false) here was wrong -- it would have tripped on ordinary heartbeats in an assert-enabled build. Simplified to a bare, return-value-ignored dispatch call, matching the file's own existing pattern for this exact situation. - The other 5 fallback sites (pos 363's retry-reset, the 367-373 fan-out, pos 365's join_secondary, pos 375/377's promotion outcome) are each reached only from inside a hand-written `if` whose own condition already guarantees a specific row must match -- genuinely "can't happen", not "shouldn't happen". Replaced with ereport(ERROR, "BUG: ...") instead of Assert(false) + a silently-kept AssignGoalState call: this fails loudly in every build, not just assert-enabled ones, and there's no more duplicate hand-written logic left to silently drift out of sync with the row it's supposed to mirror. Full regress (19) + isolation (6) suites pass in Docker (PG17), unchanged from before -- confirms every removed fallback really was dead code.
Every /* */ comment (standalone single-line, multi-line, and multi-line with text on the opening line) now wraps at 80 rendered columns (tabs expanded to 4) instead of the ad hoc widths they'd accumulated -- mechanical paragraph reflow via a small script (kept out of the repo), preserving blank-line paragraph breaks, bullet items, and anything containing real code syntax (braces, named C identifiers, #define/typedef) verbatim/unwrapped rather than risking corrupting it. Investigated using citus_indent/uncrustify itself for this (code_width=80, cmt_width=80, cmt_reflow_mode=2 via a local config copied out of the citus/stylechecker:no-py image): it technically supports both settings, but produces objectively broken output on this file -- a missing space after the '*' continuation marker on many reflowed lines, and ugly mid-declaration splits (pointer type separated from its own variable name). Reverted that approach; comments were reflowed by hand/script instead. Confirmed the result is stable under the project's own standard style-checker invocation (zero changes on a second pass) and that citus-style.cfg's own code_width=90/ cmt_width=0 (reflow disabled) stay untouched -- this is a one-file, purely cosmetic change with no config changes needed anywhere else. Code lines (signatures, expressions, struct literals) are intentionally left at their existing width -- only prose comments were in scope here. Full regress (19) + isolation (6) suites pass in Docker (PG17), as expected for a comment-only change.
…es step 2b Step 2b (keeper -> monitor reverse gap) was flagging 21 of its 38 rows as unreachable purely because dump_fsm_edges() excludes the entire api_triggered section, where remove_node()'s own DROPPED assignment lives. KeeperFSM[]'s two ANY_STATE -> DROPPED rows expand to one edge per concrete current_state (fsm.c's KeeperFSMToJSON()), so every one of them was a guaranteed false positive, not a real gap. Filter them out explicitly, and note in the comment that the remaining rows still need the same per-row api_triggered-aware judgment as step 2a.
…in SQL KeeperFSMToJSON() previously expanded a KeeperFSM[] row's ANY_STATE wildcard into one concrete edge per entry of a hand-maintained AllRealNodeStates C list. That has two problems: it silently under-covers the real wildcard semantics the moment the list drifts out of sync with the NodeState enum (a false negative nothing would ever catch), and it explodes a single keeper rule into 21 separate rows for every downstream comparison, which is exactly what forced keeper_fsm_edges.sql's previous 'assigned_state <> dropped' filter. Emit the literal string 'any' instead, and do the wildcard matching in SQL: - check_fsm_reachability(): CASE WHEN k.current = 'any' THEN true ELSE k.current::replication_state = e.current_state END -- CASE reliably short-circuits so 'any' never hits the enum cast, while every other value (including real typos) still fails loudly. - keeper_fsm_edges.sql: current_state becomes text (not the enum, since 'any' isn't a legal value); Step 1 still round-trips every non-'any' value through the enum for the same fail-loudly guarantee. Step 2a matches 'any' against every current_state. Step 2b matches it existentially, which collapses the old 21-row 'any -> dropped' explosion into a single row and removes the need for the blanket dropped filter entirely. Regenerated keeper_fsm_edges.json from the rebuilt binary and the matching expected output. Full regress (19) + isolation (6) suites pass; check_fsm_reachability's 'unrecognized state name fails loudly' case still holds.
Step 2a's 134-row gap list collapses to just 10 distinct MonitorFSM[]
rules (renamed pos -> rule in the select list), each fanning out
across many current_states via a broad NodeStatePattern. Add a
GROUPING SETS summary row per (rule, assigned_state, comment) showing
the fan-out count (n), ordered first via NULLS FIRST so each rule's
own detail rows follow its own header. Makes the shape of the gap
list ('10 broad rules, not 134 independent problems') visible without
counting detail rows by hand.
Investigated each of the 10 rules behind Step 2a's 134-row gap list against the pre-refactor hand-written code (commit 9c9c9b9^): in every case the old code's own condition was already broad (a role/negation check or opaque NodeIsXxx() helper, never an enumerated state list), so the new NodeStatePattern's fan-out is a faithful translation, not a refactor-introduced widening. Real regression/tap-spec precedent (issues #997, #1168) exists for the 'obvious' current_state each rule targets, but none of the 10 is tested from one of the other fanned-out states -- a structural artifact of this check, not 10 separate bugs. Recorded so this doesn't need re-investigating next time the gap list is reviewed.
…dowing caveat
tests/tap/specs/keeper_fsm_gap_211_primary_priority_zero.pgaf reproduces
pos 211's real gap live: a lone priority-zero primary (node2, after
node1 is dropped) gets assigned 'report_lsn' by the monitor but has no
KeeperFSM[] transition from PRIMARY_STATE to reach it, confirmed via
the exact fsm.c log_fatal firing every retry ('pg_autoctl does not
know how to reach state "report_lsn" from "primary"'). Not added to
any schedule (same convention as debug_failover_pg19.pgaf) since it's
expected to fail once the missing transition is implemented.
Attempting the equivalent for pos 209's 'maintenance' fanout state
disproved it instead: pos 205 ('converged to maintenance -> no-op',
unconditional on MAINTENANCE_STATE) intercepts every node_active() call
from a maintenance node first, so pos 209 can never actually fire for
that state -- confirmed live (monitor never re-assigned 'single').
dump_fsm_edges() resolves each row's edges independently and does not
model this first-match-wins shadowing between rows, so it can report
edges that are structurally unreachable in practice. Documented as a
caveat in dump_fsm_edges()'s own comment for whoever investigates the
remaining pos 209/211 fanned-out states next.
…een rows
dump_fsm_edges() previously resolved each MonitorFSM[] row's edges
independently, so it could report an edge for a current_state that an
EARLIER, unconditional row (lower array index, same top-level section)
would actually intercept first in real dispatch -- confirmed live via
pgaftest for pos 209's 'maintenance' fanout state, shadowed by pos
205's unconditional 'converged to maintenance -> no-op' row.
Add RuleUnconditionallyMatchesActiveNodeState/...PrimaryNodeState:
true iff a row's RuleMatches() would hold for EVERY possible dispatch
context whose activeNode/primaryNode reports a given state -- every
other NodeStatus role fully unconstrained, the role under test has no
condition beyond its own state, and every .conditions field is at its
'don't care' default (mirrors RuleMatches()'s own field list
one-for-one; a comment ties them together so future fields get added
to both). EdgeIsShadowedByEarlierRule scans earlier same-section rows
for one that qualifies, and both edge-emission loops in
dump_fsm_edges() now skip a candidate edge when this is true.
Caught and fixed one real bug while building this: a first draft
treated NodeStatePatternResolveFromStates' ASSIGNED/NOT_ASSIGNED/
TRANSITIONING handling (which resolves to 'the full state universe' or
'reportedStates alone, ignoring assignedStates' -- correct for that
function's own edge-SOURCE purpose) as if it meant those rows matched
unconditionally on reportedState too. It doesn't: ASSIGNED/NOT_ASSIGNED
key off goalState only, a separate runtime fact. Without excluding
those three kinds, pos 203 ('goalState == DROPPED, reportedState
irrelevant') looked unconditional for every state and wrongly
suppressed 5 of pos 209's other genuinely-reachable fanned states
(wait_standby, prepare_maintenance, wait_maintenance, fast_forward,
join_secondary) -- caught by re-running the pos 211 pgaftest spec and
noticing pos 209/211 lost far more than the one confirmed case.
Net effect after the fix: pos 209 and 211 each correctly lose exactly
two fanned states (maintenance, shadowed by pos 205; dropped, shadowed
by pos 201's own unconditional 'converged to dropped' row) and keep
every other one. total_edge_count: 251 -> 244. Deliberately scoped to
same-top-level-section shadowing only (documented as a sound
under-approximation, not exhaustive: EARLY_CHECKS unconditionally
precedes REPORTING_NODE in the same dispatch chain, so cross-section
shadowing can also happen in principle, just not detected here).
Full regress (19) + isolation (6) suites pass; the pos 211 pgaftest
spec (tests/tap/specs/keeper_fsm_gap_211_primary_priority_zero.pgaf)
still correctly reproduces its real, unshadowed gap after the fix.
… rejected
Attempted extending EdgeIsShadowedByEarlierRule to also treat
MONITOR_FSM_SECTION_EARLY_CHECKS as unconditionally preceding every
other section (justified by ProceedGroupStateFromContext() always
trying SectionEarlyChecks first). Found a concrete counter-example
before committing it: pos 205's STABLE-kind pattern requires
reportedState == goalState == maintenance, an equality NOT guaranteed
just because reportedState == maintenance -- stop_maintenance() on a
multi-node group dispatches through the separate api_triggered path
and can advance goalState independently of the node's own next
heartbeat. The cross-section version would have wrongly treated pos
205 as shadowing pos 369 ("MS-failover fan-out: rejoining from
maintenance -> report_lsn"), which specifically requires reportedState
== maintenance AND goalState == catchingup, deleting a real,
reachable edge. Caught by hand-tracing before committing, not by any
test failure.
Reverted to the already-committed same-section-only behavior (verified
byte-identical regress+isolation output), with the failed attempt and
its concrete counter-example documented in EdgeIsShadowedByEarlierRule's
own comment so it isn't re-attempted without this context.
…add pos 209 wait_maintenance spec
dump_fsm_edges() resolved a row's candidate current_state purely from
.statePattern, never looking at that same role's other BoolPattern
conditions -- isInPrimaryState in particular. Several primaryNode-side
rows (pos 303, 325, 391) set .primaryNode.statePattern to ANY (or a
broad NOT_STABLE set) alongside .isInPrimaryState = BOOL_TRUE, so
dump_fsm_edges() enumerated all 21 (or ~20) states as candidate
sources even though IsInPrimaryState() (node_metadata.c) can only ever
be true for a small, fixed set: CanTakeWritesInState's own
{single, primary, wait_primary, join_primary, apply_settings}, plus
primary/apply_settings via its own second disjunct (already a subset).
Every other state (catchingup, secondary, dropped, maintenance, ...)
is structurally impossible for a node IsInPrimaryState() accepts,
regardless of goalState.
Added StateCanSatisfyIsInPrimaryState (a direct, provably-correct
satisfiability check against IsInPrimaryState's own two-disjunct
definition) and NodeStatusPatternSurvivesIsInPrimaryState, wired into
both edge-emission loops in dump_fsm_edges(). Deliberately narrow in
scope to only this one field -- sibling state-dependent fields
(isInMaintenance, canTakeWrites, drainTimeExpired,
unreachableFromDemoteTimeout) are NOT touched here, since
EdgeIsShadowedByEarlierRule's own comment already documents a concrete
case (pos 369) where a seemingly-safe assumption about one of these
turned out wrong once a separate write path was accounted for --
extending further needs the same due diligence per field, not a
blanket generalization.
Effect, confirmed via full regress+isolation: total_edge_count 244 ->
198, Step 2a's gap list 143 -> 98 rows. Pos 303 is now fully closed
(zero remaining gap states -- every state IsInPrimaryState can ever
admit is already covered by a real KeeperFSM[] transition). Pos 325
and 391 drop from 16 states each to 1 ("single"). Pos 333/339/347/349/
351 are unaffected (verified: none of them actually set
.isInPrimaryState -- 333/351 use bare .exists, 339 uses NOT_STABLE
+ isInMaintenance, 347 uses drainTimeExpired, 349 uses a .conditions
flag with no .primaryNode restriction at all), confirming the fix only
touches the rows it's actually justified for.
Also adds tests/tap/specs/keeper_fsm_gap_209_wait_maintenance.pgaf,
the second fully-validated pgaftest reproduction (after pos 211's):
network-disconnecting the primary durably strands a lone last-quorum-
member secondary at WAIT_MAINTENANCE_STATE (not a narrow timing
window -- it can never receive the primary's ack while disconnected),
then dropping the primary directly via pgautofailover.remove_node()
leaves it alone in the group, confirming live that the monitor
assigns SINGLE and the keeper has no WAIT_MAINTENANCE_STATE ->
SINGLE_STATE transition (log_fatal captured). Re-verified both this
spec and the pos 211 one still reproduce their real gaps after the
isInPrimaryState fix.
…itorFSM_SIZE
pos 210 ("alone in group, already primary despite candidatePriority zero ->
single") was gated on isInPrimaryState=true, which also requires goalState to
already agree with reportedState. A live pgaftest run
(keeper_fsm_gap_211_primary_priority_zero.pgaf) caught a real
self-undermining oscillation: the row's own extraAction reassigns goalState
to SINGLE, which makes isInPrimaryState() evaluate false on the very next
dispatch (goalState=single no longer agrees with reportedState=primary), so
the row stopped matching one dispatch after firing and pos 211 (no such
requirement) fired right behind it, overwriting the assignment back to
REPORT_LSN -- silently undoing the fix in production.
Replace the isInPrimaryState condition with a new NODE_STATE_REPORTED
pattern (FSM_REPORTED_PRIMARY_ROLE_STATES) that matches on reportedState
alone, ignoring goalState entirely. Since pos 210's own action never touches
reportedState, the match stays stable across dispatches until the keeper
itself actually converges to single. Rewrote
keeper_fsm_gap_211_primary_priority_zero.pgaf to assert the fixed
convergence-to-single behavior instead of the old stuck-at-report_lsn bug it
was written to demonstrate.
Also remove MonitorFSM_SIZE, a hand-maintained #define that a prior commit
had already caught silently dropping the array's last row (pos 421) from
every bounded loop when a new row was added without also bumping it, and had
patched with an Assert(size == sizeof(...)/sizeof(...)) safeguard. That
safeguard only works after the array's full definition is lexically visible,
but two call sites need the count before that point (only an incomplete
forward declaration of MonitorFSM[] is visible there), so it couldn't close
the gap for every caller. Replace the whole mechanism with a terminator row
(.pos left at its zero default) at the end of MonitorFSM[]: every loop now
walks until it sees pos == 0 instead of a separately maintained count, so a
row added before the terminator is automatically in scope everywhere, and
the terminator can't itself drift out of sync since it's part of the array's
own literal initializer. AssertMonitorFSMWellFormed() keeps a generous
iteration-count sanity check as a backstop against the terminator being
removed or a row being added after it by mistake.
Verified: full regress (19/19) + isolation (6/6) suites pass, fsm.out and
check_fsm_reachability.out regenerated (zero change to keeper_fsm_edges.out
-- same set of assigned edges, just a more precise match condition), both
keeper_fsm_gap_209/211 pgaftest specs pass live against a rebuilt Docker
image.
Both specs were live-validated (individually and together via --schedule) against the pos 210/isInPrimaryState oscillation fix and the pos 209 wait_maintenance gap fix, and now assert the FIXED behavior rather than demonstrating a bug -- add them to tests/tap/schedules/node.sch (the schedule ci.yml's test_pgaftest job actually runs) alongside the other FSM-edge-case regression specs (demote_timeout_wait_primary_deadlock, timeline_fork_report_lsn_deadlock), and to the legacy tests/tap/schedule for documentation consistency (not read by CI, but kept in sync). Re-ran make installcheck against current HEAD to check for a new list of keeper-unreachable edges: none -- fsm.out, check_fsm_reachability.out, and keeper_fsm_edges.out are byte-identical to their committed expected versions (confirmed via explicit diff, not just pg_regress's own ok/not ok). pos 210's REPORTED-kind pattern rewrite produces the exact same set of assigned edges as before, just via a mechanism that doesn't self-undermine -- no new gap opened or closed.
pos 210 (added in 35de216) makes an already-primary, priority-zero lone
node go to SINGLE instead of pos 211's REPORT_LSN, and always intercepts
those 4 states first (identical candidateEligible/groupHasExactlyOneNode
conditions, checked earlier in the array) -- but dump_fsm_edges() kept
listing pos 211 as still capable of producing primary/wait_primary/
join_primary/apply_settings -> report_lsn. Verified live via
dump_fsm_edges() directly: the edge was still there, and Step 2a of
keeper_fsm_edges.out still flagged it as a keeper-coverage gap, even
though it's genuinely unreachable in practice.
Root cause: dump_fsm_edges()'s shadow-detector (EdgeIsShadowedByEarlierRule
/ RuleUnconditionallyMatchesActiveNodeState) only recognizes an earlier
row as shadowing a state when that earlier row is *fully* unconditional --
every field besides the state pattern must be the wildcard default. pos
210 isn't: it also requires candidateEligible=FALSE and
groupHasExactlyOneNode=TRUE. Those happen to be identical to pos 211's own
conditions, making the shadow real and provable for this specific pair,
but the existing heuristic isn't built to prove that kind of matching-
conditions subsumption, only true unconditionality.
Rather than generalize the shadow-detector (invasive, and this project has
been deliberately conservative there -- a false positive would silently
hide a real edge), narrow pos 211's own pattern instead, making its code
match what its own comment already claimed ("was never already primary").
Adds a new NodeStatusPattern field, reportedCanTakeWrites
(CanTakeWritesInState applied to reportedState alone, mirroring the
existing goalState-based canTakeWrites) -- deliberately reported-only, for
the same reason pos 210's own fix needed a reported-only condition:
gating on anything goalState-dependent risks the self-undermining
oscillation documented on pos 210 (a row whose own extraAction changes
goalState can't safely condition its own match on goalState). Wired into
NodeMatchesPattern, the dump_fsm() conditions-text column, the shadow-
detector's own "other fields are ANY" check, and both dump_fsm_edges()
enumeration loops (activeNode/primaryNode).
pos 211 now sets reportedCanTakeWrites=FALSE, dropping the 4 already-
pos-210-owned states from its own resolved edge set. Verified:
dump_fsm_edges()'s total edge count drops from 202 to 198 (exactly the 4
phantom edges), Step 2a's gap count drops from 98 to 94 rows, full regress
(19/19) + isolation (6/6) suites pass with regenerated expected/fsm.out,
check_fsm_reachability.out, and keeper_fsm_edges.out, and both
keeper_fsm_gap_209/211 pgaftest specs still pass live.
…y from pos 209/211 Follow-up to the pos 209/211 gap investigation: for each of the two already-proven-real gap states (wait_maintenance, wait_standby), fixed the one that had a real fix and documented the one that didn't. wait_maintenance (KeeperFSM[], fsm.c): a converged, actively-streaming standby whose only peer vanishes while it's waiting to enter maintenance had no WAIT_MAINTENANCE_STATE -> SINGLE_STATE (pos 209) or -> REPORT_LSN_ STATE (pos 211) row, so it got stuck. Added both, reusing fsm_promote_ standby and fsm_report_lsn respectively -- the same functions every other converged-standby source state (SECONDARY/CATCHINGUP/PREP_PROMOTION/ STOP_REPLICATION) already reuses for the same targets, since entering wait_maintenance itself runs no transition function and leaves Postgres running and replicating normally. Regenerated keeper_fsm_edges.json (the checked-in fixture keeper_fsm_edges.sql reads, generated via `pg_autoctl inspect fsm list --json` -- confirmed via Makefile inspection this has no build-time dependency wiring at all, so it silently goes stale unless manually regenerated whenever KeeperFSM[] changes, exactly as happened here). wait_standby: a node stuck there never actually started streaming (no pg_basebackup done -- fsm_init_standby, the WAIT_STANDBY_STATE -> CATCHINGUP_STATE transition, never ran). The only available local action for SINGLE_STATE (fsm_init_primary, reused by INIT_STATE/DROPPED_STATE -> SINGLE_STATE) does a fresh initdb, generating a new system_identifier -- which collides with the one already registered for this node, tripping the monitor's own same_system_identifier_within_group exclusion constraint (confirmed live: "Failed to transition from state \"wait_standby\" to state \"single\""). No safe keeper-side fix exists. Excluded wait_standby from pos 209/211 entirely instead (new reportedIsWaitStandby field, NodeStatusPattern) -- the monitor simply never assigns a goal the keeper can't safely reach, leaving the node visibly stuck (still unhealthy) for an operator to notice, rather than pretending a fix exists. The exclusion needed two iterations to get right. The first version folded WAIT_STANDBY into the existing FSM_NOT_STABLE_SINGLE pattern (a NOT_STABLE kind, which requires reported == goal to exclude a state) -- this passed regress but still failed live: pos 101 (remove_node()'s own fan-out, unconditionally assigning REPORT_LSN to every surviving non-maintenance standby) rewrites the lone node's goalState synchronously as part of dropping its peer, before pos 209/211 ever evaluate it on its own next heartbeat. That breaks reported == goal, reviving the NOT_STABLE exclusion's match -- the exact self-undermining class of bug pos 210 hit earlier in this same investigation. Fixed by making the field goal-independent (a plain reportedState equality check, mirroring reportedCanTakeWrites), which cannot be defeated by another row's own goalState write. Confirmed live this time: node2 stays safely parked (reportedstate never leaves wait_standby, logs "Still waiting for the monitor to drive us to state \"catchingup\"") regardless of what its own goalState ends up as via pos 101's independent fan-out. Added keeper_fsm_gap_211_wait_maintenance.pgaf, keeper_fsm_gap_209_ wait_standby.pgaf, keeper_fsm_gap_211_wait_standby.pgaf (new) and rewrote keeper_fsm_gap_209_wait_maintenance.pgaf (existing, now asserts the fixed convergence instead of the bug it used to demonstrate). Documented pos 209/211's other remaining gap states (prepare_maintenance, demote_timeout, prepare_promotion, stop_replication, fast_forward, join_secondary) and pos 325's remaining "single" state as investigated -- each either contrived (implies a multi-node context or a tension with candidateEligible=FALSE) or a genuine model contradiction (325) -- not pursued this pass. Verified: full regress (19/19) + isolation (6/6) pass with regenerated fsm.out/check_fsm_reachability.out/keeper_fsm_edges.out, all 5 gap specs (the 2 rewritten/new wait_maintenance ones, the 2 new wait_standby ones, and the existing primary_priority_zero one) pass live against a rebuilt Docker image.
keeper_fsm_gap_211_wait_maintenance, keeper_fsm_gap_209_wait_standby, and keeper_fsm_gap_211_wait_standby (added in 4f59a40) all pass live -- add them to node.sch (the schedule ci.yml's test_pgaftest job runs) alongside the existing gap specs, and to the legacy tests/tap/schedule for documentation consistency.
pos 209/211 ("alone in group" -> single/report_lsn) already correctly
resolved fast_forward as a source state on the monitor side, but
KeeperFSM[] had no matching row: a node left alone while genuinely
reporting fast_forward (its WAL-source peer and the old primary both
gone mid MS-failover) would get stuck logging "does not know how to
reach state ... from \"fast_forward\"".
Add both rows to KeeperFSM[] (fsm.c):
- FAST_FORWARD_STATE -> SINGLE_STATE, via fsm_promote_standby
- FAST_FORWARD_STATE -> REPORT_LSN_STATE, via fsm_report_lsn
reusing the same transition functions already shared by every other
converged-standby source state (SECONDARY/CATCHINGUP/PREP_PROMOTION/
STOP_REPLICATION/REPORT_LSN/WAIT_MAINTENANCE) -- fast_forward only
means Postgres is already running as a caught-up-enough standby, and
fsm_fast_forward's own "no upstream found" branch already accepts
promoting with local data when nothing more advanced is available, so
this is exactly as safe as the analogous existing rows.
Add pgaftest coverage for both (keeper_fsm_gap_209_fast_forward.pgaf,
keeper_fsm_gap_211_fast_forward.pgaf), reproducing a genuine MS-failover
candidate falling behind, fetching real WAL from a peer, and then being
left alone -- using the established network-disconnect + INSERT +
CHECKPOINT divergence recipe (multi_ifdown.pgaf,
debug_citus_worker_fast_forward.pgaf). Getting these to actually
exercise the new rows (rather than the monitor's own ordinary
cascade continuing past fast_forward to prepare_promotion within
about a second of convergence) requires removing the other group
members while the candidate is still mid-fetch, not after it reports
fast_forward -- see the specs' own headers for the full explanation.
Also add keeper_fsm_gap_new_node_joins_report_lsn_group.pgaf, covering
a related question raised during this investigation: once a lone node
is parked at report_lsn with candidate-priority 0 (pos 211's own
outcome), can the cluster ever recover when a new peer joins? Confirmed
already fully working via RegisterNode()'s existing report_lsn-source
handling, no code change needed -- this is a pure regression spec.
Regenerate keeper_fsm_edges.json/expected/keeper_fsm_edges.out from the
real KeeperFSM[] table, and update the "remaining gap states" follow-up
comment in keeper_fsm_edges.sql to move fast_forward from "investigated,
not pursued" to "fixed and covered". Add all three new specs to the CI
schedules (tests/tap/schedule, tests/tap/schedules/node.sch).
Verified: full regress (19/19) + isolation (6/6) installcheck, citus_indent
clean, and all three new pgaftest specs passing live.
pos 209 ("alone in group, candidate-eligible -> single") was willing to
match a node reporting join_secondary or prepare_maintenance and promote
it straight to SINGLE. Investigating this surfaced a real data-loss risk
for both, not just a missing keeper transition:
- join_secondary: entering it stops Postgres (fsm_checkpoint_and_stop_
postgres) as part of switching allegiance to an already-elected new
primary. The on-disk data is only a trustworthy copy of this node's own
last moment as the OLD primary, frozen before that new primary ever
took a write. If that new primary later also vanishes, promoting this
node straight to SINGLE would silently discard everything it committed
in the meantime.
- prepare_maintenance: the same risk, reached one step earlier. pos 343
lets the candidate standby reach WAIT_PRIMARY/PRIMARY the moment this
node's own reportedState merely converges to prepare_maintenance -- no
requirement that this node's row ever be removed first. A different,
already-promoted primary can be live and taking writes while this node
still sits in prepare_maintenance indefinitely.
Fixed by adding reportedIsJoinSecondary/reportedIsPrepareMaintenance
exclusions to pos 209 (NodeStatusPattern, NodeMatchesPattern, and the
dump_fsm_edges() shadow-detection helpers, mirroring the existing
reportedIsWaitStandby field exactly) -- not by adding KeeperFSM[] rows,
since there is nothing safe to promote either state to.
prepare_maintenance additionally needed a new no-op row (pos 208): unlike
join_secondary (already recognized by node_metadata.c's
IsParticipatingInPromotion, so a lone node there safely no-ops via the
ordinary heartbeat no-match fallthrough), a lone prepare_maintenance node
is recognized by neither that function, IsBeingPromoted, nor
IsInPrimaryState. Without an explicit match, ProceedGroupStateFromContext's
own "couldn't find the primary node" guard would ereport(ERROR) on every
single subsequent heartbeat from that node -- strictly worse than the
original bug. pos 208 matches this exact "alone, reporting
prepare_maintenance" combination with a plain no-op (same shape as pos
205's own "converged to maintenance" row).
Add keeper_fsm_gap_209_prepare_maintenance.pgaf, verifying live that a lone
prepare_maintenance primary stays safely parked (goalstate never becomes
single) and keeps successfully checking in (reporttime advancing) rather
than looping on that error. join_secondary's own safety is left to the
existing static regress coverage (fsm.out/check_fsm_reachability.out/
keeper_fsm_edges.out, all regenerated) plus the code-level
IsParticipatingInPromotion confirmation -- pos 211's own
join_secondary/prepare_maintenance handling is intentionally untouched
(report_lsn is a far less dangerous target than single).
Regenerated fsm.out/check_fsm_reachability.out/keeper_fsm_edges.out from
the real monitor extension, and updated keeper_fsm_edges.sql's own
follow-up investigation comment. Verified: full regress (19/19) +
isolation (6/6) installcheck, citus_indent clean, all existing pos 209/211
pgaftest specs re-verified passing, new spec passing live.
…econdary
pos 211 ("alone in group, candidatePriority zero -> report_lsn") was
willing to assign report_lsn to a node reporting prepare_promotion,
demote_timeout, or join_secondary, but KeeperFSM[] had no matching row for
any of the three -- the exact same shape of gap fast_forward had, just
with a much lower-stakes target: report_lsn never grants write access, so
none of pos 209's split-brain argument applies here.
Add all three to KeeperFSM[] (fsm.c), all reusing fsm_report_lsn directly:
- PREP_PROMOTION_STATE -> REPORT_LSN_STATE: entering prepare_promotion
(fsm_prepare_standby_for_promotion) is a no-op -- Postgres is
untouched, still an ordinary streaming standby.
- DEMOTE_TIMEOUT_STATE -> REPORT_LSN_STATE: fsm_stop_replication already
sets default_transaction_read_only=on before this state is ever
reported, so no writes can have landed here that a real primary
elsewhere wouldn't also already have.
- JOIN_SECONDARY_STATE -> REPORT_LSN_STATE: Postgres was cleanly
checkpointed and stopped (fsm_checkpoint_and_stop_postgres) before
reaching this state -- a trustworthy copy of data that, unlike pos
209's own join_secondary concern, has nothing to have fallen behind
(candidatePriority=0 was never in the running to become primary).
fsm_report_lsn's own restart (standby_restart_with_current_replication_
source, primary_standby.c) handles all three uniformly: it stops Postgres
if running, rewrites the recovery config with no primary_conninfo, and
restarts -- it never needs to reach any peer.
pos 211's stop_replication current_state remains genuinely unfixed:
fsm_stop_replication doesn't just stop a process, it promotes the replica
(Postgres has already left recovery onto a new timeline by the time this
state is reported), and the only existing path back to an ordinary standby
(fsm_restart_standby -> fsm_rewind_or_init) hard-requires a live, reachable
primary via keeper_get_primary() -- which cannot exist by pos 211's own
"alone in group" precondition. Left as a documented, unfixed gap.
Verification for these three is static + code-level, not a live pgaftest
reproduction: fsm_prepare_standby_for_promotion's own no-op nature means
the monitor's cascade advances from assigned=prepare_promotion straight
through to stop_replication within the same heartbeat, before any external
test script's own peer-removal SQL can land in between -- a tighter race
than fast_forward's own genuine WAL-fetch delay gave room for. That live
attempt did incidentally confirm the stop_replication dead-end for real:
the candidate sat reporting stop_replication, endlessly reassigned
report_lsn by this same row, with no KeeperFSM[] row able to reach it.
Also, per this session's own investigation: pos 209 ("alone in group,
candidate-eligible -> single") no longer assigns single to a node
reporting join_secondary or prepare_maintenance -- both found to be a
genuine split-brain/data-loss risk (a different, already-promoted primary
can be live elsewhere by the time either state is reached), fixed by
excluding them from pos 209 directly rather than adding keeper rows.
prepare_maintenance additionally needed a new pos 208 no-op row: unlike
join_secondary (already recognized by node_metadata.c's
IsParticipatingInPromotion), a lone prepare_maintenance node wasn't
recognized by that function, IsBeingPromoted, or IsInPrimaryState, so
excluding it from pos 209 alone would have left
ProceedGroupStateFromContext's own "couldn't find the primary node" guard
to ereport(ERROR) on every subsequent heartbeat -- worse than the original
bug. Verified live via keeper_fsm_gap_primary_left_alone_mid_maintenance_
handoff.pgaf: a lone prepare_maintenance primary stays safely parked
(goalstate never becomes single) and keeps successfully checking in
(reporttime advancing) rather than looping on that error.
Renamed this session's own new pgaftest spec files away from per-rule
naming (keeper_fsm_gap_209_fast_forward.pgaf ->
keeper_fsm_gap_candidate_fast_forward_left_alone.pgaf,
keeper_fsm_gap_211_fast_forward.pgaf ->
keeper_fsm_gap_priority_zero_fast_forward_left_alone.pgaf,
keeper_fsm_gap_209_prepare_maintenance.pgaf ->
keeper_fsm_gap_primary_left_alone_mid_maintenance_handoff.pgaf),
re-verified passing under their new names, and updated every reference
(schedules, keeper_fsm_edges.sql's own comment).
Regenerated keeper_fsm_edges.json/expected/keeper_fsm_edges.out from the
real monitor extension. Verified: full regress (19/19) + isolation (6/6)
installcheck, citus_indent clean, all renamed and pre-existing pos 209/211
pgaftest specs re-verified passing live.
… rows
Replaces the "static + code proof only" verification this session
previously settled for on pos 211's prepare_promotion/demote_timeout/
join_secondary -> report_lsn fixes (fe71d50) with real, deterministic
live pgaftest reproduction of all three, using the new no-autopilot +
"fsm step <node>" DSL primitives:
- keeper_fsm_gap_priority_zero_candidate_left_alone_mid_promotion.pgaf
-- a lone standby left mid-promotion (secondary -> prepare_promotion)
after its only primary is killed.
- keeper_fsm_gap_priority_zero_primary_left_alone_mid_demotion.pgaf --
the primary itself left mid-demotion (primary -> draining ->
demote_timeout) after perform_failover() hands off to its standby.
- keeper_fsm_gap_priority_zero_losing_candidate_left_alone_mid_handoff.pgaf
-- the losing side of a genuine two-candidate MS-failover election
(report_lsn -> join_secondary), needing three nodes and both
standbys under step-mode control to pace the election by hand.
Each spec drives its node to converge locally to the state under test
via one explicit "fsm step", stops there -- frozen, unreported to the
monitor, for as long as the spec likes, since nothing ticks without an
explicit command -- zeroes that node's own candidate priority and
force-removes its former peers, then one further "fsm step" reports the
frozen state, receives the newly-applicable report_lsn assignment from
pos 211, and performs the transition under test in the very same call.
This is exactly the race that made live reproduction impossible last
session (fsm_prepare_standby_for_promotion's own no-op nature let the
monitor's cascade outrun any external test script's timing); step mode
removes the race instead of trying to win it.
Notable non-obvious findings from getting these live:
- A hard kill/removal of the primary never gives it a chance to
self-report (unlike a graceful shutdown, which reports
prepare_maintenance on its own way out and, as a side effect of
processing that report, cascades a fresh goal to standbys too) --
so nothing recalculates a standby's own goal state until the
standby itself makes contact. Each spec's kill step is followed by
a 30s sleep (letting the monitor's own health-check worker notice)
and then an explicit "fsm step", not a bare wait.
- That same "fsm step" call discovers a fresh assignment and performs
the transition into it in one shot -- there is no way to observe an
assignment without also applying it. This means priority-zeroing
and peer-removal must happen *before* the node's next contact, not
after: report_lsn's own precondition (pos 211) must already be true
the moment the node reports being in prepare_promotion/
demote_timeout/join_secondary, or the monitor's ordinary cascade
(e.g. prepare_promotion -> stop_replication, a dead end -- see this
file's own updated comment) wins instead.
- The "wait until X and Y timeout Ns" multi-condition form only
supports "state is", not "assigned-state =" -- a real grammar gap
(wait_multi_condition, test_spec_parse.y), worked around here with
separate single-condition waits rather than extending the grammar,
since only these new specs currently need assigned-state waits at
all.
Regenerated expected/keeper_fsm_edges.out for the updated comment (pg_regress
echoes .sql comments verbatim) via a targeted three-test pg_regress run
(create_extension, fsm, check_fsm_reachability, keeper_fsm_edges) rather
than the full schedule, which hit an unrelated pre-existing ordering
issue (dummy_update's fake extension version bleeding into an earlier
test) reproducible even with --no-cache -- not investigated further as
out of scope for this change. citus_indent clean; all three new specs
verified passing live multiple times each.
… states
Rule 325 (primary fails, not already wait_primary -> secondary ->
prepare_promotion, primary -> draining) matched its primaryNode role via
isInPrimaryState=TRUE, which admits SINGLE as one of the 5 states
CanTakeWritesInState allows. But this row's own precondition already
requires a second, distinctly-matched node (activeNode, in SECONDARY
state) to exist in the same group -- so primaryNode genuinely reporting
SINGLE ("alone in my own group") is a real model contradiction, not just
an unlikely scenario: the instant a second node registers, the primary's
own goal moves off single before the joining node could ever reach
secondary.
dump_fsm_edges() had no way to know this: NodeStatePatternResolveFromStates()
only ever reads a row's own .statePattern, so isInPrimaryState's own
5-state set (the field responsible for admitting SINGLE at all) was
invisible to it by construction, and Step 2a's keeper/monitor gap
comparison spuriously flagged primary->single as an unexplored edge.
Fix in two parts:
- pos 325 now spells out the invariant explicitly via
.conditions.groupHasExactlyOneNode = BOOL_FALSE.
- StateCanSatisfyIsInPrimaryState() gains a new singleExcluded parameter,
fed by a new MonitorFSMTransitionExcludesSingleNode() helper that reads
groupHasExactlyOneNode/groupHasMoreThanTwoNodes off a rule's own
.conditions, and excludes SINGLE from the reachable state set whenever
either is set.
pos 391 (MS-failover cascade) needed no rule change: it already carried
groupHasMoreThanTwoNodes = BOOL_TRUE for an unrelated reason (its own
"more than two nodes" gate), so it benefits from the same narrowing for
free.
Regenerated fsm.out, check_fsm_reachability.out (194 -> 192 total edges),
and keeper_fsm_edges.out (Step 2a gap rows 82 -> 78, pos 325's and 391's
single-state gaps both gone); updated keeper_fsm_edges.sql's own
commentary to match. Verified: full regress (19/19) + isolation (6/6)
suites pass with zero unexpected diffs, multi_standbys.pgaf (27/27)
confirms the MS-failover cascade is unaffected, citus_indent clean.
…PED exclusion Rules 333/339/347/349/351 all match primaryNode/otherNode against whatever node GetPrimaryOrDemotedNodeInGroupFromList() resolves -- called once at the top of ProceedGroupStateFromContext() and threaded unchanged through every nested dispatch, including the MS-failover cascade. That resolver can never return a node reporting DROPPED: its own two-phase logic excludes it outright (phase 1 requires a writable goalState; phase 2's fallback target set doesn't include it either), and it's structurally unreachable besides -- a node's reportedState only becomes DROPPED once its own goalState already is, and pos 201 (early_checks) deletes that row from the catalog atomically, in the very same node_active() call that converges it, so a DROPPED-reporting node never persists long enough for a later node's own call to see it. dump_fsm_edges() didn't know this: it enumerated the full, unconstrained reportedState universe for any row whose primaryNode/otherNode pattern doesn't otherwise narrow it (true of all 5 of these rows), spuriously flagging primaryNode=dropped as an unexplored Step 2a gap. Fixed via a new, unconditional filter, PrimaryNodeReportedStateCanBeResolved(), applied only to the primaryNode/otherNode candidate-state loop (never activeNode's own, where DROPPED is an entirely ordinary current_state -- see pos 201). Investigated whether these same 5 rows' SINGLE gap could be closed the same way pos 325/391 were (via .isInPrimaryState + groupHasExactlyOneNode): it can't. Unlike pos 325/391, none of these rows requires primaryNode's own convergence (isInPrimaryState), so a primary that converged to SINGLE, then had a second node register (bumping its own *goal* to WAIT_PRIMARY as part of that registration), then died before ever reporting the new goal, leaves a row GetPrimaryOrDemotedNodeInGroupFromList() would still resolve as primaryNode -- a genuinely reachable case, not a false positive. Deliberately left unnarrowed and documented as such, rather than force a real, behavior-changing isInPrimaryState requirement onto rules that don't actually need one. Regenerated check_fsm_reachability.out (192 -> 187 total edges) and keeper_fsm_edges.out (Step 2a's 5 remaining wide-gap rows drop one DROPPED row each, 78 -> 73); updated keeper_fsm_edges.sql's own commentary to match (71 -> 66 detail rows) and to explain both the DROPPED fix and why SINGLE stays. Verified: full regress (19/19) + isolation (6/6) pass with zero unexpected diffs, multi_standbys.pgaf (27/27) confirms the MS-failover cascade is unaffected, citus_indent clean.
…ONDARY exclusion Reviewing rule 333's remaining Step 2a gap states surfaced two more false positives, on top of the DROPPED exclusion fixed previously. GetPrimaryOrDemotedNodeInGroupFromList()'s phase 1 admits any reportedState paired with a writable goalState -- pos 209's own broad "alone in group -> SINGLE" row (which fires for almost any reportedState) is what makes most of 333/339/347/349/351's remaining gap states genuinely reachable via a stale/unconverged primary. But pos 209 itself explicitly excludes 3 states for split-brain/data-loss reasons: WAIT_STANDBY, JOIN_SECONDARY, and PREPARE_MAINTENANCE. PREPARE_MAINTENANCE stays reachable via the resolver's own phase 2 fallback regardless. WAIT_STANDBY and JOIN_SECONDARY are not -- and an exhaustive grep confirms no other row in the whole table ever assigns a writable goal to a node reporting either (pos 315/317/319 assign only CATCHINGUP from WAIT_STANDBY; pos 359/361 assign only SECONDARY from JOIN_SECONDARY). So the resolver can never actually select such a node, and dump_fsm_edges() had no way to know it. Extended PrimaryNodeReportedStateCanBeResolved() to exclude both, alongside DROPPED. Unlike DROPPED's structural argument, this one rests on the current table's own contents (documented in the function's own comment, with a note to revisit if a future row is ever added assigning a writable goal from either state). Regenerated check_fsm_reachability.out (187 -> 177 total edges) and keeper_fsm_edges.out (Step 2a's overall total 73 -> 63 rows; the 333/339/347/349/351 cohort specifically drops 2 detail rows each, 66 -> 56); updated keeper_fsm_edges.sql's own commentary to match. Verified: full regress (19/19) + isolation (6/6) pass with zero unexpected diffs, multi_standbys.pgaf (27/27) confirms the Citus-worker/MS-failover paths are unaffected, citus_indent clean.
…ort/advance
Adds 23 KeeperFSM[] rows (fsm.c) covering the "presumed dead primary"
transition to DEMOTED/DEMOTE_TIMEOUT from every reachable current_state
identified by dump_fsm_edges() for rules 333/339/347/349/351, reusing the
existing role-agnostic fsm_stop_postgres action. This closes the last
Step 2a gap rows for those five rules (keeper_fsm_edges.json/.out/.sql
regenerated accordingly).
Also splits "pg_autoctl manual fsm step" into two independently-issuable
halves:
- `pg_autoctl manual fsm step report` calls node_active() and persists
the monitor's newly assigned goal state, without attempting the local
transition.
- `pg_autoctl manual fsm step advance` performs the transition already
on file, without an extra monitor round trip.
The combined command reports and immediately attempts whatever transition
it was just assigned, atomically, in one call -- there's no way to observe
(or hold a node frozen at) the moment in between. That in-between moment is
exactly what live-testing several of these gap rows requires: a node
needs to report its current state and have the monitor bump its goal
forward, without immediately racing off to reach it. Implemented as two
new keeper_fsm_step_report/_advance functions (fsm.c/fsm.h), wired through
the existing step-mode socket protocol (step_socket.c/h: new REPORT/ADVANCE
commands alongside STEP) and its server-side dispatch (service_keeper.c),
and exposed on the CLI (cli_do_fsm.c).
tests/tap/specs/fsm_step_report_advance.pgaf proves the split live: after
forcibly removing a node's only peer, `... report` shows the monitor
bumping goalState to single while reportedState stays frozen at
catchingup, and a subsequent `... advance` performs that transition.
Registered in tests/tap/schedule and schedules/node.sch.
Pure whitespace/line-wrapping fix (long lines rewrapped, a couple of compound-literal braces reformatted to the canonical multi-line style, one missing blank line between top-level declarations). No semantic change -- confirmed via 'git diff -w' showing only wrapping/brace-style hunks, and a clean rebuild. citus_indent --check now passes with zero failures across the whole tree.
Adds the missing KeeperFSM[] row (fsm.c) reusing fsm_report_lsn directly,
closing MonitorFSM[] rule 211's only remaining gap ("alone in group,
candidatePriority zero -> report_lsn", stop_replication current_state).
This was previously left as a documented, believed-unfixable gap on the
mistaken assumption that reaching report_lsn from stop_replication would
need the same live-primary-dependent rewind/basebackup machinery used
elsewhere (fsm_restart_standby/fsm_rewind_or_init, for reaching
catchingup -- a materially different target that genuinely does need to
stream from someone). It doesn't: fsm_report_lsn's own restart
(standby_restart_with_current_replication_source) is called with an
all-zeroed upstream, so it never contacts a peer at all -- it just stops
Postgres, writes a disconnected standby.signal, and restarts, exactly
like every other source state already reusing this same action. Postgres
itself decides "am I in recovery" purely from standby.signal's presence
at this startup, not from the fact that fsm_stop_replication had already
promoted it onto a new timeline moments earlier.
Verified live with two pgaftest specs covering both documented ways out
of the resulting parked report_lsn state (group_state_machine.c's own
pos 211 comment):
- keeper_fsm_gap_stop_replication_report_lsn_priority.pgaf: raising
candidate-priority back above 0 promotes straight to single via the
already-existing pos 209 + report_lsn -> single edge, with nothing
auto-promoting the node while priority stays 0.
- keeper_fsm_gap_stop_replication_report_lsn_new_node.pgaf: a new node
registers, basebackups from the parked node via RegisterNode's
existing report_lsn-candidate-priority-0 special case, and takes
over as primary while the parked node follows it back in as a
secondary (report_lsn -> secondary directly, not via join_secondary
as first assumed).
keeper_fsm_edges.json/.out and the sql comment narrative regenerated and
updated accordingly; both new specs registered in tests/tap/schedule and
schedules/node.sch.
The Step 2a/2b comments had accumulated a full narrative of every investigation and fix across many sessions (pos 303, 325, 333/339/347/ 349/351, 209/211's fast_forward/join_secondary/prepare_maintenance/ stop_replication, ...). None of that is needed to understand what this test does or how to read its output, and since expected/keeper_fsm_edges.out echoes the .sql file's own comments verbatim, every one of those historical asides was also a latent maintenance trap: editing a comment without regenerating the expected file breaks the test on the next run (as just happened here). Trimmed both files down to what a reader actually needs: what Step 1's fixture load and "any" sentinel do, what a non-empty Step 2a/2b result means, and the one standing expected exception (Step 2b's "any -> dropped" row). 398 lines of sql/keeper_fsm_edges.sql down to 116; no change to query logic or expected results otherwise.
The comment above the node schedule's matrix entries still listed its original contents from before #1183 (fsm_step_report_advance) and this branch's own FSM edge-gap specs were appended to tests/tap/schedules/node.sch.
NodeStatePatternResolveFromStates's NODE_STATE_ANY/ASSIGNED/NOT_ASSIGNED branch used memcpy() to copy AllReplicationStates; its own sibling branches in the same function already do this via an explicit element-by-element loop, so match that instead of reaching for IGNORE-BANNED. DispatchMonitorFSMRule's rule->comment copy used snprintf(...,"%s", ...) where strlcpy() (already used elsewhere in this same file for fixed-buffer string copies) is the simpler, non-banned fit.
…its CI coverage Adds two sections to failover-state-machine.rst, right after the existing keeper FSM mermaid diagrams: - "The monitor's FSM: pgautofailover.fsm" -- sample query output (both a narrow tabular slice and a full \x record), and how section_path (ltree) supports hierarchical queries. - "Cross-checking the monitor and keeper FSMs" -- explains dump_fsm_edges()/check_fsm_reachability()/pg_autoctl inspect fsm check, and the "How this is tested in CI" subsection describing the three regress tests (fsm.sql, check_fsm_reachability.sql, keeper_fsm_edges.sql) that run on every installcheck, for every PG version, as part of the build_run_images job. Also refreshes pg_autoctl_inspect.rst's "pg_autoctl inspect fsm" sub-command listing, which still only showed state/list/gv -- missing check and mermaid entirely.
dimitri
added
enhancement
CI failed on PG19 only (build_test_image / PG19): 'monitor' and 'timeline_fork_detection' both diffed against src/monitor/expected/pg19/expected/*.out, the PG19-specific override directory (--expecteddir, used because PG19 changed pg_lsn display format; pg_regress falls back to the shared expected/ dir for every file not present there). Both overrides were stale, from before this branch's own additions: - monitor.out was missing the trailing blank line after last_events_by_formation_and_group_count_ok, added by this branch's own last_events() coverage. - timeline_fork_detection.out still had the old sequential node IDs (24-30); this branch's new candidate_count_gate test runs earlier in regress_schedule and registers additional nodes, shifting every later test's node_id sequence values (27-33 now). Reproduced locally via a manual pg19 pg_virtualenv + installcheck run against a pgaf-base:bookworm container (mirroring the Dockerfile's own build stage), confirmed the actual output matches CI's reported diff exactly, and regenerated both files from that actual output. Re-ran the same installcheck afterward: all 19 regress + 6 isolation tests pass.
node.sch's own header comment already measured ~30 min for its original 14 specs before this branch ever touched it, against the CI step's 20-minute timeout. This branch's own keeper/monitor FSM edge-gap work appended 14 more specs at the end, and CI run 83400372049 confirmed the result: all 6 'pgaftest / node' matrix jobs (PG14-19) timed out at 20 minutes, consistently stalling around spec #20-21/28 -- a cumulative time-budget overrun, not one stuck test. Split the 14 newly-added specs (keeper_fsm_gap_* and the two stop_replication specs) into a new node-fsm-gaps.sch, run PG17-only -- the same convention ci.yml already uses for multi-alternate/multi-misc/ multi-async/citus-1/citus-2 ("FSM logic, not version-specific code paths"). node.sch itself reverts to its original 14 specs, running on all 6 PG versions as before.
…ew FSM SQL objects to the 2.2->2.3 script
CI run 83400372049's 'pgaftest / upgrade' job failed deterministically:
ALTER EXTENSION "pgautofailover" UPDATE TO "2.3":
ERROR: required extension "ltree" is not installed
pgautofailover.control's requires line gained ltree (needed for
pgautofailover.fsm's section_path::ltree column), but Postgres only
auto-resolves 'requires' on a fresh CREATE EXTENSION, never on ALTER
EXTENSION ... UPDATE -- and critically, it checks 'requires' against
what's already installed *before* it ever runs the target version's
upgrade script body. A 'CREATE EXTENSION IF NOT EXISTS ltree' placed
inside pgautofailover--2.2--2.3.sql itself is therefore unreachable
dead code: the ALTER EXTENSION statement already fails before that
script line would ever execute.
The real, working fix is client-side, in monitor_extension_update()
(monitor.c): it already has this exact pattern for btree_gist, added
years ago when 1.4 first required it ("It does not seem like Postgres
knows how to handle changes in extension control requires, so let's do
that manually here"). Added the same CREATE EXTENSION IF NOT EXISTS
ltree bootstrap, gated the same way (targetVersionNum >= 203, matching
the existing >= 104 convention).
Verified end to end against a real v2.2 -> current upgrade
(tests/upgrade/Makefile, which builds pgaf:current from the v2.2 git
tag and pgaf:next from this branch): all 10 steps of upgrade.pgaf now
pass, including test_006_verify_extension_version (confirms the
extension actually reaches 2.3) and test_007_verify_data_intact.
Also ported this branch's other new pgautofailover.sql objects into
pgautofailover--2.2--2.3.sql, which had none of them (dump_fsm(),
dump_fsm_edges(), check_fsm_reachability(), the pgautofailover.fsm view,
pgautofailover.fsm_section enum, and event.rule_pos/rule_section) --
confirmed via 'git diff origin/main HEAD -- pgautofailover.sql', the
exact 206-line diff this branch introduces there. Without this, even
with the ltree fix, an in-place upgrade would have reached 2.3 missing
this entire declarative-FSM SQL surface.
commit b3a64ec renamed the pgaftest DSL modifier from no-autopilot to suspended (lexer/grammar/compose_gen), but 5 specs authored around the same time were never updated. Since "no-autopilot" is no longer a recognized token, the parser fell back to treating it as a second, independent node identifier on the same formation line -- producing a phantom extra container (and, when two lines both said "no-autopilot", a duplicate docker-compose.yml service key that failed to parse entirely). This is what CI run 30759600065 surfaced as spurious timeouts and a YAML parse error in the newly-split node-fsm-gaps schedule. Verified live (Docker): 4 of the 5 specs now pass in full -- keeper_fsm_gap_priority_zero_candidate_left_alone_mid_promotion (3/3), keeper_fsm_gap_priority_zero_primary_left_alone_mid_demotion (6/6), keeper_fsm_gap_stop_replication_report_lsn_priority (4/4), and keeper_fsm_gap_priority_zero_losing_candidate_left_alone_mid_handoff (5/5). keeper_fsm_gap_stop_replication_report_lsn_new_node's first 3 steps now pass too, but its test_004 still fails for an unrelated, deeper reason: node4 never sees node3's replication reach Postgres's own "quorum"/"sync" sync_state, so it retries "wait_primary -> primary" forever. Not a step-count/timing issue (confirmed live: adding extra staggered "fsm step node3" calls during the wait did not help). Left for follow-up investigation.
…nating the assigned-state race CI run 30759600065 confirmed the previous design was still genuinely racy even after switching from reported-state to assigned-state polling: with a WAL gap this small (5000 rows), node3's own local fetch completes and reports back within about 2 seconds of the assignment landing, and the monitor's own cascade (driven by node2's regular ticking) advances assigned-state on to prepare_promotion before the external "wait until" poll can reliably observe fast_forward in between. Rebuilt both specs around a suspended (step-mode) node3 instead: nothing it reports to the monitor changes except in direct response to an explicit "fsm step node3" command, which removes the race structurally rather than narrowing it. Confirmed directly against FSM_REPORT_LSN_OR_FAST_FORWARD (group_state_machine.c): the cascade past fast_forward requires reportedState == goalState == FAST_FORWARD, so as long as node3 itself never contacts the monitor, the assigned goal simply cannot advance past whatever it currently is. Discovered live in the process that the very first assignment isn't fast_forward either: the monitor can't decide fast_forward vs. a direct promotion without first learning node3's own actual LSN, which needs a real contact from node3 -- so the first goal that lands (driven by node2's own regular ticking) is the ordinary standby fan-out to REPORT_LSN, and stays there until node3 itself reports in. Both specs now drive node3 through two explicit "fsm step" calls before removing peers (report_lsn assignment -> act on it -> report_lsn reported with real LSN -> fast_forward assignment -> act on it), matching what's actually observed live rather than the originally assumed single hop. Verified live in Docker, 4/4 runs each, both specs passing consistently with no timing variance.
Suspended (step-mode) nodes' node-active loop, keeper_suspended_loop(), calls keeper_fsm_step()/keeper_fsm_step_report() directly, bypassing service_keeper_node_active() -- which is where the only self-heal for groupId/replication_slot_name drift used to live inline. Autonomous nodes get this repair on every tick; suspended nodes never did. This stayed invisible for as long as a suspended node kept following the same primary it originally registered against. It surfaces the first time such a node is later assigned to follow a *different* primary (REPORT_LSN_STATE -> SECONDARY_STATE via fsm_follow_new_primary, e.g. the "a new node joins and takes over" recovery path out of a parked, candidate-priority-zero report_lsn state): with no self-heal having ever run for it, config->replication_slot_name can be empty by that point, so prepare_recovery_settings() writes no primary_slot_name at all, Postgres streams without a slot, and the new primary's own quorum-candidate query (pgsql_get_postgres_metadata, an inner join from pg_replication_slots to pg_stat_replication on active_pid) can never match a slot-less connection -- so it retries "wait_primary -> primary" forever, logging "we don't have a quorum candidate yet" (fsm_enable_sync_rep) even though a healthy, caught-up standby is right there. Root-caused live: confirmed via direct SQL that the standby's replication connection was genuinely active and caught up (pg_stat_replication.sync_state = quorum) but not attached to its own named slot (pg_replication_slots.active = false), and that SHOW primary_slot_name on the standby came back empty. Fixed by extracting the self-heal block into a new shared function, keeper_maybe_update_group_and_slot(), and calling it from keeper_fsm_step() and keeper_fsm_step_report() (fsm.c) -- the two functions a suspended node's every step actually goes through -- in addition to service_keeper_node_active() (service_keeper.c), whose own call site is now a straight extraction with identical behavior for the autonomous path. Verified live in Docker: keeper_fsm_gap_stop_replication_report_lsn_new_node.pgaf's previously-hanging test_004 now passes in ~22s instead of timing out at 90s+ (3/3 consecutive runs). Full node-fsm-gaps.sch schedule (all 14 specs) passes 14/14 with this change in place.
keeper_maybe_update_group_and_slot() (added in deae2b9) called the full keeper_ensure_configuration() to repair a drifted groupId/replication slot name. That function does more than write the config file: when state->current_role is CATCHINGUP/SECONDARY/MAINTENANCE, it also reconfigures live standby settings (primary_conninfo) by calling keeper_get_primary(). That's unsafe from this self-heal, because it can run *before* the FSM transition that would bring current_role up to date -- in particular keeper_fsm_step_report() reports without transitioning. A node that just lost its primary can still have current_role reflecting the old, now-removed primary at the moment the self-heal fires, so keeper_ensure_configuration() tries to reconnect Postgres to a stale/nonexistent primary and fails outright. This was pre-existing latent behavior on the autonomous path too (service_keeper_node_active's original inline block made the same call, unconditionally on postgresNotRunningIsOk=false) -- deae2b9's extraction just made the self-heal fire from more call sites, giving this pre-existing gap more chances to trigger. All the original replication-slot bug needed was a correct config->replication_slot_name the next time fsm_follow_new_primary() reads it, which a plain keeper_config_update() already provides. Drop the keeper_ensure_configuration() call and its now-unneeded postgresNotRunningIsOk handling entirely. Verified locally before pushing (CI run 83427068405 regressed on the previous version of this fix across node.sch and several multi/citus pytest suites): - fsm_step_report_advance.pgaf: 4/4, 3 consecutive runs - maintenance_and_drop.pgaf: 8/8 - node-fsm-gaps.sch (14 specs, including the original hanging test_004_node4_joins_and_becomes_primary): 14/14 - pytest multi group: 101/101 - pytest test_citus_force_failover.py (including the previously failing test_005_drop_primary_worker): 7/7 - citus_indent, ci/banned.h.sh: clean
Full pass over every comment in this file. Two problems, fixed together: - Several comments referenced an external design doc used to plan this refactor -- fine as working notes, but not part of this PR, so citing it left a reader with no way to check the claim. Rewrote each to state the fact directly instead of deferring to that document. - Comments were written as a migration narrative: "the original if-chain", "the real source", "an earlier version of this file had X and it broke", stale self-referential line numbers pointing at code this refactor already removed, dead symbol names (MonitorFSM_EarlyChecksStart and friends, ActionCatchupUnhealthySecondaries) left over from before the sectionPath mechanism replaced them, and cross-file line-number citations to node_active_protocol.c/ formation_metadata.c that had already drifted 100-250 lines out of date. Rewrote all of this in present tense, describing what the current table/functions do and why, the way a docstring should read -- not a commit-message account of how it got here. That history belongs in git log, not in the comments. No behavior change. Verified: clean compile, citus_indent, banned.h.sh.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the monitor's hand-written
if/AssignGoalStatedispatch chain inProceedGroupState*()(src/monitor/group_state_machine.c) with a single declarative table,MonitorFSM[]— 74 rows(ish), one per transition rule, each with an explicit precondition (NodeStatePatterns, boolean conditions, anIntPatternfor the 3 MS-failover counting gates) and assigned goal state(s). The client-sideKeeperFSM[](src/bin/pg_autoctl/fsm.c) gets the matching treatment: 23 new rows closing a presumed-dead-primary → demoted/demote_timeout gap, plus several smaller edge-gap fixes (see below).Built on top of the declarative table:
pgautofailover.fsm(view) /pgautofailover.dump_fsm()— read-only SQL introspection of the whole monitor rule table, including a hierarchicalsection_path(ltree) column.pgautofailover.dump_fsm_edges()/pgautofailover.check_fsm_reachability(jsonb)/pg_autoctl inspect fsm check— a reachability cross-check: does every transition the monitor can assign have a matchingKeeperFSM[]edge? Run automatically in CI (regress testsfsm.sql,check_fsm_reachability.sql,keeper_fsm_edges.sql) for every supported PG version, and available as a live CLI command against a real cluster.pg_autoctl inspect fsm mermaid— the keeper FSM rendered as 5 phase-split Mermaid diagrams, generated fromKeeperFSM[]directly (replaces a hand-maintainedfsm.pnglast regenerated in 2021). Seedocs/failover-state-machine.rst.Why
The hand-written dispatch had accumulated real gaps over time — several already fixed reactively, one at a time, after a user hit them in production (see the "Related issues" table below). Turning the rule set into data makes every rule's precondition and every keeper edge auditable and mechanically cross-checkable, instead of relying on someone noticing a missing
ifbranch during review.What's genuinely new (vs. what's just the refactor)
Everything here is new work not yet on
main— this branch was never previously pushed. Two closely-related pieces of it were already extracted, reviewed, and merged separately: pgaftest step-mode/suspendedtooling and thefsm step report/advancesplit (#1183), and acitus_indentDocker worktree-mount fix (#1184). This PR is what's left.MonitorFSM[](group_state_machine.c, +5,620/−1,217),MonitorFSMSection/MonitorFSMSectionPathhierarchy,IntPatterngates for the 3 MS-failover counting checks (missingNodesCount/candidateCount/quorumCandidateCount),dump_fsm()/dump_fsm_edges()/check_fsm_reachability().KeeperFSM[]additions (fsm.c, +463): 23 new DEMOTE_TIMEOUT/DEMOTED rows, plus thekeeper_fsm_step_report/_advancefunctions (already merged in pgaftest/pg_autoctl: step-mode tooling (no-autopilot, fsm step, report/advance split) #1183's renamedsuspendedform — this branch's own copy is what those PRs were extracted from).candidate_count_gate.sql/out,cluster_init_failover_rule_attribution, updatedfsm.out/keeper_fsm_edges.out/check_fsm_reachability.out.keeper_fsm_gap_*.pgafspecs wired into thenodepgaftest schedule (runs on every supported PG version), covering: rule 209/211 wait_maintenance/wait_standby/fast_forward/join_secondary gaps, priority-zero primary self-demotion, rule 325 SINGLE exclusion, and the rule 211stop_replication → report_lsngap.rule_pos/rule_sectiondisplay inpg_autoctl show state --jsonandwatch; two banned-API cleanups (memcpy→loop,snprintf→strlcpy, matching this file's own existing conventions); a stale CI schedule comment; and the two newdocs/failover-state-machine.rstsections described above, plus a stalepg_autoctl inspect fsmsub-command listing fix (check/mermaidwere undocumented).Related issues
These four issues are already fixed and already merged into
main(this branch is built on top of them) — listed because they're the same recurring bug class this refactor's tooling generalizes a fix for: an FSM rule assigning a goal state the keeper has no edge for, historically only ever caught by an operator hitting it in production.wait_primary → draininghad no keeper edge. Fixed (#1169), predates this branch.IsFailoverInProgressguard caused an unreachable goal-state assignment. Fixed (#1165), predates this branch.demote_timeout → wait_primarytransition missing, self-fenced primary deadlock. Fixed (#1158), predates this branch.stop_replicationunreachable fromwait_primary. Fixed (older PR #909), predates this branch.Using
pg_autoctl inspect fsm check/ thekeeper_fsm_edges.sqlregress test during this branch's own development is what surfaced the 23-rowKeeperFSM[]gap, the rule 209/211 gaps, and the rule 211stop_replication→report_lsngap listed above — none of which had ever been reported; they were caught before a user hit them, not after.#482 (from 2020) — investigated in detail, closed separately, not part of this PR's diff. "Dropping nodes during
join_primaryresults in illegal state transition": the monitor could assignapply_settingsto a primary currently reported atjoin_primary, whichKeeperFSM[]has no edge for.set_node_candidate_priority()/set_node_replication_quorum()/set_formation_number_sync_standbys()only excluding "alreadyapply_settings", neverjoin_primary) is unchanged inmaintoday, and preserved byte-for-byte by this branch's declarative rows (MonitorFSM[]pos 125/127/129) — not fixed, not worsened by this refactor either way.dump_fsm_edges()deliberately skips the entireapi_triggeredsection (rows whose real precondition is hand-written C ahead of dispatch, not a declarativeNodeStatePattern) — a documented, structural blind spot, not an oversight.join_primaryitself was removed from normal operation in v1.6.2 (September 2021, Get rid of the JOIN_PRIMARY state. #796) — after this issue was filed. The monitor hasn't assignedjoin_primaryas a goal state since, so a node's reported state can no longer becomejoin_primarythrough the normal join flow this issue's timeline shows, and the specific race it describes can't occur on a cluster without carried-over pre-1.6.2 on-disk state. Closed Dropping nodes duringjoin_primaryresults in illegal state transition #482 directly with that explanation rather than folding a fix in here.Verification
Re-run against this exact branch tip just before opening this PR:
make -C src/monitor/make -C src/bin/pg_autoctl— clean, no warnings.citus_indent(make docker-check) — passes.ci/banned.h.sh— passes.installcheck(PG17,pgaf:run-pg17,make -f Makefile.docker build-pg17) — all 19 regress tests + 6 isolation tests pass, includingfsm,check_fsm_reachability, andkeeper_fsm_edges; image builds clean end to end.Verified live earlier in this branch's development (not re-run at this exact tip, but unaffected by the changes since):
pg_autoctl inspect fsm checkagainst a live cluster, and the newkeeper_fsm_gap_*/fsm_step_report_advancepgaftest specs.