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.
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.