Make node region mutable; document PR #1149's region column/option#1156
Merged
Conversation
PR #1149 (issue #997, 3-DC split-brain fix) added a --region option to pg_autoctl create postgres/coordinator/worker, a matching [settings] region key in pg_autoctl_node.ini, a REGION column in `pg_autoctl watch`, and a new pgautofailover.replication_stall_timeout GUC -- none of it made it into the docs. - docs/ref/pg_autoctl_create_postgres.rst: add --region to the usage synopsis and options list (create_coordinator.rst/create_worker.rst already defer common options here, matching the existing pattern for --candidate-priority etc., so no change needed there). - docs/ref/pg_autoctl_node.rst: add region to the [settings] section reference. Verified in the source (keeper.c, nodespec.c, and the absence of a "region" case in cli_get_set_properties.c) that, unlike its section-mates candidate_priority/replication_quorum, region has no live "pg_autoctl set node region" path -- it's read from [settings] but only takes effect at node creation time. Documented that distinction explicitly and corrected the section's blanket "applied live" claim. - docs/ref/pg_autoctl_watch.rst: add the Region column to the column list. Confirmed via cli_show.c that `pg_autoctl show state` has its own, separate, unaffected column set -- only `watch` gained this column. - docs/ref/configuration.rst: add pgautofailover.replication_stall_timeout to the sample pg_settings query output and give it its own subsection (matching the existing guard_data_loss subsection), explaining the 3-DC split-brain scenario it addresses. - docs/ref/pgaftest.rst: add the `region <name>` node modifier to the DSL reference table (verified against the actual grammar rule and the replication_stall_3dc.pgaf spec's real usage: `node1 region dc1`). Also documents the manual docker-compose trick asked about separately: `compose start`/`compose stop` are DSL keywords usable only inside a spec's step{} block, with no standalone `pgaftest compose ...` command for ad hoc use from the `pgaftest tmux` bottom pane. Added a "Manually starting/ stopping a node" section showing the equivalent raw `docker compose -f $PGAFTEST_HOST_WORK_DIR/docker-compose.yml start/stop <node>` invocation, cross-referenced from the failure-simulation semantics section. Verified: `make -C docs html` builds clean; also checked with `sphinx-build -E -n` (full rebuild, nitpicky mode) -- no new warnings.
PR #1149 introduced `region` (data-centre/availability-zone label) as create-time only: a `--region` flag and a `[settings] region` key in node.ini, stored on `pgautofailover.node`, but with no way to change it on an already-registered, running node. Monitor side: - ReportAutoFailoverNodeRegion() (node_metadata.c/.h): UPDATE pgautofailover.node SET region = ... for a given node. - set_node_region() SQL function (node_active_protocol.c + pgautofailover.sql): validates the node exists and the new region is non-empty, updates it, and notifies listeners. No quorum/FSM impact, unlike candidate_priority/replication_quorum -- a straight passthrough. No extension version bump: 2.3 is still unreleased on origin/main, so this is incremental development on the current version. - Monitor regression coverage added to the existing node_active_protocol.sql/.out (not a new test file): direct set/get, unknown-node error, empty-value error. pg_autoctl CLI side: - monitor_set_node_region()/monitor_get_node_region() (monitor.c/.h). - `pg_autoctl set node region <value>` / `pg_autoctl get node region` (cli_get_set_properties.c), mirroring the existing candidate-priority/ replication-quorum commands but without their FSM-convergence wait, since region has none. Docs: pg_autoctl_node.rst flips region to "Mutable" under [settings]; new pg_autoctl_set_node_region.rst / pg_autoctl_get_node_region.rst reference pages, registered in pg_autoctl_set.rst/pg_autoctl_get.rst and pg_autoctl.rst's command listings.
… commands Testing region's node.ini live-apply path required editing a node's host-side .ini file directly (it's bind-mounted read-only inside the node's own container, so this can't go through exec/compose) -- there was no DSL primitive for that. - New `nodeini set <node> <key> <value>` / `nodeini get <node> <key> <value>` DSL commands (test_spec.h/.l/.y), sharing one file-I/O implementation (nodeini_write_value/nodeini_read_value in test_runner.c) between the DSL command execution and the new interactive commands below. - New interactive `pgaftest nodeini get|set` command, mirroring the existing `pgaftest network`/`pgaftest sql` interactive commands. - New interactive `pgaftest compose start|stop|kill|down|exec` command set, wrapping `docker compose ...` directly so the -p/-f flags don't need to be typed by hand from the tmux bottom pane -- previously documented as a manual docker compose invocation only. - docs/ref/pgaftest.rst updated: synopsis, new `pgaftest_nodeini`/ `pgaftest_compose` sub-command sections, the "Node .ini file access" DSL reference entry (with a note on Docker Desktop for macOS's virtiofs sometimes not relaying host-originated inotify events -- see the nodespec.c commit for how that's handled), and the "Manually starting/stopping a node" section now leads with the built-in command. - tests/tap/specs/config_get_set.pgaf: CLI round-trip (test_003_region_cli_roundtrip) and node.ini automated file-watch coverage (test_004_region_nodeini_live_apply) using the new `nodeini set`/`nodeini get` commands.
nodespec_apply() gains a region block matching the existing candidate_priority/replication_quorum pattern exactly: shells out to `pg_autoctl set node region --pgdata <dir> <value>` via run_program() when node.ini's region entry changes -- i.e. node.ini editing was never a second, parallel implementation for these settings, just automation that types the same CLI command a human would. Separately, hardened the watcher itself: change detection is now always CRC32C-hash based (nodespec_file_crc / NodeSpecWatcher.last_crc), with inotify (Linux only) used purely as a low-latency hint to check sooner, never trusted as the sole signal. inotify has been observed to silently miss host-originated writes to a bind-mounted node.ini under Docker Desktop for macOS's virtiofs -- the write's content lands in the container immediately, but no IN_CLOSE_WRITE event is ever raised. The hash poll (NODESPEC_HASH_POLL_INTERVAL_SECS, currently 1s) guarantees a change is picked up regardless of whether inotify fires, on any platform/filesystem.
region previously lived on KeeperConfig as a plain INI-backed option (OPTION_AUTOCTL_REGION), meaning it was read from and written to pg_autoctl.cfg and reachable via the generic `pg_autoctl config get/set pg_autoctl.region` -- a second, disconnected mutation path that never notified the monitor and (a pre-existing bug) didn't even persist reliably. candidate_priority/replication_quorum never had this generic exposure: they live on PostgresSetup.settings, merged into the config via a single direct struct-copy in keeper_config_merge_options() (config->pgSetup = pgSetup), entirely bypassing the INI option array, config file, and config get/set. region now follows the exact same path: - Moved from KeeperConfig.region to a new PostgresSetup.settings.region field (pgsetup.h), alongside candidatePriority/replicationQuorum. - OPTION_AUTOCTL_REGION and its SET_INI_OPTIONS_ARRAY entry removed entirely (keeper_config.c) -- region is no longer read from, written to, or merged through pg_autoctl.cfg in any way, and no longer reachable via keeper_config_to_json/config get/set at all. - --region at creation time now writes into LocalOptionConfig.pgSetup.settings.region (cli_common.c), riding the same direct struct-copy that already carries candidate_priority/ replication_quorum through keeper_config_merge_options. - Both monitor_register_node() call sites (keeper.c) updated to read config->pgSetup.settings.region. - `pg_autoctl config get/set pg_autoctl.region` now given a clear, dedicated error pointing to `pg_autoctl set/get node region` instead of falling through to a bare "unknown option" lookup failure (cli_config.c). The monitor remains the sole source of truth after registration, same as before. Verified with a full local Docker image rebuild (the generated docker-compose.yml has no `build:` section -- pg_autoctl:pgNN must be rebuilt manually for local source changes to take effect): config_get_set.pgaf passes reliably across repeated runs, and basic_operation.pgaf's full 27-step create/failover/maintenance/ network-partition lifecycle is unaffected.
dimitri
added
enhancement
dimitri
added a commit
that referenced
this pull request
Jul 26, 2026
CI's upgrade.pgaf job failed: an in-place ALTER EXTENSION UPDATE TO '2.3' left several functions this branch (and two other already-merged PRs) added to the fresh-install script completely absent from the incremental 2.2--2.3 upgrade script, so any node calling them post-upgrade got 'function does not exist'. Ports over, matching the fresh-install definitions in pgautofailover.sql exactly: - set_node_region() (#1156) - the four pg_version*/citus_version columns and report_postgres_version() (#1157) - node_timeline_history / accepted_timeline tables and report_timeline_history() / accept_timeline() / resolve_accepted_timeline() / node_timeline_status() (this branch) Verified with make -C tests/upgrade all (builds pgaf:current from the v2.2 tag and pgaf:next from this branch, then runs tests/tap/specs/upgrade.pgaf end to end against a live 3-node cluster) -- all 10 steps pass, including the convergence step that previously failed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #1149 introduced
region(data-centre/availability-zone label) as create-time only — a--regionflag, a[settings] regionkey in node.ini, and a column onpgautofailover.node— with no way to change it on an already-registered, running node, and with docs that hadn't caught up to the new column/option. This PR closes both gaps:regioncolumn/option and the node.ini gap acrosspg_autoctl_create_postgres,pg_autoctl_node,pg_autoctl_watch,configuration, andpgaftestreferences; documents thedocker composetrick for manually starting/stopping a node in pgaftest coverage.pg_autoctl set/get node region, and node.ini live-apply — mirroringcandidate_priority/replication_quorumexactly, including full parity (nopg_autoctl config get/setexposure, nopg_autoctl.cfgpersistence).nodeini set/getDSL primitive (needed because node.ini is bind-mounted read-only inside a node's own container) plus interactivepgaftest nodeini/pgaftest composecommands.Commits
Add region mutability: monitor SQL/C, pg_autoctl CLI, docspgaftest: nodeini set/get DSL primitive + interactive nodeini/compose commandsnodespec: live-apply region; make file-watch detection CRC-basedregion: full parity with candidate_priority/replication_quorumTesting
make docker-check(citus_indent style) andci/banned.h.sh: clean.make -C docs html: clean, no broken cross-references.node_active_protocol.sql/.out(make -C src/monitor installcheck), full 12 regress + 6 isolation tests pass.pgaftest run tests/tap/specs/config_get_set.pgaf: CLI round-trip, node.ini live-apply, all steps pass reliably across repeated runs (after rebuilding the localpg_auto_failover:pg17Docker image from source — the generated compose file has nobuild:section, so a stale image can otherwise mask changes to code that runs inside the containers).pgaftest run tests/tap/specs/basic_operation.pgaf: full 27-step create/failover/maintenance/network-partition lifecycle unaffected by the config-plumbing refactor.🤖 Generated with Claude Code