Add pgautofailover.guard_data_loss GUC and pg_autoctl perform failover --allow-data-loss - #1142
Merged
Conversation
When all quorum standbys are in REPORT_LSN and one of them is unreachable,
ProceedGroupStateForMSFailover refuses to promote any candidate — forever —
because the missing node may have acknowledged a synchronous commit that no
surviving standby has replicated. This is the correct conservative default.
The problem is there is no operator-visible escape. The only recovery path
until now was raw SQL surgery on the monitor catalog.
This commit adds two escape hatches:
1. pgautofailover.guard_data_loss GUC (PGC_SUSET, default true)
When false, ProceedGroupStateForMSFailover bypasses both the
missingNodesCount guard and the quorumCandidateCount guard, allowing
failover to proceed with whatever candidates have reported their LSN.
A LOG message is emitted for each bypassed guard so the decision is
visible in the server log.
2. pg_autoctl perform failover --allow-data-loss
Opens a transaction, executes SET LOCAL pgautofailover.guard_data_loss
TO false, then calls the node_active protocol as normal. The GUC is
local to the transaction so it cannot leak to concurrent sessions.
The option is documented in the help text with a data-loss warning.
Also adds perform_failover rescue path for stuck-in-report_lsn scenarios:
when GetNodeToFailoverFromInGroup returns NULL (no primary to promote from),
perform_failover now searches for a node in REPLICATION_STATE_REPORT_LSN
and calls ProceedGroupState on it directly.
Regression test: src/monitor/sql/guard_data_loss.sql
- Bootstraps a 3-node formation (p primary, s1/s2 secondaries,
number_sync_standbys=1)
- Manufactures the stuck state: p draining, s2 dead-and-unreported,
s1 in report_lsn/report_lsn
- Test 1: guard_data_loss=true → perform_failover returns, s1 stays
in report_lsn (no candidate selected)
- Test 2: guard_data_loss=false → perform_failover selects s1,
s1.goalstate becomes prepare_promotion
Fixes: #1113, #1060, #1059, #1014
Closes: #1055
Tests the full allow-data-loss escape hatch in a live Docker Compose cluster: - 3-node formation (number_sync_standbys=1), node1 primary - node1 (primary) and node2 (standby) killed simultaneously - node3 enters REPORT_LSN and stays stuck: missingNodesCount=1 prevents automatic promotion (guard_data_loss=true by default) - test_002 asserts node3 stays at report_lsn after 10s (no spurious promotion) - test_003 runs pg_autoctl perform failover --allow-data-loss on the monitor, node3 advances to wait_primary - test_004/005 bring node2 and node1 back; cluster reaches healthy 3-node state Added to tests/tap/schedule after multi_alternate.
pg_autoctl perform failover --allow-data-loss connects to the monitor as autoctl_node, which is not a superuser. PGC_SUSET requires superuser to SET the parameter, so the SET LOCAL in the transaction failed with "permission denied to set parameter". Change to PGC_USERSET so any authenticated user can set it within their session. The GUC only affects the failover decision within the backend that runs perform_failover(), so no additional privilege is granted beyond what the caller already has by being able to call perform_failover().
- Document pgautofailover.guard_data_loss GUC in configuration.rst - Add --allow-data-loss option to pg_autoctl perform failover ref page - Add stuck report_lsn scenario explanation to failover-state-machine.rst - Add FAQ entry for stuck report_lsn failover - Add two TikZ sequence diagrams (normal failover, stuck-election failover) replacing broken mermaid blocks in architecture-multi-standby.rst - Remove sphinxcontrib-mermaid dependency (no longer used) - Bump ensure/test_004_demoted timeout 90s→180s (CI timing headroom)
This was referenced Jul 11, 2026
dimitri
added a commit
that referenced
this pull request
Jul 20, 2026
Reproduces 5/6 pytest/monitor CI failures (PG14, PG15, PG17, PG18, PG19), identical on every run: "node3 failed to reach primary after 300 seconds". Root cause: NOT pgautofailover.guard_data_loss. That GUC (added in #1142) only changes behavior when explicitly set to false (--allow-data-loss); its default (true) reproduces byte-for-byte the pre-#1142 blocking behavior in ProceedGroupStateForMSFailover -- confirmed by diffing ca7834b: every "return false;" guarding on missingNodesCount/quorumCandidateCount already existed unconditionally before that commit, the commit only wraps it in "if (GuardDataLoss)" and adds an unblocking else-branch that only runs when the GUC is off. Default-on behavior is unchanged. The real regression is commit a7974f8 ("fsm: wait for local Postgres to accept connections before reporting SECONDARY"), landed earlier in this branch. While fixing an unrelated connection-refused race, it also "fixed" this test's assertion: assert node1.wait_until_assigned_state(target_state="draining") assert node3.wait_until_assigned_state(target_state="report_lsn") --> assert node3.wait_until_state(target_state="primary", timeout=300) reasoning that 'report_lsn' is too transient to reliably observe. That's true, but the replacement assertion is unreachable as written: at this point in the test node2 is still down (killed in test_005_001 and not restarted until test_005_003), so with number_sync_standbys=1 the monitor needs 2 LSN reports before it will elect a new primary and node3 cannot reach "primary" here -- only node2's restart in test_005_003 can unblock that, and origin/main's version of this test correctly waits for "report_lsn" here and defers the "primary" assertion to test_005_003. a7974f8 turned a transient-state observability problem into a guaranteed 300s timeout by moving the wrong assertion to the wrong test function. The .pgaf port of this same test (multi_alternate.pgaf) hit the identical issue and was already fixed correctly in 3c9ca49, right after a7974f8: it both moves "compose start node2" earlier (into test_005_002) *and* adds a "passing through report_lsn" wait primitive to robustly observe the transient state via LISTEN. This commit applies the equivalent fix to the plain pytest side, which 3c9ca49 didn't touch: start node2 before killing node1, so quorum is satisfiable and "wait for primary" is reachable within test_005_002 itself. test_005_003_bring_up_first_failed_primary no longer needs to start node2 itself, and its wait for the transient 'demoted' state (which may have already come and gone by the time the test runs, since node2 restarts earlier now) is replaced with a wait for the stable end state, matching the same fix pattern already used elsewhere in this file. test_003_002_stop_primary (the 1/6 PG16-specific failure) was not touched: its assertions already tolerate node3 staying stuck at report_lsn (it explicitly checks node3 does NOT reach wait_primary), so it isn't exposed to this bug the same way -- its single failure is more likely independent CI timing flakiness and should be monitored separately. Reproduced locally before and after: fails identically to CI before this change (333s runtime, node3 timeout after 300s); after the fix, all 15 tests in the file pass in 173s.
dimitri
added a commit
that referenced
this pull request
Jul 21, 2026
* Fix installcheck expected outputs for PG18 and PG19
- 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 pgaftest step: copy spec to workdir, derive project from workdir
- 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'.
* Fix watch: update current_state column count guard from 16 to 17
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.
* src/bin/Makefile: serialise common archive build before parallel binaries
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.
* pgaftest --tmux: interactive shell inside pgaftest service container
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.
* Fix --tmux bottom pane: buffer overflow and sh quoting in hint command
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.
* pgaftest: add help command, show subcommands, step env fallback, PGAFTEST_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: add show step with markers, auto-advance pgaftest step
- 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.
* pgaftest: fix option ordering by setting optind=0 in pgaftest_getopts
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.
* pgaftest help: use commandline_print_command_tree like pg_autoctl help
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.
* pgaftest: mount host workDir read-write in pgaftest service container
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.
* pgaftest: run pgaftest service as docker user, deploy in its HOME
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/.
* docs: rewrite pgaftest.rst — host vs container commands, DooD, state 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.)
* pgaftest: write state file to $HOME inside container, workDir on host
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.
* pgaftest: restructure CLI — cluster subcommand, tmux command, show state/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
* pgaftest: rename PGAFTEST_COMPOSE_SERVICE to PGAFTEST_IN_CONTAINER
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).
* pg_autoctl watch --wait: retry connecting to the monitor until it's ready
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.
* build: enforce -Werror; fix tmux bottom pane disappearing on container 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
* pgaftest cluster sh: open a bash shell in the pgaftest container
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).
* pgaftest tmux: switch from exec to docker compose run; 4-pane layout
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
* pgaftest: default --work-dir from last started cluster ($TMPDIR/pgaftest/.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
* pgaftest cluster down: kill tmux session on teardown
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.
* pgaftest: grant docker socket access to container user via group_add
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.
* pgaftest tmux: name containers, check tmux exists, fix socket access
- 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.
* fix: replace log_notice (nonexistent) with log_debug for tmux version
* pgaftest: rename interactive service to 'setup', use profiles to exclude 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.
* pgaftest tmux: skip runner_wait_for_monitor (docker compose up already 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.
* pgaftest: run setup container as root, integrate build-pgaftest in make build
The setup container runs Docker-out-of-Docker (DooD) by bind-mounting
/var/run/docker.sock. Running as root eliminates GID mismatch between
macOS Docker Desktop and Linux CI, where the docker socket group ID
varies by host.
Changes:
- Dockerfile: remove docker user, sudo, entrypoint.sh; add WORKDIR /root
- Makefile.docker: add build-pgaftest and force-build-pgaftest targets,
include build-pgaftest in the default 'make build' target
* pgaftest: direct libpq paths, perform failover DSL, UX polish
compose_gen.c:
- Default to pre-built images (pg_auto_failover:pg<N>, pgaf:pgaftest);
PGAF_BUILD=1 env var forces inline build: stanzas
- Setup service: working_dir /root, spec at /root/spec.pgaf
- Emit [formation default] num_sync_standbys in monitor.ini
test_runner.c:
- monitor_get_node_state(), exec_sql_on_service(), runner_promote_one(),
runner_perform_failover(): all use direct libpq via the existing
LISTEN connection instead of docker exec on the monitor container
- New runner_perform_failover(): calls pgautofailover.perform_failover()
- Remove runner_apply_formation_settings() (replaced by monitor.ini)
- Setup/teardown: --profile setup in compose down, named shell container
%s-sh for reliable cleanup; pre-clean docker rm -f %s-sh
- runner_show_state(): add fputc('\n') after output (fixes missing newline)
- In-container fast paths: pg_autoctl show state for log_formation_state
- Periodic SQL poll inside LISTEN loop (every 5s) to catch already-achieved states
test_spec.h / test_spec_parse.y / test_spec_scan.l:
- Add CMD_FAILOVER to TestCmdKind
- Add 'perform failover [in formation F] [group G]' grammar variants
nodespec.c/.h:
- Read/write/apply num_sync_standbys from [formation ...] ini sections
cli_root.c:
- In-container quick-start note in pgaftest help output (PGAFTEST_IN_CONTAINER)
* pgaftest specs: fix promote nodeX semantics, add perform failover DSL
'promote nodeX' calls pgautofailover.perform_promotion(formation, node_name)
which targets nodeX for promotion TO primary. This is the correct DSL
command when we want a specific node to become primary.
'exec nodeX pg_autoctl perform switchover' hands off FROM nodeX (makes it
hand over its primary role). These are OPPOSITE operations — using switchover
where promote was intended caused test_003 to fail after setup because
node1 (primary) would hand off, then sql node1 would write to a secondary.
Changes across 20 .pgaf spec files:
- All 'exec nodeX pg_autoctl perform switchover' → 'promote nodeX'
- 'exec monitor pg_autoctl perform failover' → 'perform failover'
(uses new direct libpq DSL command)
* docs: update pgaftest reference for root user, direct libpq, new DSL
- DooD section: document root user setup, no entrypoint, /root home
- State file: /root/pgaftest.state (not /var/lib/postgres)
- Failover DSL: document 'perform failover [in formation F] [group G]'
- Env vars: PGAF_IMAGE, PGAFTEST_IMAGE, PGAF_BUILD
- PGAFTEST_HOST_WORK_DIR description corrected
* pgaftest: add IGNORE-BANNED comments, run citus_indent
Add /* IGNORE-BANNED */ to all getenv(), fprintf(), fopen(), printf(),
memcpy(), and related calls in the pgaftest sources and nodespec.c that
were added in the previous commits but missing the required annotation
for ci/banned.h.sh.
Also re-run citus_indent to normalise whitespace introduced by the
automated comment insertion.
* docs: overhaul pgaftest documentation
docs/ref/pgaftest.rst:
- Add ASCII architecture diagram (host / compose stack / tmux panes)
- Fix interactive session example: basic_operation.pgaf (not basic_failover)
- Move DooD architecture and step state file detail to src/bin/pgaftest/README.md;
replace with a brief pointer
- Fix TAP output example: no 'TAP version 13' header is emitted
- Fix Failover DSL section: 'promote <node>' targets that node FOR promotion
(to primary); 'exec <node> pg_autoctl perform switchover' asks it to HAND
OFF its primary role — document both correctly
docs/operations.rst:
- Remove the 50-line Testing section; replace with a 3-line stub that
cross-references the new top-level pages
docs/testing.rst (new):
- New top-level Operations page: tutorial for discovering network fault
tolerance interactively with 'pgaftest tmux basic_operation.pgaf'
- Prerequisites, CI run, step-by-step interactive walkthrough
docs/reporting-bugs.rst (new):
- New top-level Operations page: how to report a bug with a .pgaf spec
- Uses issue #997 (replication stall in 3-DC topology) as a concrete example
- Shows diagnostic commands, what to include, how to run the spec headlessly
or interactively
docs/index.rst:
- Add testing and reporting-bugs to the Operations toctree
- Add ref/pgaftest to the Manual Pages toctree (was unreachable before)
docs/fault-tolerance.rst:
- Add 'See also' section pointing to the new testing and reporting-bugs pages
src/bin/pgaftest/README.md (new):
- Internal implementation notes: DooD architecture, state file format,
direct libpq fast-paths, build targets, source file layout
* docs: fix tmux pane layout — 3 panes: logs (top), watch (middle), shell (bottom)
* docs: replace host/container table with two annotated command lists
* docs: remove duplicate ref/pgaftest from index toctree (already in ref/manual)
* docs: fix PostgreSQL version command in reporting-bugs (psql --version, not pg_autoctl version)
* docs: replace TAP output example with real captured output from multi_standbys
* docs: replace failure TAP example with captured output from guard_data_loss
* pgaftest: fall back to subprocess for promote/perform-failover; tighten CI timeouts
The direct libpq path for promote and perform failover unconditionally
required r->notifyConnected, so any CI environment where the runner
cannot reach the monitor's published host port (firewall, different
network namespace) caused immediate failure for every spec that calls
'promote' or 'perform failover'.
Replace the PQexecParams calls with exec_sql_on_service(), which already
has dual-path logic: uses the LISTEN connection when available, falls
back to docker compose exec psql otherwise. The fallback path was
already exercised by monitor_get_node_state() and all wait-until polling,
so this makes promote/perform-failover consistent with the rest of the
runner.
Also tighten CI timeouts: job 40→25 min, step 35→20 min. Schedules
complete in under 10 minutes when healthy; a 20-minute cap fails fast
without burning an entire runner-hour on a hung job.
* fixup: fix three CI failures from previous commits
Failure 1 (tablespaces): perform failover DSL is async (fires SQL, returns
immediately). The spec expected sync behaviour like the old 'exec monitor
pg_autoctl perform failover' CLI, which blocks on LISTEN until a node
reaches primary. Add an explicit 'wait until node2 state is primary' before
the first write to the new primary.
Failure 2 (citus_force_failover setup): 'exec coordinator1a pg_autoctl
perform switchover' is synchronous but the monitor stalls the worker group
FSM until the coordinator group is fully settled. Running worker switchovers
immediately after coordinator switchover races against that settling. Add
explicit 'wait until coordinator1b state is primary and coordinator1a state
is secondary' before each worker switchover.
Failure 3 (upgrade): v2.2 supervisor spawns service subprocesses via
'pg_autoctl do service {postgres,listener,node-active}'. Commit c32eb03
renamed the 'do' prefix to 'internal'. After the symlink flip in the
upgrade test, the v2.2 supervisor re-execs the shim which now delegates to
the v2.3 binary. v2.3 does not recognise 'do service ...', prints the root
help, and exits 1. The supervisor retries five times in rapid succession and
hits its crash-loop guard, killing the monitor container.
Fix: add 'do service → internal service' translation in pg_autoctl_shim.sh.
The shim comment that said v2.2 uses 'internal service' was wrong; corrected.
* fixup: rename pg_autoctl_shim.sh → pg_autoctl_compat.sh
The file is more accurately described as a version-compatibility wrapper
than a shim — it translates old command forms (v2.2 'do service') to the
current form ('internal service') in addition to delegating to the current
binary. Update Dockerfile.current COPY reference accordingly.
* fixup: add 'pg_autoctl do service' hidden alias; remove compat wrapper
v2.2 supervisor spawns service subprocesses as 'pg_autoctl do service X'.
v2.3 renamed that to 'pg_autoctl internal service X' (commit c32eb03).
During an in-place upgrade the v2.2 supervisor (PID 1) re-execs after the
symlink flip, so v2.3 must still accept the old 'do service X' form.
Fix: register 'do' as a hidden root command in v2.3, pointing to the same
do_subcommands[] as 'internal'. The routing table matches 'do service postgres',
'do service listener', and 'do service node-active' to the same callbacks.
No version detection or translation is needed.
With 'do service X' working natively in v2.3, the bash compat wrapper
(pg_autoctl_compat.sh) is entirely redundant:
- 'pg_autoctl node run <ini>' is handled natively by both v2.2 and v2.3.
- 'do service X' now works in v2.3 without translation.
Replace the compat wrapper with a plain symlink:
/usr/local/bin/pg_autoctl -> pgaf/current/pg_autoctl
The symlink resolves through pgaf/current (initially -> pgaf/2.2, after flip
-> pgaf/2.3). PG_AUTOCTL_DEBUG_BIN_PATH keeps pointing to the symlink path
so pg_autoctl_argv0 stays consistent across the flip and the version-check
re-exec works correctly.
Also drop the bash apt install from Dockerfile.current (no longer needed).
* keeper_pg_init: init monitor connection before checking if node was dropped
keeper_ensure_node_has_been_dropped() queries the monitor via libpq.
It expects keeper->monitor.pgsql.connectionType == PGSQL_CONN_MONITOR,
set by monitor_init(). In the state-file-exists code path the monitor
struct was zero-initialised (PGSQL_CONN_LOCAL = 0), so the query went to
local Postgres instead, which has no pgautofailover schema.
The v2.2 -> v2.3 upgrade exposed this: after the monitor extension
reaches 2.3, v2.2 node-active children exit(EXIT_CODE_MONITOR) and the
v2.2 supervisor re-execs them from the v2.3 binary. The re-exec runs
pg_autoctl create postgres --run, which lands in keeper_pg_init. With
a pre-existing state file the old code path called
keeper_ensure_node_has_been_dropped before any monitor_init, producing
"schema pgautofailover does not exist" with a "Postgres" error prefix
(the local-connection tell-tale). Five rapid FATAL exits tripped the
v2.2 supervisor's restart-rate limit and killed the container.
Fix: call monitor_init() immediately after local_postgres_init() so the
monitor pgsql handle carries PGSQL_CONN_MONITOR before the first query.
* upgrade: add legacy-startup DSL keyword; use compose stop/start for monitor
Adds the 'legacy-startup' cluster keyword to the pgaftest DSL. When set,
compose_gen emits 'pg_autoctl create <kind> --run' as the container command
instead of the v2.3 'pg_autoctl node run <ini>' form. This matches exactly
what production operators running v2.2 had, so the upgrade test starts the
cluster the same way real deployments do.
The upgrade spec also switches from 'pg_autoctl manual service restart
postgres' to 'compose stop monitor' + 'compose start monitor'. The in-
process restart triggered a race: the v2.2 listener reconnected to the
just-restarted Postgres at the same moment data-node keepers did, producing
transient 'schema pgautofailover does not exist' errors that exhausted the
v2.2 supervisor restart limit and killed all three data-node containers.
A full container restart gives data nodes a clean 'monitor is down, keep
retrying' signal; when the monitor comes back with the v2.3 binary (symlink
already flipped) there is no listener race.
The 'legacy-startup' keyword drives:
- test_spec.h: legacyStartup bool on TestCluster
- test_spec_scan.l / test_spec_parse.y: T_LEGACY_STARTUP token + rule
- compose_gen.c: write_legacy_monitor_command / write_legacy_node_command
emit the v2.2-style create … --run command with ssl flags spelled out
on the command line (the ini-based path is not available in v2.2)
Dockerfile.current comment updated to explain the compose stop/start
rationale and document the full upgrade mechanism.
* tests: remove unused installcheck.pgaf (installcheck runs in docker build)
* upgrade: use prev/next layout; version-agnostic install script
Dockerfile.current:
- Rename pgaf/2.1 → pgaf/prev, pgaf/2.2 → pgaf/next (version-agnostic)
- Replace symlink-based shim with a plain symlink:
/usr/local/bin/pg_autoctl → pgaf/current/pg_autoctl
- Use pgautofailover* wildcard COPY for extension files
install-extension.sh:
- Rewrite as version-agnostic using pg_config at runtime
- Copies any pgautofailover*.control / *.sql from pgaf/next/ into
the system extension directory; copies pgautofailover.so into pkglibdir
Together these make the upgrade test independent of hardcoded version
strings in filenames: the same Dockerfile and install script work for
any prev→next pair without modification.
* build: add force-build / force-build-pg$N targets with --no-cache
* pgaftest: always emit TAP plan + diagnostics on step failure; add upgrade debug queries
* upgrade: remove debug diagnostic sql queries from test_007_verify_data_intact
* monitor: fix current_state column count check (16 not 17)
The SQL query in monitor_get_current_state explicitly selects 16 columns
(formation_kind … nodecluster, plus healthlag and reportlag from the JOIN).
The check in parseCurrentNodeStateArray was left at 17 from an aborted
noderegion feature that added the column to the pgautofailover--2.2.sql
function definition but never updated the C SELECT to request it, and
never updated pgautofailover.sql (the canonical source that make rebuilds
--2.2.sql from). Result: make rebuilds --2.2.sql with 16 columns, binary
checks for 17, every pg_autoctl show state call fails.
Fix: correct the check to 16, and remove the orphaned noderegion OUT
parameter from pgautofailover--2.2.sql so it stays consistent with
pgautofailover.sql after a make install.
* service_postgres: waitpid() after pg_ctl stop to reap postgres zombie
* supervisor: log_info for orphaned subprocesses reaped when running as PID 1
When the supervisor runs as PID 1 inside a container, orphaned grandchildren
(e.g. the postgres process after the postgres service controller exits) are
reparented to it by the kernel and show up in waitpid(). This is expected
behaviour, not a bug. Log at INFO with a clear message instead of ERROR so
it does not look like an internal error in CI logs.
Non-PID-1 deployments keep the log_error to surface genuine unexpected child
reaping.
* upgrade: fix keeper restart path and test procedure
Two bugs in keeper_pg_init_and_register hit during upgrades from v2.1:
1. The state-file path (when pgdata already exists) called
keeper_ensure_node_has_been_dropped without first calling monitor_init,
causing the pgsql struct to use PGSQL_CONN_LOCAL (zero-init default)
instead of PGSQL_CONN_MONITOR. The dropped-node query then went to
local Postgres, which has no pgautofailover schema. (First fixed in
commit 7697949, now protected by the test.)
2. The same path used config->groupId straight from CLI defaults (-1),
so get_nodes('default', -1) returned no rows and the keeper wrongly
concluded it had been dropped and re-registered from scratch. Fix:
call keeper_config_read_file before the dropped-node check so the
persisted groupId, formation, and monitor_pguri are loaded from disk.
The v2.1 supervisor hits both bugs before dying (5 restarts, then exit).
The upgrade test is updated to accommodate this: after the monitor
extension is upgraded the test waits 20 s for the v2.1 containers to
exit, then restarts them with 'compose start'. The restarted containers
run the v2.2 binary (symlink already flipped) which handles the
state-file path correctly.
pgaftest fix: runner_promote_one now resolves the docker service name
(nodehost) to the pgautofailover-registered nodename before calling
perform_promotion(). v2.1 assigns sequential names (node_1, node_2, ...)
that differ from the container hostnames (node1, node2, ...) so calling
perform_promotion('default', 'node2') would fail with 'node not
registered'; the lookup by nodehost finds the correct name first.
* pgaftest: simplify promote to single subquery via nodehost
Replace the two-step lookup (resolve nodename then call perform_promotion)
with a single subquery that does both at once. UNIQUE (nodehost, nodeport)
in the schema guarantees nodehost is unique across the cluster, so no LIMIT
is needed.
* tests: fix race in drop_node_destroy test_004 exec vs run
test_004_verify_pgdata_kept used 'exec node2' which requires a running
container. After pg_autoctl drop node --no-wait, the keeper detects the
monitor-side removal on its next heartbeat and calls pg_ctl stop before
exiting, taking the container with it. By the time test_004 runs the
container is already stopped so docker compose exec returns exit 256.
Fix: add 'wait until node2 stopped' first (confirms keeper self-exited),
then use 'run node2' to access the named volume directly via a fresh
one-shot container.
* set node candidate-priority: skip apply_settings wait when no primary
monitor_wait_until_primary_applied_settings() waits for a primary/apply_settings
notification that never fires when all nodes are in report_lsn. This
happens during the test_016 scenario: all candidate-priorities set to 0,
failover triggered, all nodes enter report_lsn, then one priority is
raised. The function timed out after 60 s (PG_AUTOCTL_LISTEN_NOTIFICATIONS_TIMEOUT)
and returned false, causing the CLI to exit with EXIT_CODE_MONITOR.
Fix: before calling the wait function, scan the node list we already
fetched. If no node is in PRIMARY_STATE or WAIT_PRIMARY_STATE, skip the
wait entirely. In the no-primary case the candidate-priority change
triggers the election FSM directly (not through apply_settings), so
there is nothing to wait for — the caller can poll node states
independently.
* enable maintenance --allow-failover: use 3x timeout when demoting primary
When pg_autoctl enable maintenance --allow-failover is called on the
primary, two sequential phases must complete within the wait window:
1. Promote a secondary to primary (up to ~60 s)
2. Assign and reach MAINTENANCE_STATE on the old primary (up to ~60 s)
monitor_wait_until_node_reported_state() used PG_AUTOCTL_LISTEN_NOTIFICATIONS_
TIMEOUT (60 s) as a single hard ceiling, shared across both phases. In
CI the failover leg alone consumed ~34 s, leaving only ~26 s for the
maintenance assignment — not always enough. The timer expired, the CLI
exited with EXIT_CODE_MONITOR, and pgaftest marked the step failed.
Fix: add a timeoutSecs parameter to monitor_wait_until_node_reported_state
so callers can control the window. The enable-maintenance path passes
3 * PG_AUTOCTL_LISTEN_NOTIFICATIONS_TIMEOUT (180 s) when --allow-failover
is set on a primary. All other callers pass the original 60 s constant.
* pgaftest: add dnsmasq _dns service and static IPs for reliable container DNS
Docker's embedded DNS (127.0.0.11) relies on per-container iptables DNAT
rules that can be lost under heavy container churn on GHA, causing sporadic
'Name or service not known' failures when nodes try to reach each other by
hostname.
This change makes the generated docker-compose.yml:
- Assign a deterministic /24 subnet per project name (djb2 hash of the
project name into the 172.20-172.255 range)
- Give every service a fixed static IPv4 address within that subnet
- Add a lightweight _dns service (alpine + dnsmasq) at .2 that serves
/etc/pgaf-hosts (written next to docker-compose.yml by pgaftest)
- Point every node's 'dns:' to the dnsmasq container IP instead of
Docker's embedded resolver
IP layout within each /24:
.2 _dns (dnsmasq)
.3 monitor
.4 second_monitor (when present)
.5+ data nodes
The pgaf-hosts file is skipped when compose_gen_write() is called with
/dev/stdout (the 'pgaftest show compose' dry-run path).
* fsm: wait for local Postgres to accept connections before reporting SECONDARY
The postmaster writes PM_STATUS_READY / PM_STATUS_STANDBY to postmaster.pid
a few milliseconds before the TCP listener is actually bound.
pg_setup_wait_until_is_ready() (called via ensure_postgres_service_is_running)
returns as soon as the PID-file flag is set, leaving a narrow window where
pgIsRunning is reported as true to the monitor but connections are still
refused.
This causes two distinct CI failures:
1. Connection refused: test queries a standby immediately after it reaches
SECONDARY state, hitting the window before the TCP socket is open.
2. start_maintenance '0 candidate nodes available': CountHealthyCandidates()
called IsHealthy() on the secondary just as the health-check worker ran
and marked it BAD (because TCP was not yet open).
Fix: in the two FSM transitions that promote a standby to SECONDARY
(CATCHINGUP->SECONDARY via fsm_prepare_for_secondary, and
JOIN_SECONDARY->SECONDARY via fsm_follow_new_primary), switch the local sql
client to the init retry policy (up to 15 minutes, exponential back-off from
5 ms to 2 s) and call pgsql_get_postgres_metadata() before declaring success.
pgsql_get_postgres_metadata calls pgsql_open_connection, which falls through
to pgsql_retry_open_connection (PQping loop) when the initial PQconnectdb
fails, so no new mechanism is needed. The function also captures the current
LSN, sync state and control-file data as a useful side-effect.
tests/test_multi_alternate_primary_failures.py: test_005_002 was checking for
the transient assigned state 'report_lsn' which lasts only milliseconds; the
monitor advances through it to 'prepare_promotion' before the poll can see it.
Wait for the stable end state 'primary' instead (300s timeout).
tests/tap/specs/multi_alternate.pgaf: same fix for the pgaftest path.
* tests: catch transient report_lsn via LISTEN passing-through in pgaftest spec
The 'wait until node3 assigned-state = report_lsn' used
runner_wait_assigned_goal which does NOT pre-drain the libpq notification
buffer on entry. Under load on GHA the state can arrive and be consumed
during the preceding compose-kill exec, making it invisible to the
assigned-state poll loop.
Switch to:
wait until node3 state is primary passing through report_lsn timeout 300s
This routes through runner_wait_notify_goal which:
1. Drains the entire buffered notification queue on entry via
runner_drain_notify(), catching states that arrived while the
previous exec was blocked.
2. Tracks report_lsn as a required pass-through goalState — fails if
the monitor skipped it.
3. Waits for the stable end-state (primary reported-state) rather than
a transient millisecond assignment.
The pytest counterpart (test_multi_alternate_primary_failures.py) was
already fixed in the prior commit to poll wait_until_state('primary')
since SQL polling cannot catch transient states at all.
* compose: pass --name to legacy pg_autoctl create postgres startup
write_legacy_node_command() generated 'pg_autoctl create postgres' without
--name, causing the monitor to auto-assign node_N names from the sequence
counter. Those names are derived from the insertion order, not from the
service names in the compose file, so they diverge when nodes restart in
a different order — for example in test_008_failover_post_upgrade where
the upgrade process brings nodes up sequentially.
The registered name mismatch showed up as nodes not being found by service
name in subsequent steps.
Fix: pass the service name (n->name) as --name so the monitor always
registers the node under the same identifier as the compose service.
* dnsmasq: add Dockerfile target and use pre-built image in compose
Replace the inline 'alpine:3 + apk add dnsmasq' command in the generated
docker-compose.yml with a pre-built pgaf:dnsmasq image.
Problems with the previous approach:
- apk add runs at container start, fetching from Alpine CDN on every test
run: slow (~2-5s) and fragile if the network is slow on GHA runners.
- No healthcheck: other containers could start before dnsmasq was ready,
causing initial hostname resolution failures.
- Extra network failure mode independent of the test under execution.
Changes:
Dockerfile — new 'dnsmasq' target: FROM alpine:3, apk add dnsmasq,
HEALTHCHECK with nslookup, ENTRYPOINT that reads
/etc/pgaf-hosts via --addn-hosts and forwards to
configurable upstream DNS servers (DNS1/DNS2 env vars).
compose_gen.c — _dns service uses PGAF_DNS_IMAGE env var (default
pgaf:dnsmasq) instead of alpine:3 with inline install.
Healthcheck is emitted in the compose YAML so depends_on
can use service_healthy.
monitor, second monitor, and all data nodes gain
depends_on: _dns: condition: service_healthy
so no node starts before DNS is available.
Build:
docker build --target dnsmasq -t pgaf:dnsmasq .
Override image:
PGAF_DNS_IMAGE=my-registry/dnsmasq:latest pgaftest run spec.pgaf
* ci: build and cache pgaf:dnsmasq image; wire it to pgaftest jobs
Add a build_dnsmasq job that builds the new Dockerfile 'dnsmasq' target
once per workflow run and passes the image to every pgaftest consumer:
build_dnsmasq:
- FROM alpine:3 target, no PGVERSION dependency
- GHA layer cache (scope=pgaf-dnsmasq) — fast cache hits on re-runs
- Saves pgaf:dnsmasq to a tarball artifact (retention-days: 1)
Wire-up:
test_pgaftest — needs: [build_run_images, build_pgaftest, build_dnsmasq]
downloads + loads dnsmasq-image artifact
passes -e PGAF_DNS_IMAGE=pgaf:dnsmasq to docker run
upgrade — same: needs build_dnsmasq, downloads + loads, passes env var
The generated docker-compose.yml already uses PGAF_DNS_IMAGE (default
pgaf:dnsmasq) for the _dns service, so once the image is loaded on the
runner the variable just confirms the name. Previously the compose file
used 'alpine:3' with 'apk add dnsmasq' at startup, which fetched from
Alpine CDN on every test run and had no healthcheck, causing a race where
data nodes could start before DNS was ready.
* coordinator: set lock_timeout/statement_timeout before master_update_node
master_update_node() on the Citus coordinator takes a distributed lock
that conflicts with active queries touching the old worker's shards.
Without a timeout the call blocks indefinitely. This was observed as a
20-minute CI hang: the citus_cluster_name spec performs three switchovers
in sequence; the coordinator switchover completed in ~6s but the
immediately-following worker1a switchover blocked in coordinator_update_node_prepare
because coordinator1b (the newly-promoted coordinator) had in-flight
distributed transactions from before its own failover.
Fix: after pgsql_begin() and before the master_update_node() call, issue:
SET LOCAL lock_timeout = '<cooldown>ms';
SET LOCAL statement_timeout = '<cooldown*2>ms';
lock_timeout fires if the distributed lock isn't acquired within the
cooldown window (default 10 000 ms). statement_timeout is a backstop for
the whole call in case something else inside master_update_node stalls
after the lock is acquired. Both are SET LOCAL so they are cleared
automatically at transaction end and do not affect other uses of the
connection.
On timeout the transaction is aborted with an error; the FSM logs the
failure and retries the transition on the next keeper tick instead of
hanging forever. The no-force path (older Citus without the force
argument) was previously completely unbounded; it now also benefits from
this guard.
* monitor: detect priority-induced failover in wait-for-apply-settings
When pg_autoctl set node candidate-priority raises a node's priority
above the current primary, the monitor may trigger a full failover
instead of the usual apply_settings cycle. In that case the old
primary is assigned report_lsn (not apply_settings) as its first FSM
step, so the apply_settings wait loop never saw the new primary reach
primary/primary and timed out after 60 s.
Fix: add failoverInProgress to ApplySettingsNotificationContext. When
the known primary node is assigned report_lsn we set this flag and
switch to waiting for ANY node in the formation to reach primary/primary
instead of continuing to track the apply_settings cycle for the original
primary node.
* pgaftest: accept 'passing through' as alias for 'through' in wait-until
The grammar comment and test spec files use 'passing through' as the
natural English phrasing:
wait until node3 state is primary passing through report_lsn
but the scanner only recognised the single keyword 'through'. Add a
two-word flex rule that matches 'passing' followed by whitespace and
'through', returning a single T_THROUGH token — so both forms work
with no grammar changes. Update test_spec_scan.c accordingly.
* pgaftest: fix network reconnect IP, compose network name, and test specs
compose_gen.c: compose_network_name() was returning '<project>_default'
but the generated compose YAML defines the network as 'pgafnet', so
'docker network disconnect/connect' were failing with 'network not found'.
Fix: return '<project>_pgafnet' to match the compose YAML.
test_runner.c: 'docker network connect' without --ip lets Docker assign a
fresh DHCP address. The dnsmasq pgaf-hosts file maps each node to its
original static IP; PostgreSQL resolves HBA hostnames through that file.
When the container gets a different IP after reconnect, the HBA rule no
longer matches and streaming replication fails with 'no pg_hba.conf entry'.
Fix: read the static IP from workDir/pgaf-hosts and pass --ip to
'docker network connect' so the container re-uses its original address.
cli_enable_disable.c: align ternary operator indentation to pass citus_indent.
multi_alternate.pgaf: test_005_002 starts node2 before killing node1 so that
node2 participates in the LSN quorum. node2 was SIGKILLed in test_005_001
and never reported 'demoted'; it remained registered as a sync standby with
replication_quorum=true. With number_sync_standbys=1 the monitor needs 2
LSN reporters; without node2 back online only node3 would report, stalling
the failover indefinitely. test_005_003 no longer needs compose start node2.
multi_standbys.pgaf: increase test_013_restart_node2 timeout from 90s to 180s.
After network disconnect/reconnect, node2 must go through: contact monitor ->
monitor notifies node1 -> node1 updates HBA -> pg_rewind/basebackup from node1
-> streaming replication -> secondary. 90s was insufficient.
* pgaftest: fix citus_indent brace-style violations in runner_hosts_lookup
The previous commit's runner_hosts_lookup() used single-statement
if/while bodies without braces, which citus_indent (run via 'make
docker-check', matching CI) rejects. Run 'make docker-indent' to
apply the required brace style.
* pgaftest: rename basic_citus_operation.pgaf to citus_basic_operation.pgaf
Align with the naming convention used by every other Citus spec
(citus_cluster_name, citus_force_failover, citus_multi_standbys,
citus_skip_pg_hba) so all Citus tests are prefixed the same way.
Updates the master tests/tap/schedule list and citus-2.sch.
* pgaftest indent: fix CMD_RUN and CMD_FAILOVER corruption
print_cmd() had no case for CMD_RUN ("run <svc> <args>") or CMD_FAILOVER
("perform failover [in formation F] [group G]"), so pgaftest indent
silently rewrote any spec using either into "(unknown cmd N)" — a real
data-corruption bug for a tool meant to be a safe formatter. Both commands
are in active use: drop_node_destroy.pgaf uses run, several multi-node
specs use perform failover.
CMD_RUN mirrors the existing CMD_EXEC case. CMD_FAILOVER reconstructs
whichever of the four grammar forms matches from the stored service
(formation, "default" when omitted) and waitGroups[0] (group, 0 when
omitted).
Verified: indenting multi_standbys.pgaf and drop_node_destroy.pgaf now
round-trips perform failover / run correctly (only expected diffs remain:
dropped inline step-body comments and a redundant sequence block, both
pre-existing, documented limitations of the tool).
* pg_autoctl+pgaftest: stagger node registration by declaration order
PG_AUTOCTL_TEST_DELAY previously derived each node's registration delay
by parsing a trailing numeric suffix off its name (node1 -> 2s, node2 ->
4s, ...), so that node IDs get assigned in a predictable order across
the depends_on-guaranteed-concurrent startup of every node after the
first. That parsing only works for names ending in a digit -- Citus-style
names (worker1a, coordinator1b, ...) end in a letter and silently got
zero delay, leaving their relative registration order to a race.
compose_gen_write() now assigns each node a 0-based ordinal by its
position in cluster{} declaration order across every formation (the same
order already used to pick the depends_on anchor node) and passes it
directly via PG_AUTOCTL_TEST_DELAY, instead of a node name. cli_node_run()
just reads that number -- it never parses the node's name at all, so this
works identically for any naming convention.
Also: read the value with stringToInt() instead of atoi(), matching
convention used elsewhere in this codebase (atoi silently treats garbage
as 0; stringToInt distinguishes the two and lets a malformed value log a
warning instead of misbehaving quietly). Guard the get_env_copy() call
with env_exists() first -- the monitor service (which also runs through
cli_node_run(), and never has PG_AUTOCTL_TEST_DELAY set) would otherwise
hit get_env_copy()'s unconditional log_error() for a var that is
legitimately absent.
Verified: dry-run compose generation shows correct sequential ordinals
(0,1,2,3 for a 4-node plain formation; 0-5 for a 6-node coordinator/worker
Citus formation). Full runs of multi_standbys.pgaf (27/27),
citus_multi_standbys.pgaf (14/14), and quick.sch (4/4) all pass with zero
PG_AUTOCTL_TEST_DELAY errors.
* tests: align stop/fail/kill semantics with Python predecessor
Audited every .pgaf spec's network disconnect / compose stop / stop
postgres steps against the Python method they were ported from
(node.fail(), node.stop_pg_autoctl(), node.stop_postgres(),
node.ifdown()/ifup()) and fixed two categories of drift.
1. network disconnect substituted for a graceful process stop:
node.fail() sends SIGTERM to pg_autoctl (a graceful stop), not a
network partition -- but basic_operation.pgaf's test_010_fail_primary
and multi_standbys.pgaf's test_012_fail_primary /
test_015_002_fail_two_standby_nodes ported it as 'network disconnect'.
In basic_operation.pgaf this duplicated the file's own dedicated
partition test (test_021-023, correctly ported from ifdown()/ifup()),
silently losing coverage of the graceful-stop path. In multi_standbys
the fix also removes an internal inconsistency: test_014_002 (same
scenario, different formation config) already correctly used
'compose stop'. Fixed all three to 'compose stop' / 'compose start',
matching the sibling steps that were already correct.
2. 'stop postgres <node>' used where a real external stop was needed:
'stop postgres <node>' (pg_autoctl manual service pgctl off) writes a
persistent "expected: stopped" flag that the already-running keeper
honors -- it cannot exercise the keeper's own "ensure" self-healing
logic, because it explicitly tells the keeper not to restart. Python's
node.stop_postgres() bypasses pg_autoctl entirely (raw pg_ctl --mode
fast stop), so the keeper still believes Postgres should be running
and its own polling loop notices and restarts it -- that's the actual
behaviour under test. Switched to "exec <node> pg_ctl --wait --mode
fast stop" in:
- ensure.pgaf test_003_init_secondary (the file's whole purpose is
testing this self-healing behaviour)
- basic_operation.pgaf test_024_stop_postgres_monitor (whose own
comment already claimed to bypass pg_autoctl, but the
implementation didn't)
- basic_operation.pgaf test_008_001 and multi_maintenance.pgaf
test_006a (stop-during-maintenance checks that previously
couldn't distinguish 'maintenance suppressed the restart' from
'pgctl off already guaranteed no restart regardless')
Python's test_002_stop_postgres (stop the primary directly while it's
still a lone 'single' node, verify self-heal) is intentionally not
ported into basic_operation.pgaf: that spec's setup{} brings up all
three nodes upfront, so node1 already has a synchronous secondary by
the time any step runs, and stopping its Postgres reliably triggers a
real failover instead of a quiet self-heal -- duplicating
test_010/012 rather than adding coverage. See the comment left in
place of the step for the full reasoning.
Verified with full local runs: basic_operation.pgaf 27/27,
multi_standbys.pgaf 27/27, ensure.pgaf 5/5, multi_maintenance.pgaf 22/22.
* pgaftest: rename %CIDR% to $cidr(), add $ip(<node>) macro
Unify exec/run macro syntax under one $name(args) call convention
regardless of how a macro resolves -- $cidr() shells out to the monitor
at expansion time, $ip(<node>) is a pure file lookup, but a spec author
shouldn't need two different-looking syntaxes to tell them apart.
$ip(<node>) resolves to the static IP dnsmasq's pgaf-hosts assigns to
<node> (via the existing runner_hosts_lookup() helper, also used by
runner_network_on()'s --ip fix). Neither macro is cached: every
occurrence re-resolves independently.
* docs: pgaftest test catalog, node registration order, failure semantics
docs/ref/pgaftest.rst:
- Replace the dated Python-porting-mapping content (audit-style, tied to
a specific pass over the specs) with a permanent test catalog: every
.pgaf file under tests/tap/specs/ with a short description drawn from
its own header comment. The Python method -> .pgaf command mapping now
lives once, in README.md, rather than duplicated in both places.
- Document the five distinct ways to make a node stop responding
(network disconnect, compose stop, compose kill, stop postgres,
exec pg_ctl fast stop) and what each actually exercises -- converted
from a fragile hand-aligned RST simple-table to a definition list
after a column-width edit silently broke the table.
- Document $cidr()/$ip() macro syntax and semantics.
- New "Node registration order" section explaining the depends_on +
PG_AUTOCTL_TEST_DELAY mechanism and why it applies equally to Citus
naming.
src/bin/pgaftest/README.md:
- New "Porting failure-simulation semantics from the Python suite"
section: the Python mechanism table, the .pgaf equivalents, and the
specific audit findings (which specs were checked, which were fixed
and why, which were left as an intentional simplification).
- New "Deterministic node registration order" section: the
depends_on / PG_AUTOCTL_TEST_DELAY mechanism in full, including why an
earlier name-suffix-parsing version didn't work for Citus node names.
- "Static IP on network connect" section for the runner_network_on()
--ip fix.
Verified: rst2html reports zero real errors (only pre-existing Sphinx
:ref: roles docutils doesn't know about), and make -C docs html builds
clean with zero warnings.
* pgaftest: remove unused $cidr()/$ip() exec/run macros
Neither macro has a real caller, and the reasons behind each are gone:
- %CIDR% (this macro's predecessor) existed for exactly one caller,
installcheck.pgaf's HBA-widening step, so pg_regress could reach the
monitor over the Docker bridge subnet. That file was already deleted
before this branch started (installcheck now runs inside the Docker
build stage via pg_virtualenv, connecting over a local Unix socket --
the whole "reach the monitor over some subnet" problem it solved no
longer exists). Nothing else ever called it.
- $ip(<node>) was added speculatively earlier in this same branch, while
chasing an unrelated network-reconnect bug, on a "might be useful"
basis. It was never adopted by any spec.
Removing both: runner_expand_macros() and its two call sites in
runner_exec_cmd() (CMD_EXEC, CMD_RUN -- now just copy cmd->args
verbatim), and the macro reference table in docs/ref/pgaftest.rst.
runner_hosts_lookup() itself is untouched -- it's also used by
runner_network_on()'s --ip fix, which is real and load-bearing.
Verified: make docker-check and bash ci/banned.h.sh both clean,
make -C docs html builds with zero warnings and zero remaining mentions
of either macro, and multi_standbys.pgaf still runs 27/27.
* tests: fix test_005_002_fail_primary_again quorum stall
Reproduces 5/6 pytest/monitor CI failures (PG14, PG15, PG17, PG18, PG19),
identical on every run: "node3 failed to reach primary after 300 seconds".
Root cause: NOT pgautofailover.guard_data_loss. That GUC (added in #1142)
only changes behavior when explicitly set to false (--allow-data-loss);
its default (true) reproduces byte-for-byte the pre-#1142 blocking
behavior in ProceedGroupStateForMSFailover -- confirmed by diffing ca7834b:
every "return false;" guarding on missingNodesCount/quorumCandidateCount
already existed unconditionally before that commit, the commit only wraps
it in "if (GuardDataLoss)" and adds an unblocking else-branch that only
runs when the GUC is off. Default-on behavior is unchanged.
The real regression is commit a7974f8 ("fsm: wait for local Postgres to
accept connections before reporting SECONDARY"), landed earlier in this
branch. While fixing an unrelated connection-refused race, it also
"fixed" this test's assertion:
assert node1.wait_until_assigned_state(target_state="draining")
assert node3.wait_until_assigned_state(target_state="report_lsn")
-->
assert node3.wait_until_state(target_state="primary", timeout=300)
reasoning that 'report_lsn' is too transient to reliably observe. That's
true, but the replacement assertion is unreachable as written: at this
point in the test node2 is still down (killed in test_005_001 and not
restarted until test_005_003), so with number_sync_standbys=1 the monitor
needs 2 LSN reports before it will elect a new primary and node3 cannot
reach "primary" here -- only node2's restart in test_005_003 can unblock
that, and origin/main's version of this test correctly waits for
"report_lsn" here and defers the "primary" assertion to test_005_003.
a7974f8 turned a transient-state observability problem into a guaranteed
300s timeout by moving the wrong assertion to the wrong test function.
The .pgaf port of this same test (multi_alternate.pgaf) hit the identical
issue and was already fixed correctly in 3c9ca49, right after a7974f8:
it both moves "compose start node2" earlier (into test_005_002) *and*
adds a "passing through report_lsn" wait primitive to robustly observe
the transient state via LISTEN. This commit applies the equivalent fix to
the plain pytest side, which 3c9ca49 didn't touch: start node2 before
killing node1, so quorum is satisfiable and "wait for primary" is
reachable within test_005_002 itself. test_005_003_bring_up_first_failed_primary
no longer needs to start node2 itself, and its wait for the transient
'demoted' state (which may have already come and gone by the time the
test runs, since node2 restarts earlier now) is replaced with a wait for
the stable end state, matching the same fix pattern already used
elsewhere in this file.
test_003_002_stop_primary (the 1/6 PG16-specific failure) was not touched:
its assertions already tolerate node3 staying stuck at report_lsn (it
explicitly checks node3 does NOT reach wait_primary), so it isn't exposed
to this bug the same way -- its single failure is more likely independent
CI timing flakiness and should be monitored separately.
Reproduced locally before and after: fails identically to CI before this
change (333s runtime, node3 timeout after 300s); after the fix, all 15
tests in the file pass in 173s.
* coordinator: fix connection leak in coordinator_update_node_prepare
coordinator_update_node_prepare() runs on a single connection wrapped in
pgsql_begin(), which sets PGSQL_CONNECTION_MULTI_STATEMENT. In that mode
pgsql_execute()/pgsql_execute_with_params() deliberately keep the
connection open on failure ("Multi statements might want to ROLLBACK and
hold to the open connection for a retry step" -- pgsql.c) so the caller
can decide whether to retry or roll back.
Every failure branch in this function except one (the
transactionHasAlreadyBeenPrepared case) just did "return false;" without
calling pgsql_rollback(), so the connection was never closed. On a
formation where the coordinator keeps failing the same operation on every
FSM tick -- e.g. once per second while a worker is "draining" during a
switchover -- this leaks roughly one coordinator connection per retry.
Confirmed locally: reproduced citus_cluster_name.pgaf's worker1a
switchover hang (matches CI jobs 32/33, PG17/PG18) and polled
coordinator1b's pg_stat_activity directly. Connect…
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
During multi-standby failover,
pg_auto_failoverdrives every quorum node intoREPORT_LSNso the monitor can elect the standby with the highest LSN as new primary. If one of those quorum nodes is unreachable,BuildCandidateListincrementsmissingNodesCountandProceedGroupStateForMSFailoverrefuses to proceed — forever — because that node may have been the one that acknowledged a synchronous commit to the primary, and promoting a lagging standby would silently discard that commit.This is the correct conservative default. The problem is that there is currently no operator-visible escape: the only recovery path is raw
UPDATE pgautofailover.node SET goalstate = '...'surgery on the monitor catalog, which is undocumented and has no guardrails.Fixes #1113, #1060, #1059, #1014
Closes #1055
Solution
This PR adds two escape hatches:
1.
pgautofailover.guard_data_lossGUC (defaulttrue)When
false,ProceedGroupStateForMSFailoverbypasses both themissingNodesCountguard and thequorumCandidateCountguard, allowing failover to proceed with whatever candidates have reported their LSN. ALOGmessage is emitted for each bypassed guard so the decision is visible in the server log.2.
pg_autoctl perform failover --allow-data-lossOperator-friendly CLI wrapper. Opens a transaction, executes
SET LOCAL pgautofailover.guard_data_loss TO false, then calls the node_active protocol as normal. The GUC is local to the transaction so it cannot leak to concurrent sessions.3.
perform_failoverrescue path for stuck-in-report_lsnWhen
GetNodeToFailoverFromInGroupreturns NULL (no primary available to initiate failover from, e.g. when the primary is already draining),perform_failovernow searches for a node inREPLICATION_STATE_REPORT_LSNand callsProceedGroupStateon it directly. This handles the case where the automated FSM already started the failover but stalled.Testing
New regression test
src/monitor/sql/guard_data_loss.sql:number_sync_standbys=1)report_lsn/report_lsnguard_data_loss=true):perform_failoverreturns without promoting any candidate — s1 stays inreport_lsnguard_data_loss=false):perform_failoverselects s1 as candidate —s1.goalstatebecomesprepare_promotionAll 8 monitor regression tests pass (
make installcheck).Data-loss warning
This feature is intentionally opt-in and named to make the risk explicit. When
guard_data_loss=false:wait_primarystate if there are not enough sync standbys to satisfynumber_sync_standbysThe feature is intended for situations where the alternative (cluster stuck forever) is worse than accepting the data-loss risk consciously.