Fix 3-DC split-brain: monitor assigns wait_primary on replication stall (issue #997)#1149
Open
dimitri wants to merge 29 commits into
Open
Fix 3-DC split-brain: monitor assigns wait_primary on replication stall (issue #997)#1149dimitri wants to merge 29 commits into
dimitri wants to merge 29 commits into
Conversation
When the primary and standby lose connectivity while both remain reachable
from the monitor (the 3-DC split-brain scenario), synchronous_standby_names
stays set on the primary, causing every COMMIT to hang indefinitely.
Fix
---
Monitor-side FSM rule: when a PRIMARY node reports an empty
pg_stat_replication sync_state for longer than
pgautofailover.replication_stall_timeout (default 10 s), the monitor
assigns wait_primary. This clears synchronous_standby_names and unblocks
writes without initiating a failover; the standby rejoins as secondary once
the link is restored.
Implementation details
----------------------
* pgautofailover.node gains two new columns:
region text NOT NULL DEFAULT 'default'
replication_stall_since timestamptz (NULL = not stalled)
* Extension version bumped 2.2 → 2.3; upgrade script
pgautofailover--2.2--2.3.sql added.
* register_node() gains a node_region parameter (default 'default').
* current_state() now returns noderegion so pg_autoctl watch can display
the region column in verbose+ policies.
* ReportAutoFailoverNodeState() manages replication_stall_since: it is set
to COALESCE(replication_stall_since, now()) when the node is in PRIMARY
state and reportedrepstate is empty, and cleared (set to NULL) otherwise.
* New GUC: pgautofailover.replication_stall_timeout (10 s, PGC_SIGHUP).
* FSM rule in ProceedGroupStateFromContext(): if the primary is healthy
(monitor can reach it) but replication_stall_since is set and
now - replication_stall_since > replication_stall_timeout, assign
wait_primary with a diagnostic log message.
* pg_autoctl watch: added COLUMN_TYPE_REGION, shown in verbose/almost-full/
full/fully-verbose policies.
Test
----
tests/tap/specs/replication_stall_3dc.pgaf — a new regression spec that:
1. Confirms sync replication is active.
2. Severs the node1↔node2 Docker network link (simulating DC1↔DC2 failure).
3. Waits for the primary to reach wait_primary (stall timeout = 3 s here).
4. Verifies writes no longer hang (synchronous_standby_names is empty).
5. Restores the link and verifies the standby rejoins and replication is
healthy again.
Closes #997
- Thread --region option (-G) through all three pg_autoctl create postgres long_options arrays in cli_create_node.c, central option parsing in cli_common.c (case 'G'), keeper_config KeeperConfig.region field, nodespec.c ini/argv builder, and monitor_register_node() SQL call (). - Bump extension version to 2.3 consistently: Makefile EXTVERSION, metadata.h AUTO_FAILOVER_EXTENSION_VERSION, and pgautofailover.control already at 2.3. Add pgautofailover--2.3--dummy.sql upgrade path for dummy_update regress test. Update dummy_update.out expected DETAIL line to say '2.3'. - Fix monitor regress test ordering: add ORDER BY nodeid to the unordered SELECT from pgautofailover.node so the result is deterministic regardless of heap page layout (wider rows from new region/replication_stall_since columns changed physical scan order). - Update watch_colspecs.h MAX_COL_SPECS 12→14 to accommodate the new COLUMN_TYPE_REGION entry plus COLUMN_TYPE_LAST sentinel in the fully-verbose column policy (13 entries total). - Add pgaftest DSL support for 'region <name>' node option: T_REGION token in lexer, grammar rule in parser, TestNode.region field, compose_gen writes --region flag, cli_indent pretty-prints it. - Add tests/tap/specs/replication_stall_3dc.pgaf: 2-node cluster across dc1/dc2 regions; cuts network to node2, waits for node1 to reach wait_primary (FSM rule fires after replication_stall_timeout), then restores connectivity and waits for secondary recovery.
- Dockerfile: trailing backslash from debug edit made the next FROM line get appended to the RUN command, breaking make build entirely. Remove the stray continuation backslash. - expected/upgrade_1.out: PG18 added a 'Default version' column to \dx output; upgrade_1.out is the PG18+ variant. Still showed '2.2' as the default version; update all four rows to '2.3'. - expected/pg19/expected/monitor.out: mirror of monitor.out for PG19 (different pg_lsn display format). Did not include the ORDER BY nodeid clause added to monitor.sql; add it to keep the echoed SQL in the expected output in sync with the query file.
… fix client extension version - defaults.h: bump PG_AUTOCTL_EXTENSION_VERSION to "2.3" so the client binary matches the monitor extension version and does not try to downgrade 2.3 → 2.2 on startup (which crashed the monitor container in a compose stack). - test_runner.c (runner_compose_generate): after writing docker-compose.yml and node ini files, copy the spec file to <workdir>/spec.pgaf. This lets 'pgaftest step' reload the spec without the user supplying the original path again. - test_runner.c (runner_init): derive the Docker Compose project name from the work-directory basename (e.g. replication_stall_3dc) rather than the spec filename basename. When the spec is loaded from the in-workdir copy spec.pgaf the old code yielded 'spec', which did not match the running stack and caused 'no such container: spec-node2-1' errors. COMPOSE_PROJECT_NAME env var still takes precedence (used inside the compose network). - replication_stall_3dc.pgaf: add 'ssl off' to the cluster block. Without it pgaftest defaults to self-signed SSL, which requires certificate generation before Postgres starts; node1 failed with 'could not load server certificate file server.crt'. Tested end-to-end: all six steps of replication_stall_3dc pass locally.
parseCurrentNodeState already parsed 17 columns (0-16), including noderegion at column 14 added with the --region / issue-997 work. The guard check above it still said 16, causing pg_autoctl watch to log 'Query returned 17 columns, expected 16' in a tight loop and refuse to display any node state.
…ries The old Makefile had a phony 'common' target that ran make -C common, then both pg_autoctl and pgaftest depended on it. However, because 'common' was .PHONY, make always considered it out of date and rebuilt it on every invocation — and a parallel make -j could still race the two sub-makes against each other's common/*.o output files since both include Makefile.common and carry the same compile rules. Fix: make a real file target. Both pg_autoctl and pgaftest depend on it, so make builds the archive once serially first, then the two binaries in parallel without contention.
In --tmux mode the bottom pane previously exec'd into the first data node container, which had no pgaftest binary, no docker CLI, and no knowledge of the spec file. New approach: the pgaftest service (already emitted by compose_gen for CI) is reused as the interactive shell target. That container has: - the pgaftest binary - docker + docker compose (DooD via /var/run/docker.sock) - /spec.pgaf bind-mounted from the host workDir - COMPOSE_PROJECT_NAME, PGAFTEST_HOST_WORK_DIR, PG_AUTOCTL_MONITOR set compose_gen_write gains an 'interactive' parameter: when true the pgaftest service uses 'sleep infinity' instead of 'pgaftest run /spec.pgaf', keeping it alive for a shell. The interactive flag is set when runner_setup is called with withTmux=true. The bottom pane command changes from: docker compose exec -it <node1> bash to: docker compose exec -it pgaftest sh -c 'pgaftest _setup_ ... && exec bash' Four new sub-commands mirror the DSL keywords, callable directly from the interactive shell (context resolved from PGAFTEST_SPEC / PGAFTEST_HOST_WORK_DIR env vars injected by the compose stack): pgaftest wait until <node> state = <state> [timeout <N>s] pgaftest sql <node> "<query>" pgaftest network connect|disconnect <node> pgaftest assert <node> state = <state> After setup the bottom pane prints a hint listing available steps and example commands so the user knows immediately what they can type.
Two bugs in the previous commit:
1. hintCmd[256] was smaller than the literal format string (292 bytes)
before even inserting the step list, triggering sformat's BUG assert.
2. The bottom pane command used sh -c "..." with single-quote delimiters
inside; step names or other text containing apostrophes would break
the shell syntax, causing 'unexpected EOF' on startup.
Fix: drop hintCmd and the sh -c wrapper entirely. The bottom pane now
runs pgaftest _setup_ directly via docker compose exec (no sh -c), and
cli_run_setup_only prints the hint to stdout (the pane tty) after the
setup block completes, then execlp("bash") to hand the pane over to an
interactive shell. No string embedding, no quoting concerns.
…TEST_SPEC - pgaftest help (and bare pgaftest) now prints usage instead of "unknown command" - pgaftest show is now a sub-command set: show compose|spec|steps|services - pgaftest step accepts optional <spec.pgaf> second arg; when omitted it falls back to PGAFTEST_SPEC env var, /spec.pgaf in CWD, or <workDir>/spec.pgaf - compose_gen emits PGAFTEST_SPEC=/spec.pgaf into the pgaftest service env so all interactive sub-commands (step, wait, assert, network, sql) can discover the spec without explicit path arguments inside the container - resolve_interactive_context() helper centralises the env fallback logic
- pgaftest show step: lists sequence steps with:
* next step to run
! last failed step (will retry)
(space) completed steps
- pgaftest step (no args): auto-advances to the next step using a state
file at <workDir>/pgaftest.state. On failure current stays at the
failed step so the next no-arg invocation retries it.
- pgaftest step <name>: still works; also updates the state file and
advances current when the named step matches the cursor position.
- TestRunnerState struct + runner_state_read / runner_state_write /
runner_step_next added to test_runner.c/h; state is JSON for easy
inspection with a text editor.
- show_resolve_spec() now derives workDir when it is not set, so
pgaftest show step <spec.pgaf> picks up the state file automatically.
getopt permutes argv to move flags before positionals when optind is reset to 0 (full re-initialisation). Without this, the library's first getopt pass at the root level stopped at the first non-option (the subcommand name), leaving any flags that appeared before the spec file — e.g. "setup --tmux spec.pgaf" — unprocessed by the sub-command's getopt, which then saw an empty positional list and printed the usage error. Setting optind=0 (as every pg_autoctl getopts function does) tells getopt to reorder the argument vector so all options are seen before non-option positionals, regardless of the order the user typed them. Also simplify cli_setup back to its original form now that ordering is handled at the getopt level instead of by hand.
Replaces commandline_help() (one-level flat listing) with commandline_print_command_tree() so that pgaftest help renders the full command tree with sub-command groups expanded, matching the output style of pg_autoctl help.
The workDir bind-mount was :ro, preventing runner_state_write from creating pgaftest.state inside the container. The state file belongs in workDir alongside the generated compose files, so drop the :ro flag.
The pgaftest image already creates a 'docker' user with HOME=/var/lib/postgres. Switch the pgaftest compose service from root to that user: user: docker working_dir: /var/lib/postgres spec mounted at /var/lib/postgres/spec.pgaf This lands the interactive shell in the docker user's HOME with the spec file right there, and aligns the SSL cert paths with ~/.postgresql/.
…file Adds: - Host vs. container command table with descriptions - Typical interactive session walkthrough - DooD architecture section explaining the pgaftest service container - Step state file section - Updated synopsis and sub-command reference to match current CLI (show compose|spec|step|services, step auto-advance, etc.)
runner_state_path() picks the state file location based on whether PGAFTEST_COMPOSE_SERVICE is set: - inside container → $HOME/pgaftest.state (always writable by docker user) - on host → <workDir>/pgaftest.state (alongside compose files) The bind-mounted workDir may not be writable by the container's docker user on Linux hosts where UIDs don't align. $HOME (/var/lib/postgres) is owned by the docker user in the image, so it is unconditionally writable. Update docs/ref/pgaftest.rst to reflect the new state file locations.
…ate/step Command table changes: - setup / prepare / down moved under new 'cluster' subcommand group (pgaftest cluster setup|prepare|down) - pgaftest tmux <spec.pgaf> replaces pgaftest setup --tmux; --tmux option removed from pgaftest_getopts entirely - pgaftest wait and pgaftest assert removed (not useful interactively) show subcommand changes: - show steps (plural, restored) — sequence list with */ ! progress markers - show step (singular, new) — prints the DSL commands of the next step to run - show state (new) — 'Step X/N: name' header then pg_autoctl show state output - show services removed New functions in test_runner.c: - test_cmd_print(): prints a TestCmd in DSL form to a FILE* - runner_show_state(): prints step progress header then pg_autoctl show state
The old name implied the variable held a service name. It is actually a boolean flag (set to "1") that tells the binary it is running inside the pgaftest container — which determines where to write the step state file (container $HOME vs host workDir). Also complete the docs/ref/pgaftest.rst update: Synopsis and Sub-commands sections now reflect the restructured CLI (cluster setup/prepare/down, tmux, show steps/step/state, removal of wait/assert/show services).
…eady When the monitor container is still starting up, 'pg_autoctl watch' fails on its first connection attempt and exits, closing the tmux pane. Add --wait to pg_autoctl watch: it calls pgsql_set_monitor_interactive_retry_policy() before entering the main loop, which makes the internal pgsql_open_connection() retry for up to 15 minutes with exponential back-off (same policy used by the demo app and enable/disable maintenance commands). pgaftest now passes --wait in the tmux middle pane so the watch pane stays alive while the monitor container initialises.
…r startup Makefile.common: replace individual -Werror=implicit-* flags with a single -Werror so all -Wall warnings are fatal on both macOS and Linux. The generated bison/flex files already carry -Wno-error in the pgaftest Makefile so those remain unaffected. test_runner.c: the tmux bottom pane (docker compose exec -it pgaftest) exited immediately when the container wasn't yet running, closing the pane before the user could see it. Fix: probe with a no-TTY 'docker compose exec -T pgaftest true' loop until the container accepts exec, then hand off to the interactive command. Same timing issue as the watch pane (fixed earlier with --wait). Also log the manual exec command on startup so the user can reattach to the pgaftest shell from the host tmux pane if needed: docker compose -p <project> -f <workdir>/docker-compose.yml exec -it pgaftest bash
Convenience command for opening a new interactive shell in the running pgaftest container from a host tmux pane: pgaftest cluster sh [--work-dir <dir>] Equivalent to: docker compose -p <project> -f <workdir>/docker-compose.yml exec -it pgaftest bash Resolves the project name and workdir the same way as the other cluster subcommands (--work-dir flag, PGAFTEST_HOST_WORK_DIR env, or derived from the spec file path).
The 'sleep infinity + exec' model caused a timing race: tmux opens the
pane before the pgaftest container is in 'running' state, so exec fails
and the pane closes immediately. The probe-loop workaround was a band-
aid; the real fix is to not require a background service at all.
docker compose run starts a fresh container on demand from the service
definition (same image, bind-mounts, env, network) without needing the
service to already be running. No timing race, no probe loop.
Changes:
- runner_compose_up: pass --scale pgaftest=0 in interactive mode so the
image is built but no background container is started
- tmux session now has 4 panes when the spec has a setup{} block:
pane 0 docker compose logs -f
pane 1 pg_autoctl watch --wait
pane 2 pgaftest _setup_ via docker compose run --rm (closes when done)
pane 3 interactive bash via docker compose run --rm -it
Without setup{}: 3 panes (pane 2 is the interactive shell directly)
- cluster sh: updated to use docker compose run --rm -it as well
…est/.last) pgaftest cluster setup / tmux write the active work directory to $TMPDIR/pgaftest/.last on startup. resolve_interactive_context() reads it as the final fallback when neither --work-dir nor PGAFTEST_HOST_WORK_DIR nor a spec file is available. Effect: after 'pgaftest tmux tests/.../foo.pgaf', all subsequent host commands work without arguments: pgaftest cluster sh pgaftest cluster down pgaftest show steps pgaftest show state
runner_down() now kills the tmux session (named after the project) after compose down, so a subsequent 'pgaftest tmux' for the same spec does not hit 'duplicate session: <name>'. The kill is best-effort (2>/dev/null || true) so it is harmless when no tmux session exists.
The pgaftest container runs as the 'docker' user but /var/run/docker.sock is owned by a host group (GID 1 on macOS Docker Desktop, typically 999 on Linux) that does not match any group inside the container. This caused: permission denied while trying to connect to the docker API at unix:///var/run/docker.sock Fix: stat /var/run/docker.sock at compose-generation time and emit 'group_add: [<gid>]' in the pgaftest service so the container user gains the socket's group as a supplementary group, regardless of what numeric GID the host assigns to it.
- runner_setup: check 'tmux -V' before doing anything when withTmux is
true; log the version at NOTICE level, error out cleanly if tmux is
not on PATH. Uses run_cmd_capture, the same helper used everywhere
else in the runner.
- Setup container renamed from <project>-pgaftest-run-<hash> to
<project>-setup so compose logs show a short, readable name.
Interactive shell container named <project>-sh for the same reason.
Both still carry --rm so they are removed when they exit.
docker socket permission history
---------------------------------
Previously the pgaftest service ran as 'user: root'. Root bypasses the
socket's group-ownership check, so DooD worked without any group
membership.
We changed to 'user: docker' (an unprivileged UID) to avoid running
test commands as root inside the container. /var/run/docker.sock is
typically owned root:docker with mode 0660 (or root:<N> where N is the
host docker GID, which varies: 1 on macOS Docker Desktop, ~999 on most
Linux distros). The container's 'docker' user is not in that group, so
every docker CLI call got EACCES.
The previous commit added 'group_add: ["<gid>"]' by statting the
socket at compose-generation time. 'group_add' is honoured by both
'docker compose up' and 'docker compose run', so it applies to the
setup and shell containers started via 'run --rm' as well. The compose
file must be regenerated ('pgaftest cluster down && pgaftest tmux') for
the fix to take effect on an existing cluster.
…ude from up
Root cause of monitor not running
-----------------------------------
In interactive mode, runner_compose_up passed --scale pgaftest=0 to
prevent the sleep-infinity container from starting. Docker Compose v2
still rebuilds ALL service images when --build is present, even for
scaled-to-zero services. On a re-run (stack already up from a previous
pgaftest tmux invocation), 'up --build -d --scale pgaftest=0' rebuilds
and recreates the monitor/node containers, causing a brief downtime
window during which runner_wait_for_monitor could time out, tear the
stack down, and leave the cluster gone — hence 'monitor not running'.
Fix
---
In interactive mode the service is now named 'setup' and carries
'profiles: [setup]'. Services with a profile are completely invisible
to 'docker compose up' (no build, no start, no scale interaction).
'docker compose run setup' still works and starts the container on
demand. This means:
- 'docker compose up --build -d' only builds and starts cluster
services (monitor, nodes) — never touches the setup image.
- Subsequent pgaftest tmux runs on an already-running stack are
non-destructive: up is a no-op for running healthy containers.
- --scale pgaftest=0 removed; no longer needed.
Service naming
--------------
Interactive mode: service named 'setup' — appears as '<project>-setup'
in logs and docker ps. The --name flag on docker compose run gives
containers the stable names '<project>-setup' (setup pane) and
'<project>-sh' (interactive shell pane).
CI mode: service still named 'pgaftest', no profile, command set to
'pgaftest run <spec.pgaf>' so 'docker compose up --exit-code-from
pgaftest' continues to work unchanged.
…y ensures readiness) In tmux mode the host process does not use the monitor LISTEN connection at all — it just starts the stack and hands off to the tmux session. docker compose up -d already waits for the full depends_on: service_healthy chain before returning, so the monitor is provably ready when up exits. runner_wait_for_monitor from the host is both unnecessary and the source of the intermittent 'monitor not running' failure: on Docker Desktop for Mac, published-port connections appear as 192.168.65.1, which is outside the monitor's pg_hba trust CIDR. The fallback pg_hba patch and the 120s timeout loop both ran on the critical path before tmux launched, creating a wide window for races. runner_apply_formation_settings only needs docker compose exec (internal network) — no host->monitor libpq — so it works fine without the wait. runner_wait_for_monitor is still called in CI mode (runner_run) where the host process genuinely drives steps via LISTEN/NOTIFY.
dimitri
added a commit
that referenced
this pull request
Jul 16, 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
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.
Problem
In a 3-DC topology — primary in dc1, standby in dc2, monitor in dc3 — cutting the dc1↔dc2 link while both nodes remain reachable from the monitor leaves
synchronous_standby_namesset on the primary. EveryCOMMITthen hangs indefinitely waiting for an acknowledgement from a standby that can no longer be reached.Closes #997.
Root cause
The FSM had no rule to transition the primary out of the
primarystate when synchronous replication was stalled. The monitor could see that the standby was unreachable from the primary (viapg_stat_replicationreportingsync_state = 'unknown') but took no action.Fix
Monitor side (
src/monitor/):replication_stall_sincecolumn topgautofailover.node. The monitor sets it tonow()the first time a primary reportssync_state = unknown(replication partner unreachable) and clears it when replication recovers.regioncolumn topgautofailover.node(populated from--regiononpg_autoctl create postgres). This lets operators label nodes with their DC location for observability.pgautofailover.replication_stall_timeout(default 10 s). Whennow() - replication_stall_since > timeout, the FSM assignswait_primary, which clearssynchronous_standby_namesand unblocks writes.Client side (
src/bin/pg_autoctl/):--region/-Gflag added topg_autoctl create postgresand wired throughkeeper_config,monitor_register_node, andnodespec.pg_autoctl watchupdated to display theregioncolumn (column count guard fixed from 16 → 17).Test (
tests/tap/specs/replication_stall_3dc.pgaf):New end-to-end regression test using the
pgaftestframework:The test models the 3-DC scenario in a single Docker Compose stack: the monitor is always reachable from both nodes, but
docker network disconnectsevers the dc1↔dc2 replication link. Six steps verify:synchronous_standby_names = 'ANY 1 (…)')wait_primarywithin ~20 s (well within the 60 s timeout)synchronous_standby_namesis cleared; writes no longer hangInteractive reproduction
Start the cluster (pulls / builds images on first run, then takes ~30 s):
This opens a tmux session: compose logs (top),
pg_autoctl watchon the monitor (middle), interactive shell (bottom). From the shell:Commits
b92f46a17ac3db--regionthrough CLI; extension 2.3; 3-DC test specf7c5ab4f54588fpgaftest stepspec discovery and project name derivationc3dad45pg_autoctl watchcolumn count guard (16 → 17)9aefb90src/bin/Makefile: serialise common archive before parallel builds