Track connected Postgres server version and Citus extension version#1157
Merged
Conversation
pgautofailover.node had no version column at all -- only the keeper's local state file tracked control-data version info (pg_control_version/ catalog_version_no), never the running server's own server_version_num/ server_version/version(), and Citus's installed version wasn't tracked anywhere in this codebase. Adds four nullable columns: pg_versionnum (int), pg_version (text), pg_versionstring (text), citus_version (text, NULL whenever Citus isn't installed). Design choices, each a deliberate deviation from the initial plan after digging into the actual code: - Three plain scalar columns, not a composite type. No precedent exists anywhere in this codebase for a composite CREATE TYPE used as a table column or SPI parameter (the only CREATE TYPE is the replication_state enum) -- would have been a first, for no real benefit over matching the existing all-scalar convention (region, sysidentifier, ...). - A dedicated, separate SQL API (pgautofailover.report_postgres_version), not threaded through node_active(). Postgres/Citus version can't change without a Postgres restart, so there's no reason to carry it on every few-second periodic report; it's fetched and reported once per restart instead, keeping node_active()'s hot path untouched. - Fired at both keeper start and any later detected Postgres restart, not just keeper start. This surfaced a real bug worth documenting: a naive "compare pgIsRunning to its previous value" edge check is unreliable, because LocalPostgresServer.pgIsRunning is also written directly by fsm_transition.c/primary_standby.c during FSM transitions (e.g. the very first init -> single transition), outside of keeper_update_pg_state() entirely. By the time keeper_update_pg_state() next ran, pgIsRunning could already read true from one of those other writers, making the most important edge -- Postgres coming up for the first time -- invisible. Fixed with a dedicated tracker (PostgresVersionInfo.lastKnownRunning) that only keeper_update_pg_state() itself ever writes, decoupled from every other writer of pgIsRunning. Monitor side: pgsql_get_postgres_version() (src/bin/common/pgsql.c) runs one query (server_version_num/server_version/version(), plus Citus's extversion via a scalar subquery -- NULL when Citus isn't installed) on the not-running -> running edge; monitor_report_postgres_version() (monitor.c) sends it via the new SQL function, deliberately not STRICT so the four fields can be NULL; ReportAutoFailoverNodeVersion() (node_metadata.c) does the plain SPI UPDATE, no lock or existence check needed -- a keeper self-report on its own already-known nodeid, same posture as ReportAutoFailoverNodeState()'s own report path (silent no-op on an unknown node, unlike operator-facing commands like set_node_region). Tests: extended the existing node_active_protocol.sql/.out with direct SQL coverage of report_postgres_version() (NULL handling, unknown/null node_id); new postgres_version_tracking.pgaf pgaftest spec exercises the real keeper-side edge detection end to end against a live node, added to the quick schedule. Verified config_get_set.pgaf and the full 27-step basic_operation.pgaf are unaffected by the keeper_update_pg_state() change.
dimitri
added a commit
that referenced
this pull request
Jul 26, 2026
CI's upgrade.pgaf job failed: an in-place ALTER EXTENSION UPDATE TO '2.3' left several functions this branch (and two other already-merged PRs) added to the fresh-install script completely absent from the incremental 2.2--2.3 upgrade script, so any node calling them post-upgrade got 'function does not exist'. Ports over, matching the fresh-install definitions in pgautofailover.sql exactly: - set_node_region() (#1156) - the four pg_version*/citus_version columns and report_postgres_version() (#1157) - node_timeline_history / accepted_timeline tables and report_timeline_history() / accept_timeline() / resolve_accepted_timeline() / node_timeline_status() (this branch) Verified with make -C tests/upgrade all (builds pgaf:current from the v2.2 tag and pgaf:next from this branch, then runs tests/tap/specs/upgrade.pgaf end to end against a live 3-node cluster) -- all 10 steps pass, including the convergence step that previously failed.
dimitri
added a commit
that referenced
this pull request
Jul 27, 2026
CI's upgrade.pgaf job failed: an in-place ALTER EXTENSION UPDATE TO '2.3' left several functions this branch (and two other already-merged PRs) added to the fresh-install script completely absent from the incremental 2.2--2.3 upgrade script, so any node calling them post-upgrade got 'function does not exist'. Ports over, matching the fresh-install definitions in pgautofailover.sql exactly: - set_node_region() (#1156) - the four pg_version*/citus_version columns and report_postgres_version() (#1157) - node_timeline_history / accepted_timeline tables and report_timeline_history() / accept_timeline() / resolve_accepted_timeline() / node_timeline_status() (this branch) Verified with make -C tests/upgrade all (builds pgaf:current from the v2.2 tag and pgaf:next from this branch, then runs tests/tap/specs/upgrade.pgaf end to end against a live 3-node cluster) -- all 10 steps pass, including the convergence step that previously failed.
dimitri
added a commit
that referenced
this pull request
Jul 27, 2026
…itchover (#1161) * Part 1: verify genuine timeline ancestry at the secondary gate, not just numbers Fixes #683. standby_check_timeline_with_upstream() only ever compared timeline numbers (upstreamTimeline >= localTimeline): it cannot tell "genuine ancestor, just behind" apart from "diverged, but the timeline number still happens to be lower" -- a standby whose own WAL replay has gone past the point where its timeline was actually superseded looks identical to one that's simply still catching up, as long as its number is lower. The former can never resolve through ordinary streaming replication and needs pg_rewind; the latter just needs to wait. Add standby_verify_timeline_ancestry(), which walks the upstream's own TIMELINE_HISTORY data -- already fetched into replicationSource.system.timelines by the pgctl_identify_system() call standby_check_timeline_with_upstream() already makes, so this needs no new network round trip -- and confirms the local checkpoint LSN lies strictly before the point where the local timeline was superseded in the upstream's lineage. When it doesn't (genuine divergence), invoke the existing primary_rewind_to_standby() and retry the metadata fetch, rather than retrying the same doomed comparison forever. * Part 2 (schema + publishing): node_timeline_history table and reporting Adds the shared infrastructure a report_lsn election needs to reason about timeline ancestry across nodes, since there is no elected primary at that point to check against -- that has to live on the monitor. - pgautofailover.node_timeline_history: append-only (nodeid, tli, parenttli, switchpoint_lsn) facts, unioned across every node's own history. pgautofailover.report_timeline_history(node_id, history jsonb) upserts a batch via ON CONFLICT DO NOTHING (immutable facts, safe to resend). Written as plpgsql rather than C specifically to lean on jsonb_array_elements() instead of hand-rolled JSONB C API code. - pgautofailover.accepted_timeline: the (empty by default) operator pin table part 3 will use; added now since it's part of the same migration. - timeline_history.c/.h (new): reads a node's own pg_wal/<tli>.history file directly (no replication connection needed) and encodes it as the JSON array report_timeline_history expects. - Keeper wiring, two call sites per the design: every tick in the main loop, unconditionally of FSM state (an uneventful secondary picks up a timeline switch through ordinary streaming replication with no fsm_* transition ever running, so this can't be hooked off transitions alone), gated by an in-memory watermark so it's a no-op once caught up; and once more, synchronously, immediately before fsm_report_lsn()'s standalone restart -- the riskiest moment, since a genuine divergence can hard FATAL there and the node never gets to report anything again. The election itself doesn't consult this table yet -- that's next. * Part 2 (election): filter report_lsn candidates by timeline ancestry The election never actually used timeline at all, despite a comment on WalDifferenceWithin() claiming it does: ListMostAdvancedStandbyNodes() and SelectFailoverCandidateNode() both sort/pick purely by raw reportedLSN. A node on a genuinely forked, diverged timeline compares as "more advanced" exactly like a node that's simply further along the same lineage -- there is no way to tell them apart from LSN alone. timeline_history.c/.h (new, monitor-side): walks pgautofailover.node_timeline_history to answer "is candidate tli a genuine ancestor of the group's reference tli", where the reference is either an operator's pin in pgautofailover.accepted_timeline (part 3) or, in the normal case, auto-detected as the branch containing the highest reported tli among the group's own nodes. ProceedGroupStateForMSFailover now filters the group's node list through FilterNodesByTimelineAncestry() before it reaches BuildCandidateList() and ListMostAdvancedStandbyNodes() -- the two places that assemble and rank the candidate pool. Nodes outside the reference lineage are excluded entirely, not deprioritized: a diverged node can never become comparable no matter how long the election waits for it, so counting it as "missing" would block the whole group forever (see #683, #1113); excluding it lets the remaining, genuinely comparable candidates proceed normally, and if exclusion happens to leave zero candidates, the existing "not enough candidates yet" handling already does the right, graceful thing with no new plumbing needed. The already-selected-candidate fast path (a failover already in progress) is untouched: filtering only applies to the "pick a new candidate" path. * Part 3: operator-pinned accepted_timeline for genuine forks candidate_priority doesn't work for this: it's consulted inside SelectFailoverCandidateNode, which runs strictly after BuildCandidateList has already assembled the comparable pool -- the ancestry filter lives at or below that assembly step, deciding whether nodes are comparable at all, before priority ever enters the picture. Zeroing a node's priority changes how it's ranked once it's in a comparable set; it does nothing to tell the ancestry filter "ignore this node's incomparable position". What's missing is a fact the operator asserts explicitly, stored somewhere the filter itself consults -- not inferred from a side effect of an unrelated field. - pgautofailover.accept_timeline(formation_id, group_id, tli, decided_by): validates the tli actually appears in node_timeline_history for the group (refuses to pin a timeline nobody has ever reported), then inserts the pin. GetAcceptedTimeline() (already wired into FilterNodesByTimelineAncestry in the previous commit) picks it up ahead of auto-detection. - pgautofailover.resolve_accepted_timeline(formation_id, group_id): marks the pin resolved. Wired into PromoteSelectedNode() so it fires automatically once a candidate is actually selected for promotion -- kept as a permanent audit record rather than deleted. - pg_autoctl accept timeline --formation --group --tli [--reason]: new CLI command calling accept_timeline() via monitor_accept_timeline(). * Part 4: pg_autoctl show timeline Add node_timeline_status() SQL function (recursive ancestry CTE, per node current tli/lsn against the group's reference lineage) and wire it up to a new `pg_autoctl show timeline` command that prints the group's known timeline history plus each node's position against it, flagging any node that has diverged. * Add monitor regression tests for timeline-fork detection Covers report_timeline_history()/node_timeline_history idempotency, node_timeline_status() ancestry computation (unpinned auto-detect, operator pin, and post-resolve revert), accept_timeline()'s rejection of unreported timelines, and an end-to-end election test: a 4-node group with two diverging branches where the operator's pin makes the election promote the pinned lineage and exclude the higher-numbered sibling branch, auto-resolving the pin on promotion. Also fixes two bugs the test caught: - accept_timeline()'s target_tli parameter shadowed the tli column it was compared against, so every call raised 'column reference "tli" is ambiguous' instead of doing anything. - timeline_history.c was missing from pgaftest's SHARED_SRCS list, so the pgaftest binary failed to link as soon as fsm_transition.c and service_keeper.c started calling into it (Part 2). * Fix node_timeline_history FK to cascade on node removal Without ON DELETE CASCADE, dropping any node that had ever published timeline history left pg_autoctl node drop / node_active('dropped') failing with 'update or delete on table node violates foreign key constraint node_timeline_history_nodeid_fkey' -- caught by running basic_operation.pgaf (test_016_drop_secondary) against a rebuilt docker image after the timeline-fork-detection changes. node_timeline_history rows are provenance (who reported this fact), not the fact itself -- other nodes' rows for the same tli are identical and unaffected, so cascading the delete loses nothing an election or ancestry check depends on. * Fix #683: standby ahead of upstream retried a doomed reconnect forever standby_check_timeline_with_upstream() only knew how to rewind a standby that was BEHIND its upstream's timeline (Part 1's genuine- divergence check). When local tli > upstream tli -- a standby that generated WAL past a fork point, e.g. after being promoted outside of pg_autoctl's control -- it just logged an error and returned false, forever: Postgres itself refuses to stream ("requested timeline N is not a child of this server's history" / "highest timeline N of the primary is behind recovery timeline M"), and nothing ever called pg_rewind to get out of it. That's the literal "unrecoverable state" from #683. Fixed by rewinding symmetrically in that direction too. Also broadens the ancestry check itself: fsm_follow_new_primary() is used for both REPORT_LSN/JOIN_SECONDARY -> SECONDARY (already checked) and SECONDARY -> CATCHINGUP (an ordinary health-check-driven resync, previously skipped entirely since the check was gated on assigned_role == SECONDARY_STATE). Manual reproduction showed this is exactly the path a forked standby takes when the health checker notices it and cycles it back through catchingup -- without the fix here it hit the identical doomed-reconnect loop one gate over. Finally, adds a defensive, synchronous timeline-history publish right before both ancestry checks (refactored out of fsm_report_lsn's existing one into keeper_defensive_publish_timeline_history()): the periodic per-tick publish and FSM transition processing share a cadence, so a fork that both appears and gets rewound within a single tick could otherwise never make it into node_timeline_history at all, losing the only record it happened. Root-caused and verified via manual reproduction against a live Docker cluster (network-partition the standby, promote it directly at the Postgres level to fork it, reconnect, force it back through catchingup): pre-fix, Postgres logs "waiting for WAL to become available" in an infinite retry loop; post-fix, it rewinds (or falls back to pg_basebackup when pg_rewind can't itself connect) and rejoins cleanly. tests/tap/specs/timeline_fork_report_lsn_deadlock.pgaf turns that reproduction into an automated, repeatable end-to-end spec. * timeline_fork_report_lsn_deadlock.pgaf: add live-cluster show/accept timeline coverage pg_autoctl show timeline and pg_autoctl accept timeline (Part 3/4) previously had monitor-SQL-level coverage only (timeline_fork_detection.sql) -- nothing exercised the actual CLI against a live cluster (cli_show.c/cli_accept.c, monitor_print_timeline()). New test_003_show_and_accept_timeline_cli step, inserted into the existing #683 fork scenario: unpinned, node2's fork reads clean (the known 2-node limitation from the header comment -- it IS the highest reported tli, and nobody else is arguing); accept_timeline rejects an unreported tli and pins the correct one; show timeline then correctly flags node2 as FORK. Orthogonal to the maintenance-cycle recovery step that follows it, since the standby-side #683 fix never consults the pin -- only a monitor-side election does. * ci: add every .pgaf spec to a CI schedule Audited tests/tap/specs/*.pgaf against the .sch shards actually wired into ci.yml's pgaftest matrix (the flat tests/tap/schedule file is a manual/local reference, not read by CI). Found 5 specs that existed but never ran in CI: - guard_data_loss, fast_forward: real specs (issues #1113, #1060), present in the flat schedule file but dropped when the .sch shards were split out. Added to multi-misc.sch (3-node scenarios, same family as the existing multi_standbys/multi_maintenance). - demote_timeout_wait_primary_deadlock (#1025) and timeline_fork_report_lsn_deadlock (#683): added this session, never wired up. Added to node.sch alongside replication_stall_3dc, which had the same gap. Left alone, by design: debug_citus_worker_switchover.pgaf and debug_failover_pg19.pgaf are explicitly documented as interactive-only diagnostic templates; upgrade.pgaf has its own dedicated CI job (needs pgaf:current/pgaf:next images the standard matrix doesn't build). Also brought the flat tests/tap/schedule file up to date with specs it was missing entirely (launch_deferred_set_metadata, postgres_version_tracking, tablespaces) plus the two new ones, so it stays a genuine full-suite reference for local runs. Verified locally: multi-misc.sch runs 6/7 specs clean; the 7th (fast_forward test_003) fails identically on unmodified origin/main (confirmed via a throwaway worktree build) -- a pre-existing, branch-unrelated flake, not a regression from this branch's changes. node.sch's new entries were each verified individually before being added; a full sequential node.sch run is still in progress. * fast_forward.pgaf: fix test_003 deadlock by calling perform_failover via SQL Root-caused via a throwaway origin/main worktree: this deadlock is pre-existing, not caused by the timeline-fork-detection changes, and was previously masked simply by never running in CI (see the prior commit adding this spec to multi-misc.sch). exec pg_autoctl perform failover --allow-data-loss blocks waiting for a node to reach exact PRIMARY_STATE (both goalState and reportedState) within listen_notifications_timeout (60s default). In this scenario node2 can only reach wait_primary during step 3 -- number_sync_standbys=1 needs a second live quorum standby, and node3 doesn't come back until step 4 -- so that wait can never succeed and the exec always times out. guard_data_loss.pgaf's near-identical scenario (primary + one standby killed, survivor promoted with --allow-data-loss) already documented and worked around this exact deadlock by calling pgautofailover.perform_failover() directly over SQL instead of through the CLI. Apply the same fix here. Verified stable across two full runs (5/5). * Drop fast_forward from this branch's CI schedule and revert its spec Reverts fast_forward.pgaf back to its pristine origin/main content (undoing commit cf9ba18) and removes it from multi-misc.sch. While making the spec actually exercise the real fast_forward FSM state (per review feedback that the CLI-deadlock fix alone left it functionally duplicating guard_data_loss.pgaf), it surfaced a genuine, pre-existing, unrelated bug: standby_fetch_missing_wal() (primary_standby.c) blocks in an unbounded loop with no timeout or WAL source liveness check, so once a fast_forward candidate's keeper commits to fetching, the monitor's own issue #1060 stuck-detection guard can never reach it -- the keeper simply never calls node_active() again to receive the reset. That's a real fix (a timeout/liveness check on the fetch loop), not a test-file fix, and out of scope here. fast_forward.pgaf and its stuck-detection bug will be picked up on a separate branch/PR. guard_data_loss stays in multi-misc.sch; it does not depend on fast_forward and was independently verified passing. * Fix citus_indent formatting in timeline_history.c/.h My earlier local style-check invocation (docker run citus/stylechecker without an explicit citus_indent argument) wasn't actually running the check -- it silently did nothing, so this drift went unnoticed until CI's style_checker job (which runs citus_indent --check directly inside the container) caught it on push. The Makefile already has the right invocation (make docker-check / make docker-indent); using that going forward instead of hand-rolling the docker command. * monitor.c: fix show timeline fork message to not assert election behavior monitor_print_timeline() previously predicted that 'automatic election is refused' for diverged nodes. That's a server-side election policy statement made from client-side CLI code, with nothing tying the two together, and it's imprecise: the design excludes the diverged node from candidacy, it doesn't refuse the whole election. Reword to state only the observed fact and point at the resolution command. * docs: document timeline-fork detection (accept/show timeline, FSM, fault-tolerance, architecture, FAQ, operations) New manual pages for pg_autoctl accept timeline and pg_autoctl show timeline, modeled on the existing perform-failover and show-standby-names pages, with example output captured from a live 2-node cluster forked the same way tests/tap/specs/timeline_fork_report_lsn_deadlock.pgaf does it. Wires both into the existing indexes (pg_autoctl_show.rst, manual.rst, pg_autoctl.rst). Extends the existing prose docs rather than introducing new structure: - failover-state-machine.rst: ancestry-check paragraphs in the Catchingup and Report_LSN sections, alongside the existing guard_data_loss paragraph. - fault-tolerance.rst: new "Timeline Forks" subsection with a 6th tikz sequence diagram (seq-timeline-fork.tex/.svg), following the same prose+diagram pattern as the other four scenarios. - architecture.rst: extends the existing "Node recovery" section. - faq.rst: new entry alongside the existing report_lsn-stuck entry. - operations.rst: new "Resolving a detected timeline fork" subsection under Cluster Management and Operations, plus a Trouble-Shooting Guide pointer, using the same captured-transcript style as the other operations subsections. No new operations manual needed -- docs/operations.rst already exists and is the right place for this. Verified with a full sphinx html build (no warnings) and a full tikz svg rebuild. * monitor: add missing SQL objects to the 2.2->2.3 upgrade script CI's upgrade.pgaf job failed: an in-place ALTER EXTENSION UPDATE TO '2.3' left several functions this branch (and two other already-merged PRs) added to the fresh-install script completely absent from the incremental 2.2--2.3 upgrade script, so any node calling them post-upgrade got 'function does not exist'. Ports over, matching the fresh-install definitions in pgautofailover.sql exactly: - set_node_region() (#1156) - the four pg_version*/citus_version columns and report_postgres_version() (#1157) - node_timeline_history / accepted_timeline tables and report_timeline_history() / accept_timeline() / resolve_accepted_timeline() / node_timeline_status() (this branch) Verified with make -C tests/upgrade all (builds pgaf:current from the v2.2 tag and pgaf:next from this branch, then runs tests/tap/specs/upgrade.pgaf end to end against a live 3-node cluster) -- all 10 steps pass, including the convergence step that previously failed. * docs+tests: address review feedback on timeline-fork-detection docs - pg_autoctl_accept_timeline.rst: drop the --tli 99 validation-error example (that's a unit-test concern, not documentation) and replace it with a pg_autoctl show timeline capture that makes the actual case for needing to pin a lineage. - fault-tolerance.rst: rephrase the Timeline Forks section to describe only current behavior, not implementation history ("pre-fix, ..."). - pg_autoctl_show_timeline.rst: new worked example correcting an assumption the docs didn't previously address -- a 3-node formation does NOT by itself close the 2-node auto-detection blind spot. FilterNodesByTimelineAncestry()'s heuristic only excludes a candidate when a genuinely competing branch is reported; a sibling that simply stays behind isn't competing with anything, so the fork still reads as clean and pg_autoctl accept timeline is still required. Backed by a new pgaf spec, tests/tap/specs/timeline_fork_3node_auto_detect.pgaf (added to tests/tap/schedules/node.sch), verified locally end to end -- all 6 steps pass, including the corrected assertions after the initial design wrongly assumed 3 nodes would auto-detect. Both new/changed CLI examples use real output captured from live 3-node (and, for the accept_timeline page, 2-node) clusters, same convention as the rest of this branch's docs. (The two new Unhealthy-Nodes sequence diagrams originally drafted alongside this work -- Primary node monitored unhealthy, Monitor node has failed -- document pre-existing, unrelated behavior and were split out to PR #1162 instead.) * pgaftest: size expect-mismatch error buffers to fit multi-row output runner_exec_step()'s errBuf was declared char[512] at all 8 call sites, while r->lastSqlOutput (the actual command output an 'expect' step compares against) is char[4096]. Any expect mismatch against output bigger than ~450 bytes -- e.g. pg_autoctl show timeline against a 3+ node formation -- overflowed the format buffer, and sformat()'s own overflow guard silently swallowed the real diagnostic, printing only 'BUG: sformat needs N bytes...' instead of the actual 'expected X, got Y' message. Bumped all 8 err/tdErr declarations to 8192 bytes (2x lastSqlOutput's cap plus room for the template and cmd->expected). Verified by forcing a real expect mismatch against a multi-row show timeline table (tests/tap/specs/timeline_fork_3node_auto_detect.pgaf with one expect deliberately broken): previously printed only the BUG warning, now prints the full untruncated 'expected ..., got ...' message with the complete table. * docs+tests: show concurrent application traffic during a 3-node fork The previous 3-node example forked node3 while node1/node2 sat completely idle -- unrealistic, since a fork developing on one standby elsewhere in the formation is no reason for the main system to pause. Extended timeline_fork_3node_auto_detect.pgaf so the application keeps writing to node1 (and replicating to node2) throughout every step of the scenario -- before the fork, during node3's isolation, after it reconnects, after the operator pins the lineage, and confirms all 200 rows survive node3's eventual recovery untouched. Recaptured the docs example from this updated spec: node1/node2's LSN is now visibly ahead of node3's diverged tip throughout, showing the ancestry-check blind spot alongside ordinary, uninterrupted application traffic rather than against a frozen cluster. Verified: all 6 pgaf steps pass end to end against a live 3-node cluster. * monitor: detect a timeline fork on a secondary as soon as it's reported Adds a new guard in ProceedGroupStateFromContext(), alongside the existing SECONDARY-state checks: when a node currently in SECONDARY_STATE reports a timeline that FilterNodesByTimelineAncestry() excludes from the group's reference lineage, it's pushed to CATCHINGUP immediately -- on its very next node_active() call -- instead of waiting for an incidental health-check cycle or an explicit maintenance toggle to eventually drive it there. This only changes *when* a diverged secondary gets pushed through the existing, already-tested ancestry-check-and-rewind path (the #683 fix); it reuses the same FilterNodesByTimelineAncestry() the report_lsn election path already calls, so the two can't drift apart. Scoped to SECONDARY_STATE specifically, matching the exact guard the pre-existing "unhealthy secondary" transition uses for the identical goal-state assignment. Verified: monitor rebuilt, docker image rebuilt, both live-cluster specs (2-node and 3-node) still pass end to end. Directly confirmed via a manual repro and via a new dedicated step (test_005_automatic_catchingup_without_maintenance in timeline_fork_3node_auto_detect.pgaf) that the monitor now assigns catchingup within ~1s of an operator's accept_timeline pin, with no maintenance toggle involved -- log evidence: "Setting goal state of node3 to catchingup: its reported timeline 2 does not appear to be an ancestor of the group's reference timeline 1 -- forcing a resync...". Kept the original maintenance-cycle-based full recovery check as a separate, subsequent step: reaching CATCHINGUP is now immediate, but completing the actual pg_rewind still depends on node1's pg_hba.conf hostname resolution being current against node3's address, a pre-existing dependency this change doesn't add or remove. That dependency's own staleness-after-reconnect behavior is a separate, already-flagged issue, unrelated to this fix's correctness. * pgctl: pg_rewind must connect to the formation's own database, not a hardcoded postgres Root-caused a permanent, 100%-reproducible pg_rewind failure hit while testing the new automatic timeline-fork detection: any formation whose --dbname isn't literally postgres (e.g. this branch's own timeline_fork_3node_auto_detect.pgaf, using demo) has pg_rewind's connection rejected by every peer's pg_hba.conf, every single time -- not a race, not a DNS caching issue (both directions of the hostname resolution were confirmed fine via the Postgres log's own DETAIL lines: forward lookup matches). pg_autoctl's own pghba_ensure_host_rules_exist() only ever writes HBA rules for the formation's configured database name plus the replication pseudo-database -- never a bare literal postgres unless that also happens to be the configured name. But pg_rewind()'s --source-server connection string hardcoded dbname=postgres regardless, so it could never match any rule pg_autoctl itself had written. ReplicationSource already carries the correct configured dbname (populated by standby_init_replication_source() from postgresSetup.dbname, the exact same value passed to pghba_ensure_host_rules_exist()) -- pg_rewind() just wasn't using it. Falls back to postgres only if that field is somehow empty, preserving prior behavior for that edge case. This masked itself in most existing coverage because fsm_rewind_or_init() (the pre-existing MAINTENANCE-to-CATCHINGUP path) already has a pg_basebackup fallback for any pg_rewind failure, so recovery would silently succeed via a full base backup instead of an incremental rewind -- slower and more wasteful, but not visibly broken. The #683 ancestry-check path (standby_check_timeline_with_upstream(), this branch's own new code) has no such fallback: a pg_rewind failure there just returns false and retries forever, with no escape valve, which is what actually exposed this. Verified directly: a manual replicator connection to the configured database now succeeds where it previously failed with 'no pg_hba.conf entry'; the full automatic-detection scenario (timeline_fork_3node_auto_detect.pgaf) now completes via a genuine incremental pg_rewind (pg_rewind: Done!, 85MB copied in under a second) instead of the pg_basebackup fallback it was hitting before. All 7 steps in that spec still pass end to end. * docs: describe the new automatic, near-instant catchingup push on a forked secondary Once a diverged secondary's timeline mismatch is visible to the monitor (via an explicit accept_timeline pin, or a genuinely competing branch auto-detected with no pin at all), it is now pushed to catchingup and rewound with pg_rewind within about a second -- no maintenance cycle or incidental health-check-driven resync needed to trigger it, as the docs previously described. - failover-state-machine.rst: new paragraph under Secondary describing the monitor-side ancestry check on every report. - fault-tolerance.rst: rewritten Timeline Forks prose + updated seq-timeline-fork.tex/.svg diagram (operator pin -> immediate goal=catchingup, no maintenance step depicted). - architecture.rst: Node recovery section mentions the proactive check. - faq.rst: timeline-mismatch entry no longer frames recovery as waiting on the next health-check cycle. - ref/pg_autoctl_accept_timeline.rst, ref/pg_autoctl_show_timeline.rst, operations.rst: replace "cycle the node through maintenance to force it" language with the new automatic, immediate behavior, verified against a live docker cluster (accept timeline -> catchingup -> rewound secondary within ~1s, confirmed via monitor logs). * timeline_fork_report_lsn_deadlock.pgaf: fix race between the new auto-catchingup guard and the maintenance-cycle step CI (logs_81878989213) showed test_004_maintenance_cycle_recovers failing intermittently (3/6 PG versions) with: monitor ERROR cannot start maintenance: ... current state for primary node 1 is "wait_primary" -> "wait_primary". Root cause: node2 is this formation's only standby, so once the new monitor-side ancestry guard (added earlier this branch) excludes it right after test_003's accept_timeline pin, node1 must also drop synchronous replication -- it transitions primary -> wait_primary at the same moment. The old test_004 called "enable maintenance" on node2 immediately after test_003 with nothing in between, racing node1's own transition back to primary about half the time. The 3-node companion spec never hit this: excluding one of two standbys there doesn't force the primary to touch synchronous replication at all, since a second real secondary is still available. Fix: add test_004_automatic_catchingup_without_maintenance (mirroring the 3-node spec's test_005) that both asserts the new automatic detection directly and waits for node1/node2 to fully settle back to primary/secondary before the next step touches maintenance -- renumbering the old test_004/test_005 to test_005/test_006. Updated the header comment and test_003's own comment, both of which described the pre-guard behavior ("goes unnoticed until forced") that's no longer accurate. Verified: 4/4 clean runs locally (previously ~50% failure), plus the 3-node spec re-run once more to confirm no regression there. * fast_forward: fix silent promotion corruption in multi-node failover tests/tap/specs/multi_ifdown.pgaf's failover to a lagging standby caught up via fast_forward (peer-to-peer WAL fetch, not from the primary) could silently fail to promote while reporting success, leaving the candidate stuck on its old timeline -- indistinguishable by timeline number from a demoted former primary, and a genuine risk of electing a node that never actually advanced. Root causes, found by live reproduction against real container logs rather than guesswork: * fast_forward configured Postgres's own recovery_target_lsn to an arbitrary snapshot of a peer standby's current replay position. That position isn't guaranteed to land on a genuine WAL record boundary, so Postgres could get stuck permanently 'waiting for WAL to become available' at that exact LSN, or FATAL-crash if promoted while still in that state ('recovery ended before configured recovery target was reached'). fast_forward already had its own application-level polling loop (pgsql_has_reached_target_lsn) to decide when enough WAL had been fetched; asking Postgres to also independently enforce the same threshold via recovery_target_lsn was both redundant and unsafe. The new ReplicationSource.pauseAtRecoveryTarget flag makes that opt-in, defaulting to off; fast_forward now just streams normally and decides for itself when to promote, exactly like the non-fast_forward promotion path already does. The one legitimate existing use (crash-recovery's 'immediate' target, always a well-defined, reachable point) opts back in explicitly. * Independently, the FAST_FORWARD_STATE -> PREP_PROMOTION_STATE transition (fsm_cleanup_as_primary) removed standby.signal from disk well before the real pg_ctl promote (issued later, from PREP_PROMOTION_STATE -> STOP_REPLICATION_STATE). Postgres's own promotion-completion logic removes standby.signal itself as part of processing the promote request, and FATAL-crashes if it's already gone ('could not remove file "standby.signal": No such file or directory'). fsm_cleanup_as_primary now leaves the on-disk standby setup untouched; the already-correct post-promotion cleanup in fsm_promote_standby (via standby_cleanup_as_primary) is the only place that needs to run it. * standby_cleanup_as_primary itself no longer restarts Postgres: restarting after standby.signal has been removed, but before a genuine promotion took place, makes Postgres boot as an ordinary (non-standby) server and complete plain crash recovery on its current timeline -- reaching a not-in-recovery state without ever going through the real promotion code path, which is exactly the silent-corruption failure mode this commit closes. Defense in depth, in case a future change reintroduces a similar race: * standby_promote() now calls the new standby_promotion_advanced_timeline to verify, via the local pg_wal/<TLI>.history file, that a claimed promotion genuinely advanced to a new timeline with a recorded ancestor -- not just a bare, unverified 'not in recovery' state -- both on its idempotent early-return path and after a real pg_ctl promote. * fsm_init_primary and fsm_promote_standby now publish the node's fresh timeline history to the monitor immediately after promotion (keeper_defensive_publish_timeline_history), rather than waiting for the next periodic tick, so other nodes and 'pg_autoctl show timeline' observe it as soon as it's genuinely confirmed. * standby_fetch_missing_wal and standby_promote now tell the separate Postgres controller service (service_postgres_ctl.c) to stand down (keeper_set_postgres_state_unknown/running) for the duration of their own waits, so its independent 100ms liveness poll can't race a transient unavailability during recovery/promotion into an unplanned restart of its own. Verified with repeated clean runs of multi_ifdown.pgaf (previously a deterministic 300s timeout on test_008_failover), confirming node3's genuine on-disk timeline advance (pg_walfile_name, pg_wal/*.history) and correct 'pg_autoctl show timeline' reporting across all nodes, plus timeline_fork_report_lsn_deadlock.pgaf and timeline_fork_3node_auto_detect.pgaf regressions. * primary_standby: detect same-timeline LSN divergence in standby_check_timeline_with_upstream standby_check_timeline_with_upstream's same-timeline branch previously treated 'upstream reports the same timeline number we're on' as sufficient proof of a shared history and returned success immediately. That mirrors pg_rewind's own well-documented limitation (and a matching, still-open Patroni issue for the identical scenario): a bare timeline-number match doesn't rule out genuine divergence when a node was promoted or written to outside of pg_autoctl's control while carrying the same timeline number as its nominal upstream. A genuine, unbroken standby can never get ahead of its own upstream in LSN terms while remaining on the exact same timeline -- ordinary streaming replication only ever replays WAL the upstream itself produced. So this adds one more check: when the timelines match, also compare the node's local LSN against the upstream's own reported current LSN. If local is ahead, that's the same genuine divergence #683 already handles for differing timeline numbers, just expressed here as same-number-but-diverged instead -- so it's resolved the same way, via primary_rewind_to_standby, followed by a metadata refresh and a re-check that the rewind actually landed us on the upstream's timeline. * fsm_transition_citus: fix same fast_forward promotion corruption for Citus workers fsm_citus_cleanup_and_resume_as_primary() is the Citus counterpart of fsm_cleanup_as_primary(), both firing at the FAST_FORWARD_STATE -> PREP_PROMOTION_STATE transition, i.e. before the real pg_ctl promote (issued later, from standby_promote() via fsm_promote_standby). The non-Citus path was already fixed in f375443/6462149 to leave the on-disk standby setup untouched at that point, but the Citus counterpart still called standby_cleanup_as_primary() (and, until just now, additionally restarted Postgres) at this same early call site. Live reproduction against a new debug spec (tests/tap/specs/ debug_citus_worker_fast_forward.pgaf, a 3-node Citus worker group mirroring multi_ifdown.pgaf's setup) showed Postgres FATAL-crashing during the actual promote: LOG: selected new timeline ID: 2 FATAL: could not remove file "standby.signal": No such file or directory LOG: shutting down due to startup process failure Postgres's own promotion-completion logic removes standby.signal itself while processing the promote request, and crashes if it's already gone. The crash-recovery restart that follows finds no standby.signal (already removed) and silently completes ordinary crash recovery of what looks like a primary, without ever genuinely promoting -- indistinguishable by timeline number from a demoted former primary. standby_promote()'s standby_promotion_advanced_timeline guard (from f375443) caught the resulting corruption and refused to proceed, but the underlying crash kept recurring on every retry. Fix: fsm_citus_cleanup_and_resume_as_primary() no longer touches the on-disk standby setup at all; it only prepares the coordinator-side master_update_node() call, exactly mirroring fsm_cleanup_as_primary()'s current no-op behavior. The already-correct post-promotion cleanup in fsm_promote_standby (shared with the non-Citus path) remains the only place that runs standby_cleanup_as_primary(). Verified with a clean run of debug_citus_worker_fast_forward.pgaf (previously a deterministic crash/300s timeout on this same repro), confirming worker1c's genuine promotion to timeline 2, plus citus_multi_standbys.pgaf (14/14) and citus_basic_operation.pgaf (14/14) regressions. * fsm_mermaid.c: fix citus_indent formatting (missing blank line) Pre-existing citus_indent violation in main's fsm_mermaid.c (added by #1164), surfaced by rebasing this branch onto origin/main: citus_indent wants a second blank line between FsmMermaidColorForState() and the following function comment, matching the two-blank-line convention used throughout the rest of the codebase. Caught by CI's 'Style check' job on the PR-merge ref (pull/1161/merge), which combines this branch with current main -- this file doesn't exist on this branch's pre-rebase history, so `make docker-check` only started failing locally once the rebase brought it in. * tests: use SIGINT for cluster teardown, stop leaking orphaned nodes Root-caused from CI: pytest / citus (PG16) and (PG17) both timed out at the 15-minute job-step ceiling in this branch's CI, despite every actual test assertion passing (76 passed). The failure was entirely in teardown_module()'s cluster.destroy(). Datanode.destroy() called self.stop_pg_autoctl() -- default SIGTERM -- which triggers pg_autoctl's graceful-shutdown maintenance handoff (keeper_graceful_shutdown() in service_keeper.c): for a primary with a viable standby, up to KEEPER_MAINTENANCE_SHUTDOWN_LOOP_MAX_SECS (30s) trying to reach maintenance, falling back to up to KEEPER_SHUTDOWN_LOOP_MAX_SECS (30s) reporting state to the monitor while Postgres stops -- 60s worst case, defined in defaults.h. That's exactly this module's own COMMAND_TIMEOUT (network.py), with zero margin for process/network/sudo overhead. test_citus_skip_pg_hba.py's coordinator (coord0a, primary, with standby coord0b) hit that ceiling, and stop_pg_autoctl() raised subprocess.TimeoutExpired with a margin of literal microseconds -- not random flakiness, two equal constants racing. Worse, that call ran *before* the try/except in Datanode.destroy() that respects ignore_failure, so the exception propagated straight out of Cluster.destroy()'s reversed-order loop and aborted it mid-iteration. Since coord0a is last in that order, every node destroyed cleanly except it -- and the abort meant self.monitor.destroy() and self.vlan.destroy() (the test network namespace teardown) never ran at all. The monitor's pg_autoctl process was left running as an orphan, holding the test container open until GitHub's 15-minute step timeout force-killed it, long after pytest itself had already printed its own summary and exited. There's no standby to protect when tearing down an entire test cluster, so there's no reason to pay for the graceful maintenance handoff here. SIGINT skips it entirely (keeper_graceful_shutdown() only runs on a plain SIGTERM -- service_keeper.c checks `asked_to_stop && !asked_to_stop_fast && !asked_to_quit`) while still stopping Postgres cleanly: for anything other than SIGTERM, supervisor_stop_subprocesses() forwards the signal to every service directly (not just the keeper), so the Postgres-controller sibling gets it too and calls ensure_postgres_status_stopped() on its own, same as it always does. The top-level supervisor process itself uses exitOnQuit=false (supervisor_init()), so SIGINT/SIGQUIT there still goes through the ordinary loop and properly reaps its children -- no new orphans introduced. SIGINT specifically (not SIGQUIT) because destroy() is a deliberate teardown, not a crash simulation -- that distinction is already documented on DataNode.fail()'s own default. Datanode.destroy() and MonitorNode.destroy() now both pass sig=SIGINT explicitly (stop_pg_autoctl()'s own default stays SIGTERM, still needed by tests wanting a plain graceful restart), and both wrap their stop call in the try/except that already existed for the rest of destroy() -- so a future timeout for an unrelated reason can no longer abort the rest of cluster teardown either.
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
pgautofailover.nodegains four nullable columns:pg_versionnum(int),pg_version(text),pg_versionstring(text),citus_version(text). These capture the connected node'sserver_version_num/server_version/version(), and its installed Citus extension version when present (NULL otherwise).Design
node_active().pgautofailover.report_postgres_version()is a standalone, non-STRICTfunction (fields can be NULL) called once per Postgres restart, not on every periodic report — this data can't change without a restart, so it stays offnode_active()'s hot path.LocalPostgresServer.pgIsRunningdirectly.pgIsRunningis written from multiple places (fsm_transition.c,primary_standby.c,keeper_update_pg_state()itself), so comparing it to its own previous value insidekeeper_update_pg_state()misses the first, most important transition — Postgres coming up for the first time.PostgresVersionInfo.lastKnownRunningis written only bykeeper_update_pg_state(), giving it a reliable not-running → running signal independent of every other writer.Implementation
pgsql_get_postgres_version()(src/bin/common/pgsql.c): one query fetching version info plus Citus'sextversionvia a scalar subquery.monitor_report_postgres_version()(src/bin/pg_autoctl/monitor.c): sends it to the monitor.ReportAutoFailoverNodeVersion()(src/monitor/node_metadata.c): plain SPIUPDATEbynodeid, no lock or existence check — a keeper self-report, same posture asReportAutoFailoverNodeState()(silent no-op on an unknown node, unlike operator-facing commands such asset_node_region).pgautofailover.report_postgres_version()(src/monitor/pgautofailover.sql/node_active_protocol.c): the SQL-callable entry point.Testing
node_active_protocol.sql/.outwith direct SQL coverage ofreport_postgres_version()(NULL handling, unknown/nullnode_id).postgres_version_tracking.pgafpgaftest spec, added to thequickschedule, exercises the keeper-side edge detection end to end against a live node.make docker-checkandci/banned.h.sh: clean. Full monitor regression (12 regress + 6 isolation) passes.config_get_set.pgafand the fullbasic_operation.pgafpass, confirming thekeeper_update_pg_state()change doesn't affect the core FSM lifecycle.