Repository navigation
Add pg_autoctl restart postgres: safe, maintenance-aware Postgres restart - #1198
Merged
Merged
Conversation
added 5 commits
October 13, 2026 21:58
…tart
Restarting Postgres by hand (pg_ctl restart, systemctl restart, ...) on
a node pg_autoctl manages races the keeper's own process supervision:
service_postgres_start() forks and execv()s Postgres directly as
pg_autoctl's own child, tracked via waitpid, so an out-of-band stop is
seen as an unexpected child death.
pg_autoctl restart postgres goes through pg_autoctl's own supervised
process instead (fsm_checkpoint_and_stop_postgres() + keeper's
ensure_postgres_is_running(), the same couple of CHECKPOINTs some FSM
transitions already perform before a planned shutdown), and adds the
safety rails a bare restart doesn't have:
- by default, refuses unless the node is already in maintenance, so
a restart can never happen by accident to a node still serving
traffic
- --maintenance has the command enter and leave maintenance mode
around the restart itself, for changes that only need the config
edited ahead of time
- --allow-failover, required together with --maintenance on a
primary, mirrors enable maintenance's own gate (entering
maintenance on a primary promotes a standby first)
- --force skips every check for scripted/emergency use, still going
through the same safe stop+start path
No new monitor SQL: this is pure client-side orchestration reusing
monitor_start_maintenance()/monitor_stop_maintenance()/
monitor_wait_until_node_reported_state() (already called by
enable/disable maintenance) and keeper_ensure_postgres_is_running()
(already used internally for primary_conninfo changes).
Verified end to end against a real 2-node monitor+primary+standby
cluster: refusal when not in maintenance, checkpoint+stop+start both
standalone and via --maintenance, and --force while not in
maintenance.
docs/operations.rst gains a new section walking through both usage
patterns -- a pure config edit (wal_level) via --maintenance, and a
change requiring an offline step (enabling data checksums via
pg_checksums) via manual enable/disable maintenance around the
restart -- plus the usual pg_autoctl_restart[_postgres] manual pages.
--exec <command>: a shell command to run while Postgres is stopped, before starting it back up -- for an offline step such as pg_checksums --enable that can't run on a live cluster. Run the same way Postgres itself runs archive_command: the whole string goes to system(3) rather than being split into an argv and execv()'d, so it can be a script in any language or a pipeline, not just a single directly-executable binary. A non-zero exit (or termination by signal) aborts before Postgres is started back up and before maintenance mode is disabled, leaving the node exactly where the operator can inspect and finish by hand. cli_restart_enable_maintenance()/cli_restart_disable_maintenance() were near-duplicates of cli_enable_maintenance()/ cli_disable_maintenance() in cli_enable_disable.c. Extracted the shared logic into keeper_cli_enable_maintenance()/ keeper_cli_disable_maintenance(), now used by both the existing enable/disable maintenance commands and this one -- no behavior change for the existing commands, and restart postgres now reports the exact same exit code enable maintenance itself would for the same failure (e.g. EXIT_CODE_BAD_ARGS for a primary missing --allow-failover, previously EXIT_CODE_MONITOR here). Reordered the sequence to CHECKPOINT; CHECKPOINT; enable maintenance; stop; exec; start; disable maintenance -- the two checkpoints now run first, unconditionally, before maintenance mode is touched at all, rather than being bundled with the stop step. The checkpoint action itself is now primary_checkpoint(), a new function in primary_standby.c shared with fsm_checkpoint_and_stop_postgres() in fsm_transition.c, which already issued the same two CHECKPOINTs before its own planned shutdown. Verified end to end against a real 2-node monitor + primary + standby cluster: every combination of --maintenance, --allow-failover, --force, and --exec (including a deliberately failing --exec command correctly leaving Postgres stopped, and a follow-up restart successfully recovering it). docs/operations.rst's first worked example is now a plain ALTER SYSTEM SET (shared_buffers) instead of pg_checksums, since that one doesn't need --exec at all; pg_checksums stays as the --exec example, now collapsed into a single command per node.
…page The usage/flags block duplicated pg_autoctl restart postgres --help and the pg_autoctl_restart_postgres manual page, and had drifted out of sync with both after the --exec addition and the terser option descriptions. Replace it with a :ref: link to the manual page instead of keeping a second copy to maintain.
keeper_cli_enable_maintenance()/keeper_cli_disable_maintenance() were declared in cli_common.h (the CLI command/option header, not a home for business logic) and defined in cli_enable_disable.c, a CLI-layer file. Moved both to keeper.c, next to their closest sibling keeper_restart_postgres() -- a function with the same shape: takes a Keeper *, drives local state plus monitor calls, used by more than one CLI command. Declared in keeper.h instead, and renamed to keeper_enable_maintenance()/keeper_disable_maintenance() to match keeper_restart_postgres()'s own naming, since "cli" in the name no longer signals anything once they live in keeper.c rather than a cli_*.c file. No logic changes: pure extraction, same bodies, updated call sites in cli_enable_disable.c and cli_restart.c.
cli_restart_run_exec_command() read optExecCommand (and its own emptiness check) directly. Move the "is there a --exec command at all" check to the one call site in cli_restart_stop_exec_and_start(), and pass the string in as a parameter -- the function itself no longer needs to know about the global option variable, and a caller reading it top to bottom sees the global access in exactly one place.
dimitri
pushed a commit
that referenced
this pull request
Oct 14, 2026
…docs pg_autoctl restart postgres (#1198) is now on main after rebasing this branch on top of it. Turn the plain-text command name into a proper :ref: to its manual page, and point to the Operations guide's own restart_postgres section for more worked examples of the same command (including the --exec case, which doesn't apply to a plain wal_level edit but is worth knowing about).
dimitri
pushed a commit
that referenced
this pull request
Oct 14, 2026
…by_names siblings Same methodology as the restart postgres PR (#1198) review: this PR introduced four near-duplicates of pre-existing functions, each differing only in which monitor/SQL function to call and a setting name used in error messages or JSON output. Extracted the shared logic into one new helper per layer, keeping each pair's own thin wrapper: - node_active_protocol.c: synchronized_standby_slots() duplicated the "build a comma-separated pgautofailover_standby_<id> list" loop from synchronous_standby_names(). Extracted AppendStandbySlotNames(). - monitor.c: monitor_synchronized_standby_slots() duplicated monitor_synchronous_standby_names()'s entire body (same SQL execution, same error handling) bar the SQL function name and setting name. Extracted monitor_get_group_setting_from_sql(). - cli_show.c: cli_show_synchronized_standby_slots() duplicated cli_show_standby_names()'s entire body (same fetch, same JSON/plain printing) bar the monitor function and setting name. Extracted cli_show_monitor_computed_setting(). - fsm_transition.c: fsm_apply_settings() had two copies of the same "fetch a monitor-computed setting into a buffer, or fall back to a default when there's no monitor" block, one for synchronous_standby_names and one for synchronized_standby_slots. Extracted fsm_fetch_monitor_setting(), taking the monitor function as a function pointer (both have the exact same signature). No behavior change. Verified: the monitor's own synchronized_standby_slots regression test still produces byte-identical output against a live PG17 instance, and a real 2-node cluster still correctly shows synchronous_standby_names, synchronized_standby_slots, and wal_level after the refactor.
dimitri
added a commit
that referenced
this pull request
Oct 14, 2026
…t cleanup (fixes #1188) (#1197) * Add Postgres 17+ CDC continuity: synchronized_standby_slots, demotion slot cleanup (#1188) Lets a failover-enabled logical replication slot survive a pg_auto_failover promotion on Postgres 17+, by automating the pieces Postgres itself needs: - Monitor: new pgautofailover.synchronized_standby_slots(formation, group) SQL function, added to the extension's current unreleased schema revision (still '2.3', the last tagged release being v2.2 -- folded into the existing pgautofailover--2.2--2.3.sql migration alongside the other schema changes already accumulated there since v2.2, rather than bumping to a new 2.4; this project only bumps the extension version at actual release time). Computed like synchronous_standby_names but listing every current failover candidate's physical slot name, so a failover-enabled logical slot only advances once every promotable node has replayed that far. - pg_autoctl: fetches and applies that value on the primary via the same fsm_apply_settings() path that already maintains synchronous_standby_names; enables sync_replication_slots by default on 17+ nodes (alongside the existing hot_standby_feedback default), gated by a new pg_setup_synchronized_standby_slots_supported() version check. - Fix issue #1188: a node rejoining as standby after an interrupted demotion could keep its own stale (failover AND NOT synced) logical slots, permanently blocking the slot-sync worker. Extend the existing demotion slot cleanup to drop those too. New `pg_autoctl show synchronized-standby-slots` command (mirrors `show standby-names`), and `pg_autoctl watch`'s header now shows a live "CDC Slots" count next to "Sync Standbys". wal_level stays user-managed, as does logical slot creation itself -- CDC tooling (Debezium, etc) still owns slot lifecycle; pg_auto_failover only keeps the sync infrastructure correct across role changes. Documented in a new docs/cdc.rst page (including a comparison against Patroni, CloudNativePG, EDB PGD, and repmgr's own support for this Postgres feature, and why a single merged CDC stream across Citus workers needs a separate, standalone proxy component rather than an extension of pg_auto_failover itself) and a faq.rst entry. Covered by a new monitor regression test (synchronized_standby_slots.sql) and a pgaftest spec (cdc_continuity.pgaf) exercising a clean failover and the #1188 demote_timeout scenario -- run for real against `pg_auto_failover:pg17` Docker containers via `pgaftest run`, all 5 steps passing. That run surfaced and fixed two issues in the test itself (not the feature): setup{} restarting the primary's Postgres after it already had a synchronous standby attached (the risky pattern basic_operation.pgaf warns about), and a scenario that re-promoted the slot's original creator a second time, which separately hits a genuine Postgres-level limitation (a node round-tripping back to primary can get its own slot stuck in an unresolvable sync loop and lose it) -- now documented as a known limitation rather than conflated with the #1188 regression check. Also verified the real ALTER EXTENSION upgrade path end to end: installed at 2.2, updated to 2.3, confirmed synchronized_standby_slots() is callable immediately after, against a live Postgres 17 instance. * Default wal_level to logical, add --wal-level opt-out, document rollout runbook pg_auto_failover now creates nodes with wal_level=logical by default (required for CDC continuity's failover-enabled replication slots), patched into the existing default GUC settings array at node creation time. A --wal-level replica flag (also settable later via the keeper config file) opts back out for deployments that don't need logical decoding. docs/cdc.rst gains the manual runbook for flipping wal_level on an already-running cluster via maintenance mode, and docs/orchestrated-operations.rst explores a generalized monitor/client primitive for driving that kind of whole-cluster, multi-step change as a single declared, resumable, cancellable operation. cdc_continuity.pgaf is simplified to rely on the new default instead of manually toggling wal_level in its setup block. * Fix cdc_continuity CI image pull, drop orchestrated-operations design doc The spec hardcoded image "pg_auto_failover:pg17", a tag that only ever existed on a local Docker build used for manual validation. CI builds and loads pgaf:run-pg<N> and passes it via PGAF_IMAGE, which an explicit image "..." in the spec overrides -- so CI tried to pull a nonexistent "pg_auto_failover:pg17" from a registry and failed. Dropping the explicit image line falls back to PGAF_IMAGE, same as every other spec in the suite. Also removes the orchestrated-operations design exploration from the user-facing docs (toctree entry and the cdc.rst cross-reference) and deletes the page -- not ready to present as part of this feature's documentation. * Simplify wal_level rollout runbook: stop is enough, disable maintenance restarts disable maintenance's FSM transition for a standby always ends by calling ensure_postgres_service_is_running() (via primary_rewind_to_standby(), reused by fsm_restart_standby()), so Postgres comes back up on its own as part of rejoining -- no separate start/restart step needed, the operator only has to stop it after editing the config. Also notes that pg_autoctl's own Postgres controller explicitly steps aside during enable maintenance (local_postgres_unlink_status_file(), "so that this process won't interfere with whatever the user is doing during maintenance (such as stop Postgres)"), so a plain pg_ctl stop here is already safe and doesn't race pg_autoctl's supervision. * Remove HA-tools comparison from docs/cdc.rst That section was internal research material, not something that belongs in user-facing documentation. * synchronized_standby_slots: exclude candidates not currently reporting secondary GroupListCandidates() only filters by candidatePriority > 0, regardless of the node's current reportedState -- a candidate the monitor has pushed back to catchingup (lagging, missed a reporting window, ...) was still listed, which would make a failover-enabled logical slot's progress hostage to that node recovering. Filter the candidate list down to AutoFailoverNode.reportedState == secondary. Deliberately checks reportedState, not goalState: an extra standby beyond what number_sync_standbys needs can sit at goalState catchingup indefinitely for quorum bookkeeping alone while genuinely being a fully caught-up secondary, and that node belongs in the list -- this is why the existing regression test's "both standbys are candidates" assertion is unchanged (sss_s2's goalstate is catchingup there, but it had already reported secondary). Adds a new regression scenario exercising the actual lag case: a standby that self-reports catchingup drops out of synchronized_standby_slots, and reappears once it reports secondary again, verified against a live PG17 instance. * Fix stale FAQ wording, make wal_level's hardcoded default match reality The FAQ still said "you still need to set wal_level = logical yourself" -- stale since wal_level now defaults to logical at node creation (this same PR). Point to the new default/opt-out and rollout sections instead. Also stop lying in the C source: DEFAULT_GUC_SETTINGS_FOR_PG_AUTO_FAILOVER hardcoded wal_level to 'replica' with a comment saying it gets patched to 'logical' by default -- so the array never actually produced 'replica' in the common case, only in the --wal-level replica opt-out. Flip the hardcoded literal to 'logical' (the real default) and only patch the array when a non-Citus node explicitly opted out to 'replica', instead of unconditionally copying and patching on every node creation just to re-assert the same default value. * Simplify wal_level rollout runbook using pg_autoctl restart postgres Replaces the four manual steps per node (enable maintenance, edit config, stop Postgres, disable maintenance) with the new pg_autoctl restart postgres --maintenance [--allow-failover] command from a separate, in-progress PR: edit the config, then one command per node. --allow-failover is still required for the primary, same gate as enable maintenance itself. * Link docs/cdc.rst's wal_level runbook to the merged restart postgres docs pg_autoctl restart postgres (#1198) is now on main after rebasing this branch on top of it. Turn the plain-text command name into a proper :ref: to its manual page, and point to the Operations guide's own restart_postgres section for more worked examples of the same command (including the --exec case, which doesn't apply to a plain wal_level edit but is worth knowing about). * Dedupe synchronized_standby_slots code paths against their sync_standby_names siblings Same methodology as the restart postgres PR (#1198) review: this PR introduced four near-duplicates of pre-existing functions, each differing only in which monitor/SQL function to call and a setting name used in error messages or JSON output. Extracted the shared logic into one new helper per layer, keeping each pair's own thin wrapper: - node_active_protocol.c: synchronized_standby_slots() duplicated the "build a comma-separated pgautofailover_standby_<id> list" loop from synchronous_standby_names(). Extracted AppendStandbySlotNames(). - monitor.c: monitor_synchronized_standby_slots() duplicated monitor_synchronous_standby_names()'s entire body (same SQL execution, same error handling) bar the SQL function name and setting name. Extracted monitor_get_group_setting_from_sql(). - cli_show.c: cli_show_synchronized_standby_slots() duplicated cli_show_standby_names()'s entire body (same fetch, same JSON/plain printing) bar the monitor function and setting name. Extracted cli_show_monitor_computed_setting(). - fsm_transition.c: fsm_apply_settings() had two copies of the same "fetch a monitor-computed setting into a buffer, or fall back to a default when there's no monitor" block, one for synchronous_standby_names and one for synchronized_standby_slots. Extracted fsm_fetch_monitor_setting(), taking the monitor function as a function pointer (both have the exact same signature). No behavior change. Verified: the monitor's own synchronized_standby_slots regression test still produces byte-identical output against a live PG17 instance, and a real 2-node cluster still correctly shows synchronous_standby_names, synchronized_standby_slots, and wal_level after the refactor. * Fix citus_indent style violations (space before paren) * Update CDC docs limitation: lagging candidates now self-exclude The "No N of M" limitation bullet was stale: it described a permanently lagging/unreachable candidate as permanently stalling decoding, before the fix that filters synchronized_standby_slots down to candidates currently reporting themselves as secondary. A lagging candidate now drops out of the list on its own (pushed to catchingup by the monitor) and reappears once it catches back up, so the remaining limitation is narrower: there's still no knob to require only some of several simultaneously-healthy candidates, not that a single bad node blocks everything indefinitely. * Fix wal_level runbook: prose must precede the command, not follow it Step 1.b was just "b. ::" with a bare command block, and the explanation of what the command does trailed after it instead of introducing it -- a reader hit the command with zero context first. Restructured to match step 2's existing pattern: explain what's about to happen, then show the command.
This was referenced Oct 14, 2026
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
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
Restarting Postgres by hand (
pg_ctl restart,systemctl restart, ...) on a node pg_autoctl manages races the keeper's own process supervision:service_postgres_start()forks andexecv()s Postgres directly as pg_autoctl's own child process, tracked viawaitpid, so an out-of-band stop is seen by the supervisor as an unexpected child death.pg_autoctl restart postgresgoes through pg_autoctl's own supervised process instead, and adds safety rails a bare restart doesn't have:--maintenancehas the command enter and leave maintenance mode around the restart itself -- for a change that only needs the config edited ahead of time, nothing more--allow-failover, required together with--maintenanceon a primary, mirrorsenable maintenance's own gate (entering maintenance on a primary promotes a standby first)--forceskips every check, for scripted/emergency use, while still going through the same safe stop+start path--exec <command>runs a shell command while Postgres is stopped, before starting it back up -- for an offline step such aspg_checksums --enablethat can't run on a live cluster. Run the same way Postgres itself runsarchive_command: the whole string goes tosystem(3)rather than being split into an argv andexecv()'d, so it can be a script in any language or a pipeline, not just a single directly-executable binary. A non-zero exit (or termination by signal) aborts before Postgres is started back up and before maintenance mode is disabled, leaving the node exactly where the operator can inspect and finish by hand.The full sequence is:
CHECKPOINT;CHECKPOINT; enable maintenance; stop;--exec; start; disable maintenance. The two checkpoints run first, unconditionally, before maintenance mode is touched at all -- the same couple ofCHECKPOINTsfsm_checkpoint_and_stop_postgres()already performs before a planned shutdown elsewhere in the FSM, now shared via a newprimary_checkpoint()inprimary_standby.crather than two copies of the same loop.No new monitor SQL -- this is pure client-side orchestration reusing
monitor_start_maintenance()/monitor_stop_maintenance()/monitor_wait_until_node_reported_state()andkeeper_ensure_postgres_is_running()(already used internally forprimary_conninfochanges). The maintenance-enter/leave logic itself is shared code too: extracted fromcli_enable_maintenance()/cli_disable_maintenance()intokeeper_cli_enable_maintenance()/keeper_cli_disable_maintenance(), used by both the existing commands and this one, sorestart postgresreports the exact same exit codeenable maintenanceitself would for the same failure.Testing
Verified end to end against a real 2-node monitor + primary + standby cluster built from this branch, every run fully sequential (no overlapping test processes):
enable maintenance+restart postgres(already in maintenance) +disable maintenance: real doubleCHECKPOINT, stop, and restart of a running Postgres, confirmed via logs andshow staterestart postgres --maintenanceon a standby: full auto enable+restart+disable sequencerestart postgres --maintenanceon a primary without--allow-failover: refused (EXIT_CODE_BAD_ARGS, matchingenable maintenance's own exit code)restart postgres --maintenance --allow-failoveron a primary: promotion, restart, and maintenance disabled on rejoinrestart postgres --forcewhile not in maintenance: succeeds unconditionally--force --exec "echo ... > marker": marker file written while Postgres was down, Postgres restarted successfully afterward--force --exec "exit 7": command failure correctly left Postgres stopped; a follow-up plain restart successfully recovered it--forcecombined with--maintenance/--allow-failover, and--allow-failoverwithout--maintenance: both rejected at option-parsing time with a clear messageDocs
docs/operations.rstsection (Applying a Postgres change that requires a restart) with two worked examples: a pure config edit (ALTER SYSTEM SET shared_buffers) via--maintenancealone, and a change requiring an offline step (enabling data checksums viapg_checksums --enable) via--exec, collapsed into a single command per node.pg_autoctl_restart/pg_autoctl_restart_postgresmanual pages, registered indocs/ref/manual.rst.Test plan
pg_autoctl restart postgres --helpshows correct usage--maintenance(standby and primary, with and without--allow-failover),--force,--exec(success and failure paths)--force+--maintenancemutual exclusion,--allow-failoverrequires--maintenance)