From 03b1c0b7c5d2f38c5bc5eb184f5d790ca64073ac Mon Sep 17 00:00:00 2001 From: "andres.suarez" Date: Wed, 7 Oct 2026 16:58:01 +0200 Subject: [PATCH 01/14] fix: keep source postgres down during pg_upgrade copy --- .../pg_upgrade_scripts/initiate.sh | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh b/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh index 439f2bf702..9d0a1aee0b 100755 --- a/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh +++ b/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh @@ -85,6 +85,88 @@ fi OLD_BOOTSTRAP_USER=$(run_sql -A -t -c "select rolname from pg_authid where oid = 10;") +# From step 10 until cleanup, nothing may start the source postgres: pg_upgrade +# copies the data files without a postmaster holding the directory, and a start +# in that window (Salt's `service.running`, adminapi, Restart=always, ...) +# yields a copy that is inconsistent with the dump. Two independent guards: +# 1. a runtime systemd drop-in that makes `systemctl start` fail; +# 2. a postmaster.pid in the old data dir holding pg_upgrade's own PID, which +# makes any postmaster (systemd or direct pg_ctl) refuse to start while +# pg_upgrade lives. The file goes stale when pg_upgrade exits. +PG_UPGRADE_LOCK_DROPIN_DIR="/run/systemd/system/postgresql.service.d" +PG_UPGRADE_LOCK_DROPIN="$PG_UPGRADE_LOCK_DROPIN_DIR/pg-upgrade-lock.conf" +PG_UPGRADE_LOCK_WATCHER_PID="" + +block_postgres_start() { + mkdir -p "$PG_UPGRADE_LOCK_DROPIN_DIR" + printf '[Service]\nExecStartPre=/bin/false\nRestart=no\n' >"$PG_UPGRADE_LOCK_DROPIN" + systemctl daemon-reload +} + +# Wait for the new cluster's temp server (the old one is down from then on, +# its schema dump is done) and then claim the old data dir's postmaster.pid. +# It must not exist before: pg_upgrade aborts if it finds one at startup. +start_pid_lock_watcher() { + ( + # set -E would otherwise run cleanup() from this subshell + trap - ERR + set +e + until [ -e "$PGDATANEW/postmaster.pid" ]; do sleep 0.2; done + upg_pid=$(pgrep -u postgres -f -- "--old-datadir=${PGDATAOLD}" | head -1) + if [ -z "$upg_pid" ]; then + exit 0 + fi + if [ -e "$PGDATAOLD/postmaster.pid" ]; then + log "WARNING: $PGDATAOLD/postmaster.pid already exists; not creating the pg_upgrade lock" + exit 0 + fi + printf '%s\n' "$upg_pid" >"$PGDATAOLD/postmaster.pid" + chown postgres:postgres "$PGDATAOLD/postmaster.pid" + chmod 0600 "$PGDATAOLD/postmaster.pid" + log "Locked $PGDATAOLD with pg_upgrade pid $upg_pid" + ) & + PG_UPGRADE_LOCK_WATCHER_PID=$! +} + +# Idempotent: cleanup() can run before step 10 or more than once. +unblock_postgres_start() { + if [ -n "$PG_UPGRADE_LOCK_WATCHER_PID" ]; then + kill "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null || true + PG_UPGRADE_LOCK_WATCHER_PID="" + fi + # Only remove our own one-line file, never a real postmaster's (6+ lines). + local pidfile="${PGDATAOLD:-/nonexistent}/postmaster.pid" + if [ -f "$pidfile" ] && [ "$(wc -l <"$pidfile")" -le 1 ]; then + rm -f "$pidfile" + fi + if [ -z "$IS_CI" ]; then + if [ -e "$PG_UPGRADE_LOCK_DROPIN" ]; then + rm -f "$PG_UPGRADE_LOCK_DROPIN" + rmdir --ignore-fail-on-non-empty "$PG_UPGRADE_LOCK_DROPIN_DIR" || true + systemctl daemon-reload + fi + systemctl reset-failed postgresql || true + fi +} + +# The source must have stayed down for the whole copy; if not, the copy is +# inconsistent and the upgrade has to fail (the source data is untouched). +assert_source_stayed_down() { + local state="" + if [ -z "$IS_CI" ]; then + state=$(systemctl is-active postgresql || true) + fi + if [ "$state" = "active" ] || [ "$state" = "activating" ] || [ "$state" = "reloading" ]; then + log "ERROR: postgresql.service is ${state} after pg_upgrade; the copied data files may be inconsistent" + return 1 + fi + # A postmaster writes 6+ lines; our lock is a single line. + if [ -f "$PGDATAOLD/postmaster.pid" ] && [ "$(wc -l <"$PGDATAOLD/postmaster.pid")" -gt 1 ]; then + log "ERROR: a postmaster ran in $PGDATAOLD during pg_upgrade; the copied data files may be inconsistent" + return 1 + fi +} + cleanup() { UPGRADE_STATUS=${1:-"failed"} EXIT_CODE=${?:-0} @@ -128,6 +210,8 @@ cleanup() { fi fi + unblock_postgres_start + log "Restarting postgresql" if [ -z "$IS_CI" ]; then systemctl enable postgresql @@ -616,10 +700,13 @@ EOF sleep 3 systemctl stop postgresql + block_postgres_start else CI_stop_postgres fi + start_pid_lock_watcher + # Start the old PostgreSQL instance with version-specific options if [[ ${PGVERSION%%.*} -ge 16 ]]; then GRN_PLUGINS_DIR=/var/lib/postgresql/.nix-profile/lib/groonga/plugins LC_ALL=en_US.UTF-8 LANGUAGE=en_US.UTF-8 LANG=en_US.UTF-8 LOCALE_ARCHIVE=/usr/lib/locale/locale-archive su -pc "$UPGRADE_COMMAND" -s "$SHELL" postgres @@ -627,6 +714,10 @@ EOF GRN_PLUGINS_DIR=/var/lib/postgresql/.nix-profile/lib/groonga/plugins LC_ALL=en_US.UTF-8 LC_CTYPE=$SERVER_LC_CTYPE LC_COLLATE=$SERVER_LC_COLLATE LANGUAGE=en_US.UTF-8 LANG=en_US.UTF-8 LOCALE_ARCHIVE=/usr/lib/locale/locale-archive su -pc "$UPGRADE_COMMAND" -s "$SHELL" postgres fi + if ! assert_source_stayed_down; then + false # not exit: let the ERR trap run cleanup + fi + # copying custom configurations log "11. Copying custom configurations" mkdir -p "$MOUNT_POINT/conf" From 160fbbb3ae8dd2ca4aa188911d8a02350c4d9e02 Mon Sep 17 00:00:00 2001 From: "andres.suarez" Date: Wed, 7 Oct 2026 17:20:05 +0200 Subject: [PATCH 02/14] fix: address review findings in pg_upgrade source-start guard --- .../pg_upgrade_scripts/initiate.sh | 59 ++++++++++++++----- 1 file changed, 44 insertions(+), 15 deletions(-) diff --git a/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh b/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh index 9d0a1aee0b..8128a480f2 100755 --- a/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh +++ b/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh @@ -99,7 +99,9 @@ PG_UPGRADE_LOCK_WATCHER_PID="" block_postgres_start() { mkdir -p "$PG_UPGRADE_LOCK_DROPIN_DIR" - printf '[Service]\nExecStartPre=/bin/false\nRestart=no\n' >"$PG_UPGRADE_LOCK_DROPIN" + # The empty ExecStartPre= resets the unit's own list: otherwise a blocked start + # would still run postgres_prestart.sh (as root) before /bin/false fails it. + printf '[Service]\nExecStartPre=\nExecStartPre=/bin/false\nRestart=no\n' >"$PG_UPGRADE_LOCK_DROPIN" systemctl daemon-reload } @@ -109,7 +111,7 @@ block_postgres_start() { start_pid_lock_watcher() { ( # set -E would otherwise run cleanup() from this subshell - trap - ERR + trap - ERR EXIT set +e until [ -e "$PGDATANEW/postmaster.pid" ]; do sleep 0.2; done upg_pid=$(pgrep -u postgres -f -- "--old-datadir=${PGDATAOLD}" | head -1) @@ -139,14 +141,18 @@ unblock_postgres_start() { if [ -f "$pidfile" ] && [ "$(wc -l <"$pidfile")" -le 1 ]; then rm -f "$pidfile" fi + # Callers run this under `||` where set -e is off, so report failures through + # the return status: a failed daemon-reload leaves the block loaded in systemd. + local rc=0 if [ -z "$IS_CI" ]; then if [ -e "$PG_UPGRADE_LOCK_DROPIN" ]; then - rm -f "$PG_UPGRADE_LOCK_DROPIN" + rm -f "$PG_UPGRADE_LOCK_DROPIN" || rc=1 rmdir --ignore-fail-on-non-empty "$PG_UPGRADE_LOCK_DROPIN_DIR" || true - systemctl daemon-reload + systemctl daemon-reload || rc=1 fi systemctl reset-failed postgresql || true fi + return $rc } # The source must have stayed down for the whole copy; if not, the copy is @@ -167,7 +173,10 @@ assert_source_stayed_down() { fi } +CLEANUP_STARTED="" + cleanup() { + CLEANUP_STARTED=1 UPGRADE_STATUS=${1:-"failed"} EXIT_CODE=${?:-0} @@ -188,16 +197,8 @@ cleanup() { enable_conflicting_timers || log "WARNING: failed to re-enable one or more timers; check 'systemctl list-timers --all' on this host" fi - if [ -d "${MOUNT_POINT}/pgdata/pg_upgrade_output.d/" ]; then - log "Copying pg_upgrade output to /var/log" - cp -R "${MOUNT_POINT}/pgdata/pg_upgrade_output.d/" /var/log/ || true - chown -R postgres:postgres /var/log/pg_upgrade_output.d/ - chmod -R 0750 /var/log/pg_upgrade_output.d/ - ship_logs "$LOG_FILE" || true - tail -n +1 /var/log/pg_upgrade_output.d/*/* >/var/log/pg_upgrade_output.d/pg_upgrade.log || true - ship_logs "/var/log/pg_upgrade_output.d/pg_upgrade.log" || true - fi - + # Put the old share dir back before the start block is lifted, so a start that + # slips in right after it cannot see the new version's files. if [ -L "/usr/share/postgresql/${PGVERSION}" ]; then rm "/usr/share/postgresql/${PGVERSION}" @@ -210,7 +211,19 @@ cleanup() { fi fi - unblock_postgres_start + # Same reasoning as the timers: lift the start block before anything else that + # can fail under set -e, or the restart below would be refused. + unblock_postgres_start || log "WARNING: failed to lift the postgres start block; check /run/systemd/system/postgresql.service.d" + + if [ -d "${MOUNT_POINT}/pgdata/pg_upgrade_output.d/" ]; then + log "Copying pg_upgrade output to /var/log" + cp -R "${MOUNT_POINT}/pgdata/pg_upgrade_output.d/" /var/log/ || true + chown -R postgres:postgres /var/log/pg_upgrade_output.d/ + chmod -R 0750 /var/log/pg_upgrade_output.d/ + ship_logs "$LOG_FILE" || true + tail -n +1 /var/log/pg_upgrade_output.d/*/* >/var/log/pg_upgrade_output.d/pg_upgrade.log || true + ship_logs "/var/log/pg_upgrade_output.d/pg_upgrade.log" || true + fi log "Restarting postgresql" if [ -z "$IS_CI" ]; then @@ -267,6 +280,17 @@ EOF fi } +# EXIT trap: any exit that did not go through cleanup() (set -u abort, TERM/INT) +# still restores the source, since step 10 left it stopped and disabled. If +# cleanup() already ran (or died part way under set -e), only make sure the +# start block is gone. +on_exit() { + if [ -z "$CLEANUP_STARTED" ]; then + cleanup failed + fi + unblock_postgres_start || true +} + function handle_extensions { if [ -z "$IS_CI" ]; then retry 5 systemctl restart postgresql @@ -317,6 +341,11 @@ EOF } function initiate_upgrade { + # This runs in a background subshell, where the parent's traps do not apply. + trap on_exit EXIT + trap 'exit 143' TERM + trap 'exit 130' INT + # Before anything destructive: no || true here, a timer firing mid-upgrade is # exactly what this guards against, and failing now leaves the project untouched. # Interrupting an apt-daily run can leave dpkg half-configured; step 2's From be0c0c77a34b82f8853486238f1f64657e4da5e9 Mon Sep 17 00:00:00 2001 From: "andres.suarez" Date: Wed, 7 Oct 2026 17:57:27 +0200 Subject: [PATCH 03/14] test: cover pg_upgrade start guard and harden exit cleanup --- .../pg_upgrade_scripts/initiate.sh | 64 ++-- nix/docs/testing-pg-upgrade-scripts.md | 14 + tests/pg_upgrade/test-start-guard.sh | 281 ++++++++++++++++++ 3 files changed, 342 insertions(+), 17 deletions(-) create mode 100644 tests/pg_upgrade/test-start-guard.sh diff --git a/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh b/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh index 8128a480f2..fb4956eac4 100755 --- a/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh +++ b/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh @@ -38,6 +38,7 @@ IS_NIX_UPGRADE=${IS_NIX_UPGRADE:-} IS_NIX_BASED_SYSTEM="false" PGVERSION=$1 +POSTGRES_SHARE_DIR="/usr/share/postgresql/${PGVERSION}" MOUNT_POINT="/data_migration" LOG_FILE="/var/log/pg-upgrade-initiate.log" @@ -95,13 +96,16 @@ OLD_BOOTSTRAP_USER=$(run_sql -A -t -c "select rolname from pg_authid where oid = # pg_upgrade lives. The file goes stale when pg_upgrade exits. PG_UPGRADE_LOCK_DROPIN_DIR="/run/systemd/system/postgresql.service.d" PG_UPGRADE_LOCK_DROPIN="$PG_UPGRADE_LOCK_DROPIN_DIR/pg-upgrade-lock.conf" +PG_UPGRADE_PID_LOCK_MARKER="/run/pg-upgrade-source.pid" PG_UPGRADE_LOCK_WATCHER_PID="" +PG_UPGRADE_START_BLOCKED="" block_postgres_start() { mkdir -p "$PG_UPGRADE_LOCK_DROPIN_DIR" # The empty ExecStartPre= resets the unit's own list: otherwise a blocked start # would still run postgres_prestart.sh (as root) before /bin/false fails it. printf '[Service]\nExecStartPre=\nExecStartPre=/bin/false\nRestart=no\n' >"$PG_UPGRADE_LOCK_DROPIN" + PG_UPGRADE_START_BLOCKED=1 systemctl daemon-reload } @@ -122,7 +126,14 @@ start_pid_lock_watcher() { log "WARNING: $PGDATAOLD/postmaster.pid already exists; not creating the pg_upgrade lock" exit 0 fi - printf '%s\n' "$upg_pid" >"$PGDATAOLD/postmaster.pid" + printf '%s\n' "$upg_pid" >"$PG_UPGRADE_PID_LOCK_MARKER" || exit 1 + if ! ( + set -o noclobber + cat "$PG_UPGRADE_PID_LOCK_MARKER" >"$PGDATAOLD/postmaster.pid" + ); then + log "WARNING: could not create the pg_upgrade PID lock" + exit 1 + fi chown postgres:postgres "$PGDATAOLD/postmaster.pid" chmod 0600 "$PGDATAOLD/postmaster.pid" log "Locked $PGDATAOLD with pg_upgrade pid $upg_pid" @@ -134,23 +145,32 @@ start_pid_lock_watcher() { unblock_postgres_start() { if [ -n "$PG_UPGRADE_LOCK_WATCHER_PID" ]; then kill "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null || true + wait "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null || true PG_UPGRADE_LOCK_WATCHER_PID="" fi - # Only remove our own one-line file, never a real postmaster's (6+ lines). local pidfile="${PGDATAOLD:-/nonexistent}/postmaster.pid" - if [ -f "$pidfile" ] && [ "$(wc -l <"$pidfile")" -le 1 ]; then - rm -f "$pidfile" - fi # Callers run this under `||` where set -e is off, so report failures through # the return status: a failed daemon-reload leaves the block loaded in systemd. local rc=0 + if [ -f "$PG_UPGRADE_PID_LOCK_MARKER" ]; then + if [ -f "$pidfile" ] && cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$pidfile"; then + rm -f "$pidfile" || rc=1 + fi + if [ "$rc" -eq 0 ]; then + rm -f "$PG_UPGRADE_PID_LOCK_MARKER" || rc=1 + fi + fi if [ -z "$IS_CI" ]; then if [ -e "$PG_UPGRADE_LOCK_DROPIN" ]; then + PG_UPGRADE_START_BLOCKED=1 rm -f "$PG_UPGRADE_LOCK_DROPIN" || rc=1 rmdir --ignore-fail-on-non-empty "$PG_UPGRADE_LOCK_DROPIN_DIR" || true - systemctl daemon-reload || rc=1 fi - systemctl reset-failed postgresql || true + systemctl daemon-reload || rc=1 + if [ "$rc" -eq 0 ] && [ -n "$PG_UPGRADE_START_BLOCKED" ]; then + systemctl reset-failed postgresql || true + PG_UPGRADE_START_BLOCKED="" + fi fi return $rc } @@ -166,8 +186,7 @@ assert_source_stayed_down() { log "ERROR: postgresql.service is ${state} after pg_upgrade; the copied data files may be inconsistent" return 1 fi - # A postmaster writes 6+ lines; our lock is a single line. - if [ -f "$PGDATAOLD/postmaster.pid" ] && [ "$(wc -l <"$PGDATAOLD/postmaster.pid")" -gt 1 ]; then + if [ -f "$PGDATAOLD/postmaster.pid" ] && ! cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$PGDATAOLD/postmaster.pid"; then log "ERROR: a postmaster ran in $PGDATAOLD during pg_upgrade; the copied data files may be inconsistent" return 1 fi @@ -199,15 +218,15 @@ cleanup() { # Put the old share dir back before the start block is lifted, so a start that # slips in right after it cannot see the new version's files. - if [ -L "/usr/share/postgresql/${PGVERSION}" ]; then - rm "/usr/share/postgresql/${PGVERSION}" + if [ -L "$POSTGRES_SHARE_DIR" ]; then + rm "$POSTGRES_SHARE_DIR" || log "WARNING: failed to remove the postgres share symlink" - if [ -f "/usr/share/postgresql/${PGVERSION}.bak" ]; then - mv "/usr/share/postgresql/${PGVERSION}.bak" "/usr/share/postgresql/${PGVERSION}" + if [ -f "${POSTGRES_SHARE_DIR}.bak" ]; then + mv "${POSTGRES_SHARE_DIR}.bak" "$POSTGRES_SHARE_DIR" || log "WARNING: failed to restore the postgres share directory" fi - if [ -d "/usr/share/postgresql/${PGVERSION}.bak" ]; then - mv "/usr/share/postgresql/${PGVERSION}.bak" "/usr/share/postgresql/${PGVERSION}" + if [ -d "${POSTGRES_SHARE_DIR}.bak" ]; then + mv "${POSTGRES_SHARE_DIR}.bak" "$POSTGRES_SHARE_DIR" || log "WARNING: failed to restore the postgres share directory" fi fi @@ -285,10 +304,21 @@ EOF # cleanup() already ran (or died part way under set -e), only make sure the # start block is gone. on_exit() { + local exit_code=$? + trap - ERR + set +e if [ -z "$CLEANUP_STARTED" ]; then - cleanup failed + ( + trap - ERR EXIT + set -e + cleanup failed + ) + if [ "$exit_code" -eq 0 ]; then + exit_code=1 + fi fi - unblock_postgres_start || true + unblock_postgres_start || log "WARNING: failed to lift the postgres start block; check /run/systemd/system/postgresql.service.d" + exit "$exit_code" } function handle_extensions { diff --git a/nix/docs/testing-pg-upgrade-scripts.md b/nix/docs/testing-pg-upgrade-scripts.md index 9aa9cc7306..d72064ba81 100644 --- a/nix/docs/testing-pg-upgrade-scripts.md +++ b/nix/docs/testing-pg-upgrade-scripts.md @@ -2,6 +2,20 @@ This document describes how to test changes to the PostgreSQL upgrade scripts on a running machine. +## Startup Guard Unit Tests + +Run the deterministic startup-guard tests locally on macOS or Linux: + +```bash +bash tests/pg_upgrade/test-start-guard.sh +``` + +The suite uses temporary directories and command shims, not a live PostgreSQL +instance or systemd. It covers PID lock ownership, drop-in install/removal, +reload retries, share-directory restoration failures, and ERR/EXIT/TERM/INT +cleanup. It is not run in CI. A full non-CI upgrade still requires a Linux +staging host with real systemd. + ## Prerequisites - A running PostgreSQL instance diff --git a/tests/pg_upgrade/test-start-guard.sh b/tests/pg_upgrade/test-start-guard.sh new file mode 100644 index 0000000000..e9ba6aba1d --- /dev/null +++ b/tests/pg_upgrade/test-start-guard.sh @@ -0,0 +1,281 @@ +#!/usr/bin/env bash + +set -eEuo pipefail + +REPO_ROOT=$(cd "$(dirname "$0")/../.." && pwd) +TEST_ROOT=$(mktemp -d) +trap 'rm -rf "$TEST_ROOT"' EXIT + +eval "$(awk ' + /^(block_postgres_start|start_pid_lock_watcher|unblock_postgres_start|assert_source_stayed_down|cleanup|on_exit)\(\) \{/ { printing = 1 } + printing { print } + printing && /^}/ { printing = 0 } +' "$REPO_ROOT/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh")" + +mkdir "$TEST_ROOT/bin" +cat >"$TEST_ROOT/bin/systemctl" <<'SHIM' +#!/usr/bin/env bash +printf '%s\n' "$*" >>"$SYSTEMCTL_LOG" +case "$1" in +daemon-reload) + if [ -f "$FAIL_RELOAD_ONCE" ]; then + rm "$FAIL_RELOAD_ONCE" + exit 1 + fi + ;; +is-active) + printf '%s\n' "${SERVICE_STATE:-inactive}" + ;; +esac +SHIM +cat >"$TEST_ROOT/bin/pgrep" <<'SHIM' +#!/usr/bin/env bash +printf '12345\n' +SHIM +cat >"$TEST_ROOT/bin/chown" <<'SHIM' +#!/usr/bin/env bash +exit 0 +SHIM +cat >"$TEST_ROOT/bin/rmdir" <<'SHIM' +#!/usr/bin/env bash +if [ "$1" = --ignore-fail-on-non-empty ]; then + shift +fi +exec /bin/rmdir "$@" +SHIM +cat >"$TEST_ROOT/bin/rm" <<'SHIM' +#!/usr/bin/env bash +if [ "${FAIL_SHARE_RESTORE:-}" = rm ] && [ "$1" = "$POSTGRES_SHARE_DIR" ]; then + printf 'restore-rm\n' >>"$SYSTEMCTL_LOG" + exit 1 +fi +exec /bin/rm "$@" +SHIM +cat >"$TEST_ROOT/bin/mv" <<'SHIM' +#!/usr/bin/env bash +if [ "${FAIL_SHARE_RESTORE:-}" = mv ] && [ "$1" = "${POSTGRES_SHARE_DIR}.bak" ]; then + printf 'restore-mv\n' >>"$SYSTEMCTL_LOG" + exit 1 +fi +exec /bin/mv "$@" +SHIM +chmod +x "$TEST_ROOT/bin/"* +export PATH="$TEST_ROOT/bin:$PATH" + +log() { printf '%s\n' "$*"; } +retry() { + printf 'cleanup-abort\n' >>"$SYSTEMCTL_LOG" + exit 1 +} + +setup() { + local test_dir + test_dir=$(mktemp -d "$TEST_ROOT/case.XXXXXX") + unset IS_CI + export IS_CI=${IS_CI:-} + IS_LOCAL_UPGRADE=true + PGVERSION="guard-test-$$" + PGDATAOLD="$test_dir/old" + PGDATANEW="$test_dir/new" + MOUNT_POINT="$test_dir/mount" + PG_UPGRADE_LOCK_DROPIN_DIR="$test_dir/postgresql.service.d" + PG_UPGRADE_LOCK_DROPIN="$PG_UPGRADE_LOCK_DROPIN_DIR/pg-upgrade-lock.conf" + PG_UPGRADE_PID_LOCK_MARKER="$test_dir/pg-upgrade-source.pid" + PG_UPGRADE_LOCK_WATCHER_PID="" + PG_UPGRADE_START_BLOCKED="" + CLEANUP_STARTED="" + POSTGRES_SHARE_DIR="$test_dir/share" + POST_UPGRADE_EXTENSION_SCRIPT="$test_dir/extensions.sql" + POST_UPGRADE_POSTGRES_PERMS_SCRIPT="$test_dir/perms.sql" + LOG_FILE="$test_dir/upgrade.log" + export IS_LOCAL_UPGRADE PGVERSION PGDATAOLD PGDATANEW MOUNT_POINT LOG_FILE + export POSTGRES_SHARE_DIR POST_UPGRADE_EXTENSION_SCRIPT POST_UPGRADE_POSTGRES_PERMS_SCRIPT + export PG_UPGRADE_LOCK_DROPIN_DIR PG_UPGRADE_LOCK_DROPIN PG_UPGRADE_PID_LOCK_MARKER + export PG_UPGRADE_LOCK_WATCHER_PID PG_UPGRADE_START_BLOCKED CLEANUP_STARTED + export SYSTEMCTL_LOG="$test_dir/systemctl.log" + export FAIL_RELOAD_ONCE="$test_dir/fail-reload" + export FAIL_SHARE_RESTORE="" + export SERVICE_STATE=inactive + mkdir "$PGDATAOLD" "$PGDATANEW" + : >"$SYSTEMCTL_LOG" +} + +assert() { + if ! "$@"; then + printf 'FAIL: %s\n' "$*" >&2 + exit 1 + fi +} + +test_dropin() { + setup + block_postgres_start + assert grep -qx 'ExecStartPre=' "$PG_UPGRADE_LOCK_DROPIN" + assert grep -qx 'ExecStartPre=/bin/false' "$PG_UPGRADE_LOCK_DROPIN" + assert grep -qx 'Restart=no' "$PG_UPGRADE_LOCK_DROPIN" + unblock_postgres_start + assert test ! -e "$PG_UPGRADE_LOCK_DROPIN" + assert test "$(grep -c '^daemon-reload$' "$SYSTEMCTL_LOG")" -eq 2 + assert grep -qx 'reset-failed postgresql' "$SYSTEMCTL_LOG" + unblock_postgres_start + assert test "$(grep -c '^reset-failed postgresql$' "$SYSTEMCTL_LOG")" -eq 1 +} + +test_no_block_reset() { + setup + unblock_postgres_start + if grep -q '^reset-failed postgresql$' "$SYSTEMCTL_LOG"; then + exit 1 + fi +} + +test_block_reload_failure() { + setup + touch "$FAIL_RELOAD_ONCE" + if block_postgres_start; then + exit 1 + fi + assert test -f "$PG_UPGRADE_LOCK_DROPIN" + unblock_postgres_start + assert test ! -e "$PG_UPGRADE_LOCK_DROPIN" +} + +test_unblock_reload_retry() { + setup + block_postgres_start + touch "$FAIL_RELOAD_ONCE" + if unblock_postgres_start; then + exit 1 + fi + assert test ! -e "$PG_UPGRADE_LOCK_DROPIN" + unblock_postgres_start + assert test "$(grep -c '^daemon-reload$' "$SYSTEMCTL_LOG")" -eq 3 +} + +test_owned_pid() { + setup + touch "$PGDATANEW/postmaster.pid" + start_pid_lock_watcher + wait "$PG_UPGRADE_LOCK_WATCHER_PID" + assert cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$PGDATAOLD/postmaster.pid" + assert_source_stayed_down + unblock_postgres_start + assert test ! -e "$PGDATAOLD/postmaster.pid" + assert test ! -e "$PG_UPGRADE_PID_LOCK_MARKER" +} + +test_foreign_pid() { + local content + for content in '' '54321' $'54321\n/data'; do + setup + printf '12345\n' >"$PG_UPGRADE_PID_LOCK_MARKER" + printf '%s' "$content" >"$PGDATAOLD/postmaster.pid" + if assert_source_stayed_down; then + exit 1 + fi + unblock_postgres_start + assert test -f "$PGDATAOLD/postmaster.pid" + assert test "$(cat "$PGDATAOLD/postmaster.pid")" = "$content" + done + setup + printf '12345\n' >"$PGDATAOLD/postmaster.pid" + if assert_source_stayed_down; then + exit 1 + fi + unblock_postgres_start + assert test -f "$PGDATAOLD/postmaster.pid" +} + +test_watcher_existing_pid() { + setup + touch "$PGDATANEW/postmaster.pid" + printf '54321\n' >"$PGDATAOLD/postmaster.pid" + start_pid_lock_watcher + wait "$PG_UPGRADE_LOCK_WATCHER_PID" + unblock_postgres_start + assert test "$(cat "$PGDATAOLD/postmaster.pid")" = 54321 +} + +test_active_source() { + local state + for state in active activating reloading; do + setup + export SERVICE_STATE="$state" + if assert_source_stayed_down; then + exit 1 + fi + done +} + +test_exit_cleanup() { + local trigger exit_code expected_code + for trigger in failure TERM INT clean-exit cleanup-exit already-cleaned restore-rm restore-mv; do + setup + mkdir "${POSTGRES_SHARE_DIR}.bak" "$POSTGRES_SHARE_DIR.new" + ln -s "$POSTGRES_SHARE_DIR.new" "$POSTGRES_SHARE_DIR" + exit_code=0 + expected_code=1 + case "$trigger" in + TERM | cleanup-exit) expected_code=143 ;; + INT) expected_code=130 ;; + already-cleaned) expected_code=0 ;; + restore-rm) export FAIL_SHARE_RESTORE=rm ;; + restore-mv) export FAIL_SHARE_RESTORE=mv ;; + esac + "$BASH" -s "$trigger" <<-'TRAP_TEST' || exit_code=$? + set -eEuo pipefail + if [ "$1" = cleanup-exit ]; then + cleanup() { + printf "cleanup-abort\n" >>"$SYSTEMCTL_LOG" + exit 1 + } + fi + trap on_exit EXIT + trap "cleanup failed" ERR + trap "exit 143" TERM + trap "exit 130" INT + block_postgres_start + touch "$FAIL_RELOAD_ONCE" + case "$1" in + TERM | INT) kill -s "$1" "$$" ;; + clean-exit) exit 0 ;; + cleanup-exit) rm "$FAIL_RELOAD_ONCE"; exit 143 ;; + already-cleaned) rm "$FAIL_RELOAD_ONCE"; CLEANUP_STARTED=1; exit 0 ;; + *) false ;; + esac + TRAP_TEST + assert test "$exit_code" -eq "$expected_code" + assert test ! -e "$PG_UPGRADE_LOCK_DROPIN" + assert test ! -e "$FAIL_RELOAD_ONCE" + assert test "$(tail -n 2 "$SYSTEMCTL_LOG")" = $'daemon-reload\nreset-failed postgresql' + case "$trigger" in + already-cleaned) + assert test "$(grep -c '^daemon-reload$' "$SYSTEMCTL_LOG")" -eq 2 + if grep -q '^cleanup-abort$' "$SYSTEMCTL_LOG"; then exit 1; fi + ;; + cleanup-exit) + assert test "$(grep -c '^daemon-reload$' "$SYSTEMCTL_LOG")" -eq 2 + assert test "$(grep -c '^cleanup-abort$' "$SYSTEMCTL_LOG")" -eq 1 + ;; + *) + assert test "$(grep -c '^daemon-reload$' "$SYSTEMCTL_LOG")" -eq 3 + assert test "$(grep -c '^cleanup-abort$' "$SYSTEMCTL_LOG")" -eq 1 + case "$trigger" in + restore-rm | restore-mv) assert grep -qx "$trigger" "$SYSTEMCTL_LOG" ;; + *) assert test ! -L "$POSTGRES_SHARE_DIR" ;; + esac + ;; + esac + done +} + +export -f log retry block_postgres_start unblock_postgres_start cleanup on_exit +for test_name in test_dropin test_no_block_reset test_block_reload_failure test_unblock_reload_retry test_owned_pid test_foreign_pid test_watcher_existing_pid test_active_source test_exit_cleanup; do + case_output="$TEST_ROOT/$test_name.log" + trap 'exit_code=$?; cat "$case_output"; exit "$exit_code"' ERR + ( + trap - ERR + "$test_name" + ) >"$case_output" 2>&1 + printf 'PASS: %s\n' "$test_name" +done From 818dd001041e5ccfdab8a968f034f8ead09c0587 Mon Sep 17 00:00:00 2001 From: "andres.suarez" Date: Fri, 9 Oct 2026 11:43:59 +0200 Subject: [PATCH 04/14] fix: make the pg_upgrade start guard self-expiring Replace the ExecStartPre=/bin/false drop-in with an ExecCondition guard that refuses a start only while initiate.sh or pg_upgrade is alive, so a SIGKILLed initiate.sh no longer leaves the source blocked until a reboot. The guard runs as root (`+`) because the unit runs as postgres, and a start that gets past it removes the drop-in via ExecStartPost so Restart=no does not outlive the block. Co-Authored-By: Claude Sonnet 5.5 --- .../pg_upgrade_scripts/initiate.sh | 35 +++++++++-- tests/pg_upgrade/test-start-guard.sh | 60 ++++++++++++++++++- 2 files changed, 88 insertions(+), 7 deletions(-) diff --git a/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh b/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh index fb4956eac4..2c573d0bfe 100755 --- a/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh +++ b/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh @@ -90,21 +90,47 @@ OLD_BOOTSTRAP_USER=$(run_sql -A -t -c "select rolname from pg_authid where oid = # copies the data files without a postmaster holding the directory, and a start # in that window (Salt's `service.running`, adminapi, Restart=always, ...) # yields a copy that is inconsistent with the dump. Two independent guards: -# 1. a runtime systemd drop-in that makes `systemctl start` fail; +# 1. a runtime systemd drop-in that makes `systemctl start` fail while this +# script or pg_upgrade is alive (a SIGKILLed script must not leave the +# source blocked until a reboot); # 2. a postmaster.pid in the old data dir holding pg_upgrade's own PID, which # makes any postmaster (systemd or direct pg_ctl) refuse to start while # pg_upgrade lives. The file goes stale when pg_upgrade exits. PG_UPGRADE_LOCK_DROPIN_DIR="/run/systemd/system/postgresql.service.d" PG_UPGRADE_LOCK_DROPIN="$PG_UPGRADE_LOCK_DROPIN_DIR/pg-upgrade-lock.conf" PG_UPGRADE_PID_LOCK_MARKER="/run/pg-upgrade-source.pid" +PG_UPGRADE_INITIATE_PID_FILE="/run/pg-upgrade-initiate.pid" +PG_UPGRADE_START_GUARD="/run/pg-upgrade-start-guard.sh" PG_UPGRADE_LOCK_WATCHER_PID="" PG_UPGRADE_START_BLOCKED="" block_postgres_start() { mkdir -p "$PG_UPGRADE_LOCK_DROPIN_DIR" - # The empty ExecStartPre= resets the unit's own list: otherwise a blocked start - # would still run postgres_prestart.sh (as root) before /bin/false fails it. - printf '[Service]\nExecStartPre=\nExecStartPre=/bin/false\nRestart=no\n' >"$PG_UPGRADE_LOCK_DROPIN" + # initiate_upgrade runs in a background subshell: $BASHPID is its own pid, $$ is + # the already exited parent. + printf '%s\n' "${BASHPID:-$$}" >"$PG_UPGRADE_INITIATE_PID_FILE" + # Refuse a start only while the pid in one of the files is alive, so a SIGKILLed + # script stops blocking once pg_upgrade is gone too. Run through /bin/sh by the + # drop-in because /run is mounted noexec. + cat >"$PG_UPGRADE_START_GUARD" <<'GUARD' +for pid_file in "$@"; do + pid=$(cat "$pid_file" 2>/dev/null) || continue + if [ -n "$pid" ] && kill -0 "$pid" 2>/dev/null; then + exit 255 + fi +done +exit 0 +GUARD + # ExecCondition runs before the unit's own ExecStartPre (postgres_prestart.sh, + # as root), which therefore does not run for a refused start. Exit 255 fails the + # start, 1-254 would only skip it silently. The unit does not run as root, so + # `+` is needed to see root-owned pids and to remove files under /run. A start + # that gets past the guard means this script and pg_upgrade are gone: the + # ExecStartPost lines then remove the drop-in so Restart=no does not outlive it. + printf '[Service]\nExecCondition=+/bin/sh %s %s %s\n' \ + "$PG_UPGRADE_START_GUARD" "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_PID_LOCK_MARKER" >"$PG_UPGRADE_LOCK_DROPIN" + printf 'ExecStartPost=-+/bin/rm -f %s %s %s %s\nExecStartPost=-+/bin/systemctl daemon-reload\nRestart=no\n' \ + "$PG_UPGRADE_LOCK_DROPIN" "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_PID_LOCK_MARKER" "$PG_UPGRADE_START_GUARD" >>"$PG_UPGRADE_LOCK_DROPIN" PG_UPGRADE_START_BLOCKED=1 systemctl daemon-reload } @@ -164,6 +190,7 @@ unblock_postgres_start() { if [ -e "$PG_UPGRADE_LOCK_DROPIN" ]; then PG_UPGRADE_START_BLOCKED=1 rm -f "$PG_UPGRADE_LOCK_DROPIN" || rc=1 + rm -f "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_START_GUARD" || rc=1 rmdir --ignore-fail-on-non-empty "$PG_UPGRADE_LOCK_DROPIN_DIR" || true fi systemctl daemon-reload || rc=1 diff --git a/tests/pg_upgrade/test-start-guard.sh b/tests/pg_upgrade/test-start-guard.sh index e9ba6aba1d..31828929b0 100644 --- a/tests/pg_upgrade/test-start-guard.sh +++ b/tests/pg_upgrade/test-start-guard.sh @@ -81,6 +81,8 @@ setup() { PG_UPGRADE_LOCK_DROPIN_DIR="$test_dir/postgresql.service.d" PG_UPGRADE_LOCK_DROPIN="$PG_UPGRADE_LOCK_DROPIN_DIR/pg-upgrade-lock.conf" PG_UPGRADE_PID_LOCK_MARKER="$test_dir/pg-upgrade-source.pid" + PG_UPGRADE_INITIATE_PID_FILE="$test_dir/pg-upgrade-initiate.pid" + PG_UPGRADE_START_GUARD="$test_dir/pg-upgrade-start-guard.sh" PG_UPGRADE_LOCK_WATCHER_PID="" PG_UPGRADE_START_BLOCKED="" CLEANUP_STARTED="" @@ -91,6 +93,7 @@ setup() { export IS_LOCAL_UPGRADE PGVERSION PGDATAOLD PGDATANEW MOUNT_POINT LOG_FILE export POSTGRES_SHARE_DIR POST_UPGRADE_EXTENSION_SCRIPT POST_UPGRADE_POSTGRES_PERMS_SCRIPT export PG_UPGRADE_LOCK_DROPIN_DIR PG_UPGRADE_LOCK_DROPIN PG_UPGRADE_PID_LOCK_MARKER + export PG_UPGRADE_INITIATE_PID_FILE PG_UPGRADE_START_GUARD export PG_UPGRADE_LOCK_WATCHER_PID PG_UPGRADE_START_BLOCKED CLEANUP_STARTED export SYSTEMCTL_LOG="$test_dir/systemctl.log" export FAIL_RELOAD_ONCE="$test_dir/fail-reload" @@ -110,11 +113,16 @@ assert() { test_dropin() { setup block_postgres_start - assert grep -qx 'ExecStartPre=' "$PG_UPGRADE_LOCK_DROPIN" - assert grep -qx 'ExecStartPre=/bin/false' "$PG_UPGRADE_LOCK_DROPIN" + assert grep -qx "ExecCondition=+/bin/sh $PG_UPGRADE_START_GUARD $PG_UPGRADE_INITIATE_PID_FILE $PG_UPGRADE_PID_LOCK_MARKER" "$PG_UPGRADE_LOCK_DROPIN" + assert grep -qx "ExecStartPost=-+/bin/rm -f $PG_UPGRADE_LOCK_DROPIN $PG_UPGRADE_INITIATE_PID_FILE $PG_UPGRADE_PID_LOCK_MARKER $PG_UPGRADE_START_GUARD" "$PG_UPGRADE_LOCK_DROPIN" + assert grep -qx 'ExecStartPost=-+/bin/systemctl daemon-reload' "$PG_UPGRADE_LOCK_DROPIN" assert grep -qx 'Restart=no' "$PG_UPGRADE_LOCK_DROPIN" + assert test -s "$PG_UPGRADE_INITIATE_PID_FILE" + assert test -s "$PG_UPGRADE_START_GUARD" unblock_postgres_start assert test ! -e "$PG_UPGRADE_LOCK_DROPIN" + assert test ! -e "$PG_UPGRADE_INITIATE_PID_FILE" + assert test ! -e "$PG_UPGRADE_START_GUARD" assert test "$(grep -c '^daemon-reload$' "$SYSTEMCTL_LOG")" -eq 2 assert grep -qx 'reset-failed postgresql' "$SYSTEMCTL_LOG" unblock_postgres_start @@ -207,6 +215,52 @@ test_active_source() { done } +test_start_guard() { + local live dead + setup + block_postgres_start + sleep 60 & + live=$! + (exit 0) & + dead=$! + wait "$dead" + guard_exit() { + local exit_code=0 + sh -c "$(sed -n 's/^ExecCondition=+//p' "$PG_UPGRADE_LOCK_DROPIN")" || exit_code=$? + printf '%s\n' "$exit_code" + } + assert test "$(guard_exit)" -eq 255 + printf '%s\n' "$dead" >"$PG_UPGRADE_INITIATE_PID_FILE" + assert test "$(guard_exit)" -eq 0 + printf '%s\n' "$live" >"$PG_UPGRADE_PID_LOCK_MARKER" + assert test "$(guard_exit)" -eq 255 + printf '%s\n' "$live" >"$PG_UPGRADE_INITIATE_PID_FILE" + printf '%s\n' "$dead" >"$PG_UPGRADE_PID_LOCK_MARKER" + assert test "$(guard_exit)" -eq 255 + printf '%s\n' "$dead" >"$PG_UPGRADE_INITIATE_PID_FILE" + printf '%s\n' "$dead" >"$PG_UPGRADE_PID_LOCK_MARKER" + assert test "$(guard_exit)" -eq 0 + printf 'not-a-pid\n' >"$PG_UPGRADE_INITIATE_PID_FILE" + : >"$PG_UPGRADE_PID_LOCK_MARKER" + assert test "$(guard_exit)" -eq 0 + rm -f "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_PID_LOCK_MARKER" + assert test "$(guard_exit)" -eq 0 + kill "$live" + wait "$live" 2>/dev/null || true +} + +test_dropin_expiry() { + setup + block_postgres_start + printf '%s\n' 12345 >"$PG_UPGRADE_PID_LOCK_MARKER" + sh -c "$(sed -n 's/^ExecStartPost=-+\(\/bin\/rm .*\)$/\1/p' "$PG_UPGRADE_LOCK_DROPIN")" + assert test ! -e "$PG_UPGRADE_LOCK_DROPIN" + assert test ! -e "$PG_UPGRADE_INITIATE_PID_FILE" + assert test ! -e "$PG_UPGRADE_PID_LOCK_MARKER" + assert test ! -e "$PG_UPGRADE_START_GUARD" + unblock_postgres_start +} + test_exit_cleanup() { local trigger exit_code expected_code for trigger in failure TERM INT clean-exit cleanup-exit already-cleaned restore-rm restore-mv; do @@ -270,7 +324,7 @@ test_exit_cleanup() { } export -f log retry block_postgres_start unblock_postgres_start cleanup on_exit -for test_name in test_dropin test_no_block_reset test_block_reload_failure test_unblock_reload_retry test_owned_pid test_foreign_pid test_watcher_existing_pid test_active_source test_exit_cleanup; do +for test_name in test_dropin test_no_block_reset test_block_reload_failure test_unblock_reload_retry test_owned_pid test_foreign_pid test_watcher_existing_pid test_active_source test_start_guard test_dropin_expiry test_exit_cleanup; do case_output="$TEST_ROOT/$test_name.log" trap 'exit_code=$?; cat "$case_output"; exit "$exit_code"' ERR ( From a0257c1da17633e54a7277db816fb391aacedd1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Boros?= Date: Fri, 9 Oct 2026 19:21:06 +0300 Subject: [PATCH 05/14] test(nix): run pg_upgrade start guard test in flake check Refs: MPG-1334 Co-Authored-By: Claude Sonnet 5.5 --- nix/checks.nix | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/nix/checks.nix b/nix/checks.nix index 1a4b21a212..2b5e2b848f 100644 --- a/nix/checks.nix +++ b/nix/checks.nix @@ -608,6 +608,20 @@ psql_17_cli = pkgs.runCommand "run-check-harness-psql-17-cli" { } ( lib.getExe (makeCheckHarness self'.packages."psql_17_cli/bin" { isCliVariant = true; }) ); + pg_upgrade_start_guard = + let + src = lib.fileset.toSource { + root = ../.; + fileset = lib.fileset.unions [ + ../tests/pg_upgrade/test-start-guard.sh + ../ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh + ]; + }; + in + pkgs.runCommand "pg-upgrade-start-guard-test" { } '' + bash ${src}/tests/pg_upgrade/test-start-guard.sh + touch $out + ''; # Portable CLI bundle portability checks psql_17_cli_portable = pkgs.runCommand "psql_17_cli_portable-portability-check" From 0d96aa5125eecb8e44a4a24970afed06d9918a19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Boros?= Date: Fri, 9 Oct 2026 23:54:54 +0300 Subject: [PATCH 06/14] test(nix): add pg_upgrade start guard VM test Refs: MPG-1334 Co-Authored-By: Claude Sonnet 5.5 --- nix/ext/tests/pg-upgrade-start-guard.nix | 227 +++++++++++++++++++++++ 1 file changed, 227 insertions(+) create mode 100644 nix/ext/tests/pg-upgrade-start-guard.nix diff --git a/nix/ext/tests/pg-upgrade-start-guard.nix b/nix/ext/tests/pg-upgrade-start-guard.nix new file mode 100644 index 0000000000..c4de5fc6d6 --- /dev/null +++ b/nix/ext/tests/pg-upgrade-start-guard.nix @@ -0,0 +1,227 @@ +{ self, pkgs }: +let + inherit (pkgs) lib; + lpkgs = pkgs.pkgsLinux; + system = lpkgs.stdenv.hostPlatform.system; + + pg15 = self.packages.${system}."psql_15/bin"; + pg17 = self.packages.${system}."psql_17/bin"; + + dataOld = "/var/lib/postgresql/data"; + dataNew = "/var/lib/postgresql/data-17"; + dropIn = "/run/systemd/system/postgresql.service.d/pg-upgrade-lock.conf"; + + initiate = builtins.path { + path = ../../../ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh; + name = "initiate.sh"; + }; + + # initiate.sh runs the upgrade when sourced, so extract only the guard. + startGuard = lpkgs.runCommand "start-guard.sh" { } '' + awk ' + /^PG_UPGRADE_(LOCK|PID|INITIATE|START)_[A-Z_]+=/ { print } + /^(block_postgres_start|start_pid_lock_watcher|unblock_postgres_start|assert_source_stayed_down)\(\) \{/ { printing = 1 } + printing { print } + printing && /^}/ { printing = 0 } + ' ${initiate} >$out + ''; + + guardEnv = lpkgs.writeText "guard-env.sh" '' + export PATH=${ + lib.makeBinPath ( + with lpkgs; + [ + bash + coreutils + diffutils + gnugrep + gnused + procps + systemd + util-linux + ] + ) + } + IS_CI="" + PGDATAOLD=${dataOld} + PGDATANEW=${dataNew} + log() { echo "[$(date +%T)] $*"; } + retry() { + local n=$1 i + shift + for i in $(seq 1 "$n"); do + "$@" && return 0 + sleep 1 + done + return 1 + } + . ${startGuard} + ''; + + holdBlock = lpkgs.writeShellScript "hold-block" '' + set -eEuo pipefail + . ${guardEnv} + systemctl stop postgresql + block_postgres_start + exec sleep infinity + ''; + + saltSim = lpkgs.writeShellScript "salt-sim" '' + export PATH=${ + lib.makeBinPath [ + lpkgs.coreutils + lpkgs.systemd + ] + } + : >/tmp/salt-states + while :; do + systemctl start postgresql 2>/dev/null || true + systemctl is-active postgresql >>/tmp/salt-states || true + sleep 0.1 + done + ''; + + holdLock = lpkgs.writeShellScript "hold-lock" '' + set -eEuo pipefail + . ${guardEnv} + PGDATANEW=/tmp/fake-new + mkdir -p "$PGDATANEW" + runuser -u postgres -- bash -c 'sleep infinity; true' fake-pg-upgrade "--old-datadir=$PGDATAOLD" & + start_pid_lock_watcher + touch "$PGDATANEW/postmaster.pid" + wait + ''; + + guardedUpgrade = lpkgs.writeShellScript "guarded-upgrade" '' + set -eEuo pipefail + . ${guardEnv} + systemctl stop postgresql + block_postgres_start + runuser -u postgres -- ${pg17}/bin/initdb --no-locale -E UTF8 -U postgres -D "$PGDATANEW" + start_pid_lock_watcher + systemd-run --unit=salt-sim --collect ${saltSim} + cd /var/lib/postgresql + runuser -u postgres -- ${pg17}/bin/pg_upgrade \ + --username=postgres \ + --old-datadir="$PGDATAOLD" --new-datadir="$PGDATANEW" \ + --old-bindir=${pg15}/bin --new-bindir=${pg17}/bin + systemctl stop salt-sim + assert_source_stayed_down + unblock_postgres_start + ''; + + initOld = lpkgs.writeShellScript "init-old" '' + set -euo pipefail + if [ ! -f ${dataOld}/PG_VERSION ]; then + ${lpkgs.coreutils}/bin/install -d -o postgres -g postgres -m 0700 ${dataOld} + ${lpkgs.util-linux}/bin/runuser -u postgres -- \ + ${pg15}/bin/initdb --no-locale -E UTF8 -U postgres -D ${dataOld} + fi + ''; +in +pkgs.testers.runNixOSTest { + name = "pg-upgrade-start-guard"; + nodes.server = + { ... }: + { + virtualisation.memorySize = 2048; + virtualisation.diskSize = 4096; + + users.users.postgres = { + isSystemUser = true; + group = "postgres"; + home = "/var/lib/postgresql"; + createHome = true; + }; + users.groups.postgres = { }; + systemd.tmpfiles.rules = [ "d /run/postgresql 0755 postgres postgres -" ]; + + systemd.services.postgresql = { + wantedBy = [ "multi-user.target" ]; + serviceConfig = { + Type = "notify"; + User = "postgres"; + Group = "postgres"; + ExecStartPre = "+${initOld}"; + ExecStart = "${pg15}/bin/postgres -D ${dataOld}"; + KillMode = "mixed"; + KillSignal = "SIGINT"; + Restart = "always"; + RestartSec = 1; + }; + }; + }; + + testScript = '' + PG15 = "${pg15}/bin" + PG17 = "${pg17}/bin" + DROPIN = "${dropIn}" + + def sql(query, port=5432, bindir=PG15): + return server.succeed( + f"{bindir}/psql -h 127.0.0.1 -p {port} -U postgres -d postgres -Atc \"{query}\"" + ).strip() + + start_all() + server.wait_for_unit("postgresql.service") + + with subtest("Seed the source cluster"): + sql("create table seed as select g as id, repeat(md5(g::text), 20) as pad from generate_series(1, 400000) g") + expected = sql("select count(*) || ',' || sum(id) from seed") + + with subtest("Control: with no guard the start loop brings the source up"): + server.succeed("systemctl stop postgresql") + server.succeed("systemd-run --unit=salt-sim --collect ${saltSim}") + server.wait_until_succeeds("systemctl is-active --quiet postgresql", timeout=30) + server.succeed("systemctl stop salt-sim") + + with subtest("A systemd start is refused while the block is held"): + server.succeed("systemd-run --unit=hold-block --collect ${holdBlock}") + server.wait_until_succeeds("systemctl show -p ExecCondition postgresql | grep -q start-guard", timeout=120) + server.fail("systemctl start postgresql") + server.sleep(5) + server.fail("systemctl is-active --quiet postgresql") + assert "Restart=no" in server.succeed("systemctl show -p Restart postgresql") + + with subtest("The block expires on its own when the holder is SIGKILLed"): + server.succeed("systemctl kill -s KILL hold-block") + server.wait_until_fails("systemctl is-active --quiet hold-block") + server.succeed("systemctl start postgresql") + server.wait_for_unit("postgresql.service") + server.fail(f"test -e {DROPIN}") + assert "Restart=always" in server.succeed("systemctl show -p Restart postgresql") + assert "NeedDaemonReload=no" in server.succeed("systemctl show -p NeedDaemonReload postgresql") + + with subtest("The pid lock stops a direct start once the new cluster is up"): + server.succeed("systemctl stop postgresql") + server.succeed("systemd-run --unit=hold-lock --collect ${holdLock}") + server.wait_until_succeeds("test -s ${dataOld}/postmaster.pid", timeout=120) + server.fail(f"runuser -u postgres -- {PG15}/pg_ctl start -D ${dataOld} -w -t 5 -s -l /tmp/pg15-locked.log") + server.succeed("systemctl kill -s KILL hold-lock") + server.wait_until_fails("systemctl is-active --quiet hold-lock") + server.succeed(f"runuser -u postgres -- {PG15}/pg_ctl start -D ${dataOld} -w -s -l /tmp/pg15.log") + server.succeed(f"runuser -u postgres -- {PG15}/pg_ctl stop -D ${dataOld} -w -s") + server.succeed("rm -f /run/pg-upgrade-source.pid") + server.succeed("systemctl start postgresql") + server.wait_for_unit("postgresql.service") + + with subtest("A real pg_upgrade copy under a start storm"): + server.succeed("systemd-run --unit=fake-initiate --collect --pipe --wait ${guardedUpgrade}") + attempts = int(server.succeed("wc -l = 5, f"start loop barely ran: {attempts} attempts" + server.fail("grep -qx active /tmp/salt-states") + + with subtest("The new cluster matches the seed"): + server.succeed( + f"runuser -u postgres -- {PG17}/pg_ctl start -D ${dataNew} -w -s -l /tmp/pg17.log -o '-p 5433'" + ) + assert sql("select count(*) || ',' || sum(id) from seed", port=5433, bindir=PG17) == expected + server.succeed(f"runuser -u postgres -- {PG17}/pg_ctl stop -D ${dataNew} -w -s") + + with subtest("The source is untouched and starts again"): + server.succeed("systemctl start postgresql") + server.wait_for_unit("postgresql.service") + assert sql("select count(*) || ',' || sum(id) from seed") == expected + server.fail(f"test -e {DROPIN}") + ''; +} From 885aa929a43513cad44bf1ebfcc7c1a511ef3301 Mon Sep 17 00:00:00 2001 From: "andres.suarez" Date: Tue, 13 Oct 2026 11:52:43 +0200 Subject: [PATCH 07/14] refactor: split pg_upgrade start guard into start_guard.sh and address review - Move the start guard functions out of initiate.sh into start_guard.sh, sourced by initiate.sh and test-start-guard.sh. - Resolve sh, rm and systemctl with type -P and fail loudly if missing, instead of hardcoding /bin. - Remove the guard script only after daemon-reload succeeds, so a failed reload cannot leave a drop-in pointing at a missing guard. - Make the pid lock watcher exit when initiate.sh and pg_upgrade are gone. - Test the generated ExecStartPost line instead of hardcoded paths, add a watcher-abandoned test, and include start_guard.sh in the nix check. Co-Authored-By: Claude Sonnet 5.5 --- .../pg_upgrade_scripts/initiate.sh | 134 +------------- .../pg_upgrade_scripts/start_guard.sh | 166 ++++++++++++++++++ nix/checks.nix | 1 + nix/docs/testing-pg-upgrade-scripts.md | 2 +- tests/pg_upgrade/test-start-guard.sh | 67 +++++-- 5 files changed, 218 insertions(+), 152 deletions(-) create mode 100755 ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh diff --git a/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh b/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh index 2c573d0bfe..94cb0e2ae5 100755 --- a/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh +++ b/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh @@ -86,138 +86,8 @@ fi OLD_BOOTSTRAP_USER=$(run_sql -A -t -c "select rolname from pg_authid where oid = 10;") -# From step 10 until cleanup, nothing may start the source postgres: pg_upgrade -# copies the data files without a postmaster holding the directory, and a start -# in that window (Salt's `service.running`, adminapi, Restart=always, ...) -# yields a copy that is inconsistent with the dump. Two independent guards: -# 1. a runtime systemd drop-in that makes `systemctl start` fail while this -# script or pg_upgrade is alive (a SIGKILLed script must not leave the -# source blocked until a reboot); -# 2. a postmaster.pid in the old data dir holding pg_upgrade's own PID, which -# makes any postmaster (systemd or direct pg_ctl) refuse to start while -# pg_upgrade lives. The file goes stale when pg_upgrade exits. -PG_UPGRADE_LOCK_DROPIN_DIR="/run/systemd/system/postgresql.service.d" -PG_UPGRADE_LOCK_DROPIN="$PG_UPGRADE_LOCK_DROPIN_DIR/pg-upgrade-lock.conf" -PG_UPGRADE_PID_LOCK_MARKER="/run/pg-upgrade-source.pid" -PG_UPGRADE_INITIATE_PID_FILE="/run/pg-upgrade-initiate.pid" -PG_UPGRADE_START_GUARD="/run/pg-upgrade-start-guard.sh" -PG_UPGRADE_LOCK_WATCHER_PID="" -PG_UPGRADE_START_BLOCKED="" - -block_postgres_start() { - mkdir -p "$PG_UPGRADE_LOCK_DROPIN_DIR" - # initiate_upgrade runs in a background subshell: $BASHPID is its own pid, $$ is - # the already exited parent. - printf '%s\n' "${BASHPID:-$$}" >"$PG_UPGRADE_INITIATE_PID_FILE" - # Refuse a start only while the pid in one of the files is alive, so a SIGKILLed - # script stops blocking once pg_upgrade is gone too. Run through /bin/sh by the - # drop-in because /run is mounted noexec. - cat >"$PG_UPGRADE_START_GUARD" <<'GUARD' -for pid_file in "$@"; do - pid=$(cat "$pid_file" 2>/dev/null) || continue - if [ -n "$pid" ] && kill -0 "$pid" 2>/dev/null; then - exit 255 - fi -done -exit 0 -GUARD - # ExecCondition runs before the unit's own ExecStartPre (postgres_prestart.sh, - # as root), which therefore does not run for a refused start. Exit 255 fails the - # start, 1-254 would only skip it silently. The unit does not run as root, so - # `+` is needed to see root-owned pids and to remove files under /run. A start - # that gets past the guard means this script and pg_upgrade are gone: the - # ExecStartPost lines then remove the drop-in so Restart=no does not outlive it. - printf '[Service]\nExecCondition=+/bin/sh %s %s %s\n' \ - "$PG_UPGRADE_START_GUARD" "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_PID_LOCK_MARKER" >"$PG_UPGRADE_LOCK_DROPIN" - printf 'ExecStartPost=-+/bin/rm -f %s %s %s %s\nExecStartPost=-+/bin/systemctl daemon-reload\nRestart=no\n' \ - "$PG_UPGRADE_LOCK_DROPIN" "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_PID_LOCK_MARKER" "$PG_UPGRADE_START_GUARD" >>"$PG_UPGRADE_LOCK_DROPIN" - PG_UPGRADE_START_BLOCKED=1 - systemctl daemon-reload -} - -# Wait for the new cluster's temp server (the old one is down from then on, -# its schema dump is done) and then claim the old data dir's postmaster.pid. -# It must not exist before: pg_upgrade aborts if it finds one at startup. -start_pid_lock_watcher() { - ( - # set -E would otherwise run cleanup() from this subshell - trap - ERR EXIT - set +e - until [ -e "$PGDATANEW/postmaster.pid" ]; do sleep 0.2; done - upg_pid=$(pgrep -u postgres -f -- "--old-datadir=${PGDATAOLD}" | head -1) - if [ -z "$upg_pid" ]; then - exit 0 - fi - if [ -e "$PGDATAOLD/postmaster.pid" ]; then - log "WARNING: $PGDATAOLD/postmaster.pid already exists; not creating the pg_upgrade lock" - exit 0 - fi - printf '%s\n' "$upg_pid" >"$PG_UPGRADE_PID_LOCK_MARKER" || exit 1 - if ! ( - set -o noclobber - cat "$PG_UPGRADE_PID_LOCK_MARKER" >"$PGDATAOLD/postmaster.pid" - ); then - log "WARNING: could not create the pg_upgrade PID lock" - exit 1 - fi - chown postgres:postgres "$PGDATAOLD/postmaster.pid" - chmod 0600 "$PGDATAOLD/postmaster.pid" - log "Locked $PGDATAOLD with pg_upgrade pid $upg_pid" - ) & - PG_UPGRADE_LOCK_WATCHER_PID=$! -} - -# Idempotent: cleanup() can run before step 10 or more than once. -unblock_postgres_start() { - if [ -n "$PG_UPGRADE_LOCK_WATCHER_PID" ]; then - kill "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null || true - wait "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null || true - PG_UPGRADE_LOCK_WATCHER_PID="" - fi - local pidfile="${PGDATAOLD:-/nonexistent}/postmaster.pid" - # Callers run this under `||` where set -e is off, so report failures through - # the return status: a failed daemon-reload leaves the block loaded in systemd. - local rc=0 - if [ -f "$PG_UPGRADE_PID_LOCK_MARKER" ]; then - if [ -f "$pidfile" ] && cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$pidfile"; then - rm -f "$pidfile" || rc=1 - fi - if [ "$rc" -eq 0 ]; then - rm -f "$PG_UPGRADE_PID_LOCK_MARKER" || rc=1 - fi - fi - if [ -z "$IS_CI" ]; then - if [ -e "$PG_UPGRADE_LOCK_DROPIN" ]; then - PG_UPGRADE_START_BLOCKED=1 - rm -f "$PG_UPGRADE_LOCK_DROPIN" || rc=1 - rm -f "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_START_GUARD" || rc=1 - rmdir --ignore-fail-on-non-empty "$PG_UPGRADE_LOCK_DROPIN_DIR" || true - fi - systemctl daemon-reload || rc=1 - if [ "$rc" -eq 0 ] && [ -n "$PG_UPGRADE_START_BLOCKED" ]; then - systemctl reset-failed postgresql || true - PG_UPGRADE_START_BLOCKED="" - fi - fi - return $rc -} - -# The source must have stayed down for the whole copy; if not, the copy is -# inconsistent and the upgrade has to fail (the source data is untouched). -assert_source_stayed_down() { - local state="" - if [ -z "$IS_CI" ]; then - state=$(systemctl is-active postgresql || true) - fi - if [ "$state" = "active" ] || [ "$state" = "activating" ] || [ "$state" = "reloading" ]; then - log "ERROR: postgresql.service is ${state} after pg_upgrade; the copied data files may be inconsistent" - return 1 - fi - if [ -f "$PGDATAOLD/postmaster.pid" ] && ! cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$PGDATAOLD/postmaster.pid"; then - log "ERROR: a postmaster ran in $PGDATAOLD during pg_upgrade; the copied data files may be inconsistent" - return 1 - fi -} +# shellcheck disable=SC1091 +source "$SCRIPT_DIR/start_guard.sh" CLEANUP_STARTED="" diff --git a/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh b/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh new file mode 100755 index 0000000000..748a72d0ad --- /dev/null +++ b/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh @@ -0,0 +1,166 @@ +#! /usr/bin/env bash + +# Start guard for initiate.sh: keeps the source Postgres down during the pg_upgrade +# file copy. Sourced by initiate.sh and tests/pg_upgrade/test-start-guard.sh. +# Expects log, IS_CI, PGDATAOLD and PGDATANEW from the caller. + +# From step 10 until cleanup, nothing may start the source postgres: pg_upgrade +# copies the data files without a postmaster holding the directory, and a start +# in that window (Salt's `service.running`, adminapi, Restart=always, ...) +# yields a copy that is inconsistent with the dump. Two independent guards: +# 1. a runtime systemd drop-in that makes `systemctl start` fail while this +# script or pg_upgrade is alive (a SIGKILLed script must not leave the +# source blocked until a reboot); +# 2. a postmaster.pid in the old data dir holding pg_upgrade's own PID, which +# makes any postmaster (systemd or direct pg_ctl) refuse to start while +# pg_upgrade lives. The file goes stale when pg_upgrade exits. +PG_UPGRADE_LOCK_DROPIN_DIR="/run/systemd/system/postgresql.service.d" +PG_UPGRADE_LOCK_DROPIN="$PG_UPGRADE_LOCK_DROPIN_DIR/pg-upgrade-lock.conf" +PG_UPGRADE_PID_LOCK_MARKER="/run/pg-upgrade-source.pid" +PG_UPGRADE_INITIATE_PID_FILE="/run/pg-upgrade-initiate.pid" +PG_UPGRADE_START_GUARD="/run/pg-upgrade-start-guard.sh" +PG_UPGRADE_LOCK_WATCHER_PID="" +PG_UPGRADE_START_BLOCKED="" + +block_postgres_start() { + # systemd needs absolute paths, and a drop-in naming a missing binary would + # silently never expire: fail here instead. + local sh_path rm_path systemctl_path + sh_path=$(type -P sh) || { + log "ERROR: sh not found in PATH" + return 1 + } + rm_path=$(type -P rm) || { + log "ERROR: rm not found in PATH" + return 1 + } + systemctl_path=$(type -P systemctl) || { + log "ERROR: systemctl not found in PATH" + return 1 + } + mkdir -p "$PG_UPGRADE_LOCK_DROPIN_DIR" + # initiate_upgrade runs in a background subshell: $BASHPID is its own pid, $$ is + # the already exited parent. + printf '%s\n' "${BASHPID:-$$}" >"$PG_UPGRADE_INITIATE_PID_FILE" + # Refuse a start only while the pid in one of the files is alive, so a SIGKILLed + # script stops blocking once pg_upgrade is gone too. Run through sh by the + # drop-in because /run is mounted noexec. + cat >"$PG_UPGRADE_START_GUARD" <<'GUARD' +for pid_file in "$@"; do + pid=$(cat "$pid_file" 2>/dev/null) || continue + if [ -n "$pid" ] && kill -0 "$pid" 2>/dev/null; then + exit 255 + fi +done +exit 0 +GUARD + # ExecCondition runs before the unit's own ExecStartPre (postgres_prestart.sh, + # as root), which therefore does not run for a refused start. Exit 255 fails the + # start, 1-254 would only skip it silently. The unit does not run as root, so + # `+` is needed to see root-owned pids and to remove files under /run. A start + # that gets past the guard means this script and pg_upgrade are gone: the + # ExecStartPost lines then remove the drop-in so Restart=no does not outlive it. + printf '[Service]\nExecCondition=+%s %s %s %s\n' \ + "$sh_path" "$PG_UPGRADE_START_GUARD" "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_PID_LOCK_MARKER" >"$PG_UPGRADE_LOCK_DROPIN" + printf 'ExecStartPost=-+%s -f %s %s %s %s\nExecStartPost=-+%s daemon-reload\nRestart=no\n' \ + "$rm_path" "$PG_UPGRADE_LOCK_DROPIN" "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_PID_LOCK_MARKER" "$PG_UPGRADE_START_GUARD" \ + "$systemctl_path" >>"$PG_UPGRADE_LOCK_DROPIN" + PG_UPGRADE_START_BLOCKED=1 + systemctl daemon-reload +} + +# Wait for the new cluster's temp server (the old one is down from then on, +# its schema dump is done) and then claim the old data dir's postmaster.pid. +# It must not exist before: pg_upgrade aborts if it finds one at startup. +start_pid_lock_watcher() { + local initiate_pid=${BASHPID:-$$} + ( + # set -E would otherwise run cleanup() from this subshell + trap - ERR EXIT + set +e + until [ -e "$PGDATANEW/postmaster.pid" ]; do + # initiate.sh SIGKILLed and pg_upgrade gone: nothing left to lock + if ! kill -0 "$initiate_pid" 2>/dev/null && ! pgrep -u postgres -f -- "--old-datadir=${PGDATAOLD}" >/dev/null; then + exit 0 + fi + sleep 0.2 + done + upg_pid=$(pgrep -u postgres -f -- "--old-datadir=${PGDATAOLD}" | head -1) + if [ -z "$upg_pid" ]; then + exit 0 + fi + if [ -e "$PGDATAOLD/postmaster.pid" ]; then + log "WARNING: $PGDATAOLD/postmaster.pid already exists; not creating the pg_upgrade lock" + exit 0 + fi + printf '%s\n' "$upg_pid" >"$PG_UPGRADE_PID_LOCK_MARKER" || exit 1 + if ! ( + set -o noclobber + cat "$PG_UPGRADE_PID_LOCK_MARKER" >"$PGDATAOLD/postmaster.pid" + ); then + log "WARNING: could not create the pg_upgrade PID lock" + exit 1 + fi + chown postgres:postgres "$PGDATAOLD/postmaster.pid" + chmod 0600 "$PGDATAOLD/postmaster.pid" + log "Locked $PGDATAOLD with pg_upgrade pid $upg_pid" + ) & + PG_UPGRADE_LOCK_WATCHER_PID=$! +} + +# Idempotent: cleanup() can run before step 10 or more than once. +unblock_postgres_start() { + if [ -n "$PG_UPGRADE_LOCK_WATCHER_PID" ]; then + kill "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null || true + wait "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null || true + PG_UPGRADE_LOCK_WATCHER_PID="" + fi + local pidfile="${PGDATAOLD:-/nonexistent}/postmaster.pid" + # Callers run this under `||` where set -e is off, so report failures through + # the return status: a failed daemon-reload leaves the block loaded in systemd. + local rc=0 + if [ -f "$PG_UPGRADE_PID_LOCK_MARKER" ]; then + if [ -f "$pidfile" ] && cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$pidfile"; then + rm -f "$pidfile" || rc=1 + fi + if [ "$rc" -eq 0 ]; then + rm -f "$PG_UPGRADE_PID_LOCK_MARKER" || rc=1 + fi + fi + if [ -z "$IS_CI" ]; then + if [ -e "$PG_UPGRADE_LOCK_DROPIN" ]; then + PG_UPGRADE_START_BLOCKED=1 + rm -f "$PG_UPGRADE_LOCK_DROPIN" || rc=1 + rm -f "$PG_UPGRADE_INITIATE_PID_FILE" || rc=1 + rmdir --ignore-fail-on-non-empty "$PG_UPGRADE_LOCK_DROPIN_DIR" || true + fi + systemctl daemon-reload || rc=1 + # Only after a successful reload: with the drop-in still loaded, a missing + # guard exits 127, which systemd treats as a silent skip of the start. + if [ "$rc" -eq 0 ]; then + rm -f "$PG_UPGRADE_START_GUARD" || rc=1 + fi + if [ "$rc" -eq 0 ] && [ -n "$PG_UPGRADE_START_BLOCKED" ]; then + systemctl reset-failed postgresql || true + PG_UPGRADE_START_BLOCKED="" + fi + fi + return $rc +} + +# The source must have stayed down for the whole copy; if not, the copy is +# inconsistent and the upgrade has to fail (the source data is untouched). +assert_source_stayed_down() { + local state="" + if [ -z "$IS_CI" ]; then + state=$(systemctl is-active postgresql || true) + fi + if [ "$state" = "active" ] || [ "$state" = "activating" ] || [ "$state" = "reloading" ]; then + log "ERROR: postgresql.service is ${state} after pg_upgrade; the copied data files may be inconsistent" + return 1 + fi + if [ -f "$PGDATAOLD/postmaster.pid" ] && ! cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$PGDATAOLD/postmaster.pid"; then + log "ERROR: a postmaster ran in $PGDATAOLD during pg_upgrade; the copied data files may be inconsistent" + return 1 + fi +} diff --git a/nix/checks.nix b/nix/checks.nix index 2b5e2b848f..906eb5a3ad 100644 --- a/nix/checks.nix +++ b/nix/checks.nix @@ -615,6 +615,7 @@ fileset = lib.fileset.unions [ ../tests/pg_upgrade/test-start-guard.sh ../ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh + ../ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh ]; }; in diff --git a/nix/docs/testing-pg-upgrade-scripts.md b/nix/docs/testing-pg-upgrade-scripts.md index d72064ba81..2b6afafd83 100644 --- a/nix/docs/testing-pg-upgrade-scripts.md +++ b/nix/docs/testing-pg-upgrade-scripts.md @@ -13,7 +13,7 @@ bash tests/pg_upgrade/test-start-guard.sh The suite uses temporary directories and command shims, not a live PostgreSQL instance or systemd. It covers PID lock ownership, drop-in install/removal, reload retries, share-directory restoration failures, and ERR/EXIT/TERM/INT -cleanup. It is not run in CI. A full non-CI upgrade still requires a Linux +cleanup. It is run in CI as `nix flake check`. A full non-CI upgrade still requires a Linux staging host with real systemd. ## Prerequisites diff --git a/tests/pg_upgrade/test-start-guard.sh b/tests/pg_upgrade/test-start-guard.sh index 31828929b0..f882d006d1 100644 --- a/tests/pg_upgrade/test-start-guard.sh +++ b/tests/pg_upgrade/test-start-guard.sh @@ -6,11 +6,14 @@ REPO_ROOT=$(cd "$(dirname "$0")/../.." && pwd) TEST_ROOT=$(mktemp -d) trap 'rm -rf "$TEST_ROOT"' EXIT +SCRIPTS_DIR="$REPO_ROOT/ansible/files/admin_api_scripts/pg_upgrade_scripts" + +# cleanup and on_exit are part of initiate.sh itself, which cannot be sourced. eval "$(awk ' - /^(block_postgres_start|start_pid_lock_watcher|unblock_postgres_start|assert_source_stayed_down|cleanup|on_exit)\(\) \{/ { printing = 1 } + /^(cleanup|on_exit)\(\) \{/ { printing = 1 } printing { print } printing && /^}/ { printing = 0 } -' "$REPO_ROOT/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh")" +' "$SCRIPTS_DIR/initiate.sh")" mkdir "$TEST_ROOT/bin" cat >"$TEST_ROOT/bin/systemctl" <<'SHIM' @@ -30,34 +33,37 @@ esac SHIM cat >"$TEST_ROOT/bin/pgrep" <<'SHIM' #!/usr/bin/env bash +if [ -n "${PGREP_EMPTY:-}" ]; then + exit 1 +fi printf '12345\n' SHIM cat >"$TEST_ROOT/bin/chown" <<'SHIM' #!/usr/bin/env bash exit 0 SHIM -cat >"$TEST_ROOT/bin/rmdir" <<'SHIM' +cat >"$TEST_ROOT/bin/rmdir" <"$TEST_ROOT/bin/rm" <<'SHIM' +cat >"$TEST_ROOT/bin/rm" <>"$SYSTEMCTL_LOG" +if [ "\${FAIL_SHARE_RESTORE:-}" = rm ] && [ "\$1" = "\$POSTGRES_SHARE_DIR" ]; then + printf 'restore-rm\n' >>"\$SYSTEMCTL_LOG" exit 1 fi -exec /bin/rm "$@" +exec $(type -P rm) "\$@" SHIM -cat >"$TEST_ROOT/bin/mv" <<'SHIM' +cat >"$TEST_ROOT/bin/mv" <>"$SYSTEMCTL_LOG" +if [ "\${FAIL_SHARE_RESTORE:-}" = mv ] && [ "\$1" = "\${POSTGRES_SHARE_DIR}.bak" ]; then + printf 'restore-mv\n' >>"\$SYSTEMCTL_LOG" exit 1 fi -exec /bin/mv "$@" +exec $(type -P mv) "\$@" SHIM chmod +x "$TEST_ROOT/bin/"* export PATH="$TEST_ROOT/bin:$PATH" @@ -68,6 +74,9 @@ retry() { exit 1 } +# shellcheck disable=SC1091 +source "$SCRIPTS_DIR/start_guard.sh" + setup() { local test_dir test_dir=$(mktemp -d "$TEST_ROOT/case.XXXXXX") @@ -113,9 +122,9 @@ assert() { test_dropin() { setup block_postgres_start - assert grep -qx "ExecCondition=+/bin/sh $PG_UPGRADE_START_GUARD $PG_UPGRADE_INITIATE_PID_FILE $PG_UPGRADE_PID_LOCK_MARKER" "$PG_UPGRADE_LOCK_DROPIN" - assert grep -qx "ExecStartPost=-+/bin/rm -f $PG_UPGRADE_LOCK_DROPIN $PG_UPGRADE_INITIATE_PID_FILE $PG_UPGRADE_PID_LOCK_MARKER $PG_UPGRADE_START_GUARD" "$PG_UPGRADE_LOCK_DROPIN" - assert grep -qx 'ExecStartPost=-+/bin/systemctl daemon-reload' "$PG_UPGRADE_LOCK_DROPIN" + assert grep -qx "ExecCondition=+$(type -P sh) $PG_UPGRADE_START_GUARD $PG_UPGRADE_INITIATE_PID_FILE $PG_UPGRADE_PID_LOCK_MARKER" "$PG_UPGRADE_LOCK_DROPIN" + assert grep -qx "ExecStartPost=-+$TEST_ROOT/bin/rm -f $PG_UPGRADE_LOCK_DROPIN $PG_UPGRADE_INITIATE_PID_FILE $PG_UPGRADE_PID_LOCK_MARKER $PG_UPGRADE_START_GUARD" "$PG_UPGRADE_LOCK_DROPIN" + assert grep -qx "ExecStartPost=-+$TEST_ROOT/bin/systemctl daemon-reload" "$PG_UPGRADE_LOCK_DROPIN" assert grep -qx 'Restart=no' "$PG_UPGRADE_LOCK_DROPIN" assert test -s "$PG_UPGRADE_INITIATE_PID_FILE" assert test -s "$PG_UPGRADE_START_GUARD" @@ -204,6 +213,24 @@ test_watcher_existing_pid() { assert test "$(cat "$PGDATAOLD/postmaster.pid")" = 54321 } +test_watcher_abandoned() { + local watcher_pid_file tries=0 + setup + export PGREP_EMPTY=1 + watcher_pid_file="$PGDATAOLD/../watcher.pid" + # a separate shell stands in for a SIGKILLed initiate.sh: it exits, the watcher + # stays (BASHPID is missing on bash 3.2, so a subshell would not work here) + "$BASH" -c 'log() { :; }; source "$1"; start_pid_lock_watcher; printf "%s\n" "$PG_UPGRADE_LOCK_WATCHER_PID" >"$2"' _ \ + "$SCRIPTS_DIR/start_guard.sh" "$watcher_pid_file" + while kill -0 "$(cat "$watcher_pid_file")" 2>/dev/null; do + tries=$((tries + 1)) + assert test "$tries" -lt 50 + sleep 0.2 + done + assert test ! -e "$PG_UPGRADE_PID_LOCK_MARKER" + unset PGREP_EMPTY +} + test_active_source() { local state for state in active activating reloading; do @@ -250,10 +277,12 @@ test_start_guard() { } test_dropin_expiry() { + local exec_start_post setup block_postgres_start printf '%s\n' 12345 >"$PG_UPGRADE_PID_LOCK_MARKER" - sh -c "$(sed -n 's/^ExecStartPost=-+\(\/bin\/rm .*\)$/\1/p' "$PG_UPGRADE_LOCK_DROPIN")" + exec_start_post=$(grep -F "ExecStartPost=-+$TEST_ROOT/bin/rm " "$PG_UPGRADE_LOCK_DROPIN") + sh -c "${exec_start_post#ExecStartPost=-+}" assert test ! -e "$PG_UPGRADE_LOCK_DROPIN" assert test ! -e "$PG_UPGRADE_INITIATE_PID_FILE" assert test ! -e "$PG_UPGRADE_PID_LOCK_MARKER" @@ -323,8 +352,8 @@ test_exit_cleanup() { done } -export -f log retry block_postgres_start unblock_postgres_start cleanup on_exit -for test_name in test_dropin test_no_block_reset test_block_reload_failure test_unblock_reload_retry test_owned_pid test_foreign_pid test_watcher_existing_pid test_active_source test_start_guard test_dropin_expiry test_exit_cleanup; do +export -f log retry block_postgres_start unblock_postgres_start assert_source_stayed_down cleanup on_exit +for test_name in test_dropin test_no_block_reset test_block_reload_failure test_unblock_reload_retry test_owned_pid test_foreign_pid test_watcher_existing_pid test_watcher_abandoned test_active_source test_start_guard test_dropin_expiry test_exit_cleanup; do case_output="$TEST_ROOT/$test_name.log" trap 'exit_code=$?; cat "$case_output"; exit "$exit_code"' ERR ( From 0870272c611fd81151fa85c64349fff612819418 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Boros?= Date: Sat, 10 Oct 2026 00:13:47 +0300 Subject: [PATCH 08/14] test(nix): tidy pg_upgrade start guard VM test Refs: MPG-1334 Co-Authored-By: Claude Sonnet 5.5 --- nix/ext/tests/pg-upgrade-start-guard.nix | 299 +++++++++++------------ 1 file changed, 149 insertions(+), 150 deletions(-) diff --git a/nix/ext/tests/pg-upgrade-start-guard.nix b/nix/ext/tests/pg-upgrade-start-guard.nix index c4de5fc6d6..814a4b181c 100644 --- a/nix/ext/tests/pg-upgrade-start-guard.nix +++ b/nix/ext/tests/pg-upgrade-start-guard.nix @@ -1,15 +1,11 @@ { self, pkgs }: let inherit (pkgs) lib; - lpkgs = pkgs.pkgsLinux; - system = lpkgs.stdenv.hostPlatform.system; - - pg15 = self.packages.${system}."psql_15/bin"; - pg17 = self.packages.${system}."psql_17/bin"; + pg15 = self.packages.${pkgs.pkgsLinux.stdenv.hostPlatform.system}."psql_15/bin"; + pg17 = self.packages.${pkgs.pkgsLinux.stdenv.hostPlatform.system}."psql_17/bin"; dataOld = "/var/lib/postgresql/data"; dataNew = "/var/lib/postgresql/data-17"; - dropIn = "/run/systemd/system/postgresql.service.d/pg-upgrade-lock.conf"; initiate = builtins.path { path = ../../../ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh; @@ -17,7 +13,7 @@ let }; # initiate.sh runs the upgrade when sourced, so extract only the guard. - startGuard = lpkgs.runCommand "start-guard.sh" { } '' + guard = pkgs.pkgsLinux.runCommand "start-guard.sh" { } '' awk ' /^PG_UPGRADE_(LOCK|PID|INITIATE|START)_[A-Z_]+=/ { print } /^(block_postgres_start|start_pid_lock_watcher|unblock_postgres_start|assert_source_stayed_down)\(\) \{/ { printing = 1 } @@ -26,202 +22,205 @@ let ' ${initiate} >$out ''; - guardEnv = lpkgs.writeText "guard-env.sh" '' - export PATH=${ - lib.makeBinPath ( - with lpkgs; - [ - bash - coreutils - diffutils - gnugrep - gnused - procps - systemd - util-linux - ] - ) - } - IS_CI="" - PGDATAOLD=${dataOld} - PGDATANEW=${dataNew} - log() { echo "[$(date +%T)] $*"; } - retry() { - local n=$1 i - shift - for i in $(seq 1 "$n"); do - "$@" && return 0 - sleep 1 - done - return 1 - } - . ${startGuard} - ''; - - holdBlock = lpkgs.writeShellScript "hold-block" '' - set -eEuo pipefail - . ${guardEnv} - systemctl stop postgresql - block_postgres_start - exec sleep infinity - ''; - - saltSim = lpkgs.writeShellScript "salt-sim" '' - export PATH=${ - lib.makeBinPath [ - lpkgs.coreutils - lpkgs.systemd - ] - } - : >/tmp/salt-states - while :; do - systemctl start postgresql 2>/dev/null || true - systemctl is-active postgresql >>/tmp/salt-states || true - sleep 0.1 - done - ''; - - holdLock = lpkgs.writeShellScript "hold-lock" '' - set -eEuo pipefail - . ${guardEnv} - PGDATANEW=/tmp/fake-new - mkdir -p "$PGDATANEW" - runuser -u postgres -- bash -c 'sleep infinity; true' fake-pg-upgrade "--old-datadir=$PGDATAOLD" & - start_pid_lock_watcher - touch "$PGDATANEW/postmaster.pid" - wait - ''; - - guardedUpgrade = lpkgs.writeShellScript "guarded-upgrade" '' - set -eEuo pipefail - . ${guardEnv} - systemctl stop postgresql - block_postgres_start - runuser -u postgres -- ${pg17}/bin/initdb --no-locale -E UTF8 -U postgres -D "$PGDATANEW" - start_pid_lock_watcher - systemd-run --unit=salt-sim --collect ${saltSim} - cd /var/lib/postgresql - runuser -u postgres -- ${pg17}/bin/pg_upgrade \ - --username=postgres \ - --old-datadir="$PGDATAOLD" --new-datadir="$PGDATANEW" \ - --old-bindir=${pg15}/bin --new-bindir=${pg17}/bin - systemctl stop salt-sim - assert_source_stayed_down - unblock_postgres_start - ''; + drive = pkgs.pkgsLinux.writeShellApplication { + name = "drive"; + runtimeInputs = with pkgs.pkgsLinux; [ + bash + coreutils + diffutils + procps + systemd + util-linux + ]; + checkPhase = ""; + text = '' + IS_CI="" + PGDATAOLD=${dataOld} + PGDATANEW=${dataNew} + log() { echo "[$(date +%T)] $*"; } + retry() { + local n=$1 i + shift + for i in $(seq 1 "$n"); do + "$@" && return 0 + sleep 1 + done + return 1 + } + . ${guard} + + hold_block() { + systemctl stop postgresql + block_postgres_start + exec sleep infinity + } + + hold_lock() { + PGDATANEW=/tmp/fake-new + mkdir -p "$PGDATANEW" + runuser -u postgres -- bash -c 'sleep infinity; true' fake-pg-upgrade "--old-datadir=$PGDATAOLD" & + start_pid_lock_watcher + touch "$PGDATANEW/postmaster.pid" + wait + } + + salt_sim() { + : >/tmp/salt-states + while :; do + systemctl start postgresql 2>/dev/null || true + systemctl is-active postgresql >>/tmp/salt-states || true + sleep 0.1 + done + } + + upgrade() { + systemctl stop postgresql + block_postgres_start + runuser -u postgres -- ${pg17}/bin/initdb --no-locale -E UTF8 -U postgres -D "$PGDATANEW" + start_pid_lock_watcher + systemd-run --unit=salt-sim --collect "$0" salt_sim + cd /var/lib/postgresql + runuser -u postgres -- ${pg17}/bin/pg_upgrade \ + --username=postgres \ + --old-datadir="$PGDATAOLD" --new-datadir="$PGDATANEW" \ + --old-bindir=${pg15}/bin --new-bindir=${pg17}/bin + systemctl stop salt-sim + assert_source_stayed_down + unblock_postgres_start + } + + "$@" + ''; + }; - initOld = lpkgs.writeShellScript "init-old" '' - set -euo pipefail - if [ ! -f ${dataOld}/PG_VERSION ]; then - ${lpkgs.coreutils}/bin/install -d -o postgres -g postgres -m 0700 ${dataOld} - ${lpkgs.util-linux}/bin/runuser -u postgres -- \ - ${pg15}/bin/initdb --no-locale -E UTF8 -U postgres -D ${dataOld} - fi - ''; + initOld = pkgs.pkgsLinux.writeShellApplication { + name = "init-old"; + runtimeInputs = with pkgs.pkgsLinux; [ + coreutils + util-linux + ]; + text = '' + if [ ! -f ${dataOld}/PG_VERSION ]; then + install -d -o postgres -g postgres -m 0700 ${dataOld} + runuser -u postgres -- ${pg15}/bin/initdb --no-locale -E UTF8 -U postgres -D ${dataOld} + fi + ''; + }; in pkgs.testers.runNixOSTest { name = "pg-upgrade-start-guard"; - nodes.server = - { ... }: - { - virtualisation.memorySize = 2048; - virtualisation.diskSize = 4096; - users.users.postgres = { + nodes.server = { + virtualisation = { + memorySize = 2048; + diskSize = 4096; + }; + + users = { + users.postgres = { isSystemUser = true; group = "postgres"; home = "/var/lib/postgresql"; createHome = true; }; - users.groups.postgres = { }; - systemd.tmpfiles.rules = [ "d /run/postgresql 0755 postgres postgres -" ]; - - systemd.services.postgresql = { - wantedBy = [ "multi-user.target" ]; - serviceConfig = { - Type = "notify"; - User = "postgres"; - Group = "postgres"; - ExecStartPre = "+${initOld}"; - ExecStart = "${pg15}/bin/postgres -D ${dataOld}"; - KillMode = "mixed"; - KillSignal = "SIGINT"; - Restart = "always"; - RestartSec = 1; - }; + groups.postgres = { }; + }; + + systemd.tmpfiles.rules = [ "d /run/postgresql 0755 postgres postgres -" ]; + + systemd.services.postgresql = { + wantedBy = [ "multi-user.target" ]; + serviceConfig = { + Type = "notify"; + User = "postgres"; + Group = "postgres"; + ExecStartPre = "+${lib.getExe initOld}"; + ExecStart = "${pg15}/bin/postgres -D ${dataOld}"; + KillMode = "mixed"; + KillSignal = "SIGINT"; + Restart = "always"; + RestartSec = 1; }; }; + }; testScript = '' + DRIVE = "${lib.getExe drive}" PG15 = "${pg15}/bin" PG17 = "${pg17}/bin" - DROPIN = "${dropIn}" + OLD = "${dataOld}" + NEW = "${dataNew}" + DROPIN = "/run/systemd/system/postgresql.service.d/pg-upgrade-lock.conf" + + def unit(name, job): + server.succeed(f"systemd-run --unit={name} --collect {DRIVE} {job}") + + def kill(name): + server.succeed(f"systemctl kill -s KILL {name}") + server.wait_until_fails(f"systemctl is-active --quiet {name}") - def sql(query, port=5432, bindir=PG15): + def pg_ctl(bindir, action, data, opts=""): + log = "-l /tmp/pg_ctl.log" if action == "start" else "" + return f"runuser -u postgres -- {bindir}/pg_ctl {action} -D {data} -w -s {log} {opts}" + + def sql(query, bindir=PG15, port=5432): return server.succeed( f"{bindir}/psql -h 127.0.0.1 -p {port} -U postgres -d postgres -Atc \"{query}\"" ).strip() + def show(prop): + return server.succeed(f"systemctl show -p {prop} postgresql") + start_all() server.wait_for_unit("postgresql.service") with subtest("Seed the source cluster"): - sql("create table seed as select g as id, repeat(md5(g::text), 20) as pad from generate_series(1, 400000) g") + sql("create table seed as select g as id, repeat(md5(g::text), 20) as pad from generate_series(1, 100000) g") expected = sql("select count(*) || ',' || sum(id) from seed") with subtest("Control: with no guard the start loop brings the source up"): server.succeed("systemctl stop postgresql") - server.succeed("systemd-run --unit=salt-sim --collect ${saltSim}") - server.wait_until_succeeds("systemctl is-active --quiet postgresql", timeout=30) + unit("salt-sim", "salt_sim") + server.wait_until_succeeds("systemctl is-active --quiet postgresql", timeout=60) server.succeed("systemctl stop salt-sim") with subtest("A systemd start is refused while the block is held"): - server.succeed("systemd-run --unit=hold-block --collect ${holdBlock}") + unit("hold-block", "hold_block") server.wait_until_succeeds("systemctl show -p ExecCondition postgresql | grep -q start-guard", timeout=120) server.fail("systemctl start postgresql") server.sleep(5) server.fail("systemctl is-active --quiet postgresql") - assert "Restart=no" in server.succeed("systemctl show -p Restart postgresql") + assert "Restart=no" in show("Restart") with subtest("The block expires on its own when the holder is SIGKILLed"): - server.succeed("systemctl kill -s KILL hold-block") - server.wait_until_fails("systemctl is-active --quiet hold-block") + kill("hold-block") server.succeed("systemctl start postgresql") server.wait_for_unit("postgresql.service") server.fail(f"test -e {DROPIN}") - assert "Restart=always" in server.succeed("systemctl show -p Restart postgresql") - assert "NeedDaemonReload=no" in server.succeed("systemctl show -p NeedDaemonReload postgresql") + assert "Restart=always" in show("Restart") + assert "NeedDaemonReload=no" in show("NeedDaemonReload") with subtest("The pid lock stops a direct start once the new cluster is up"): server.succeed("systemctl stop postgresql") - server.succeed("systemd-run --unit=hold-lock --collect ${holdLock}") - server.wait_until_succeeds("test -s ${dataOld}/postmaster.pid", timeout=120) - server.fail(f"runuser -u postgres -- {PG15}/pg_ctl start -D ${dataOld} -w -t 5 -s -l /tmp/pg15-locked.log") - server.succeed("systemctl kill -s KILL hold-lock") - server.wait_until_fails("systemctl is-active --quiet hold-lock") - server.succeed(f"runuser -u postgres -- {PG15}/pg_ctl start -D ${dataOld} -w -s -l /tmp/pg15.log") - server.succeed(f"runuser -u postgres -- {PG15}/pg_ctl stop -D ${dataOld} -w -s") + unit("hold-lock", "hold_lock") + server.wait_until_succeeds(f"test -s {OLD}/postmaster.pid", timeout=120) + server.fail(pg_ctl(PG15, "start", OLD, "-t 5")) + kill("hold-lock") + server.succeed(pg_ctl(PG15, "start", OLD)) + server.succeed(pg_ctl(PG15, "stop", OLD)) server.succeed("rm -f /run/pg-upgrade-source.pid") server.succeed("systemctl start postgresql") server.wait_for_unit("postgresql.service") with subtest("A real pg_upgrade copy under a start storm"): - server.succeed("systemd-run --unit=fake-initiate --collect --pipe --wait ${guardedUpgrade}") + server.succeed(f"systemd-run --unit=fake-initiate --collect --pipe --wait {DRIVE} upgrade") attempts = int(server.succeed("wc -l = 5, f"start loop barely ran: {attempts} attempts" server.fail("grep -qx active /tmp/salt-states") with subtest("The new cluster matches the seed"): - server.succeed( - f"runuser -u postgres -- {PG17}/pg_ctl start -D ${dataNew} -w -s -l /tmp/pg17.log -o '-p 5433'" - ) - assert sql("select count(*) || ',' || sum(id) from seed", port=5433, bindir=PG17) == expected - server.succeed(f"runuser -u postgres -- {PG17}/pg_ctl stop -D ${dataNew} -w -s") - - with subtest("The source is untouched and starts again"): - server.succeed("systemctl start postgresql") - server.wait_for_unit("postgresql.service") - assert sql("select count(*) || ',' || sum(id) from seed") == expected - server.fail(f"test -e {DROPIN}") + server.succeed(pg_ctl(PG17, "start", NEW, "-o '-p 5433'")) + assert sql("select count(*) || ',' || sum(id) from seed", PG17, 5433) == expected + server.succeed(pg_ctl(PG17, "stop", NEW)) ''; } From 5be1bb669190146d090b459294c2798a8f9a9e6f Mon Sep 17 00:00:00 2001 From: "andres.suarez" Date: Tue, 13 Oct 2026 12:27:49 +0200 Subject: [PATCH 09/14] fix: ignore non-numeric pids in the start guard and silence SC2016 - The guard now skips pid files that do not hold a number. With bash as sh, `kill -0 not-a-pid` succeeded, so a garbage pid file refused every start. - Silence shellcheck SC2016 for the intentional single-quoted bash -c in the watcher test. Co-Authored-By: Claude Sonnet 5.5 --- .../admin_api_scripts/pg_upgrade_scripts/start_guard.sh | 6 +++++- tests/pg_upgrade/test-start-guard.sh | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh b/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh index 748a72d0ad..47ecaf63d0 100755 --- a/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh +++ b/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh @@ -48,7 +48,11 @@ block_postgres_start() { cat >"$PG_UPGRADE_START_GUARD" <<'GUARD' for pid_file in "$@"; do pid=$(cat "$pid_file" 2>/dev/null) || continue - if [ -n "$pid" ] && kill -0 "$pid" 2>/dev/null; then + # not a number: some shells' kill treat it as a job name, not an error + case "$pid" in + '' | *[!0-9]*) continue ;; + esac + if kill -0 "$pid" 2>/dev/null; then exit 255 fi done diff --git a/tests/pg_upgrade/test-start-guard.sh b/tests/pg_upgrade/test-start-guard.sh index f882d006d1..98d8c88631 100644 --- a/tests/pg_upgrade/test-start-guard.sh +++ b/tests/pg_upgrade/test-start-guard.sh @@ -220,6 +220,7 @@ test_watcher_abandoned() { watcher_pid_file="$PGDATAOLD/../watcher.pid" # a separate shell stands in for a SIGKILLed initiate.sh: it exits, the watcher # stays (BASHPID is missing on bash 3.2, so a subshell would not work here) + # shellcheck disable=SC2016 "$BASH" -c 'log() { :; }; source "$1"; start_pid_lock_watcher; printf "%s\n" "$PG_UPGRADE_LOCK_WATCHER_PID" >"$2"' _ \ "$SCRIPTS_DIR/start_guard.sh" "$watcher_pid_file" while kill -0 "$(cat "$watcher_pid_file")" 2>/dev/null; do From 01c5945e442134917e33ea1c796ee87faf1bec07 Mon Sep 17 00:00:00 2001 From: "andres.suarez" Date: Tue, 13 Oct 2026 12:39:50 +0200 Subject: [PATCH 10/14] test: point start guard test shims at the running bash The Nix build sandbox on Linux has no /usr/bin/env, so the shims failed with "bad interpreter". Rewrite their shebang to $BASH. Co-Authored-By: Claude Sonnet 5.5 --- tests/pg_upgrade/test-start-guard.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/pg_upgrade/test-start-guard.sh b/tests/pg_upgrade/test-start-guard.sh index 98d8c88631..63c861fa8b 100644 --- a/tests/pg_upgrade/test-start-guard.sh +++ b/tests/pg_upgrade/test-start-guard.sh @@ -65,6 +65,14 @@ if [ "\${FAIL_SHARE_RESTORE:-}" = mv ] && [ "\$1" = "\${POSTGRES_SHARE_DIR}.bak" fi exec $(type -P mv) "\$@" SHIM +# The Nix build sandbox on Linux has no /usr/bin/env: point the shims at this bash. +for shim in "$TEST_ROOT/bin/"*; do + { + printf '#!%s\n' "$BASH" + tail -n +2 "$shim" + } >"$shim.tmp" + mv "$shim.tmp" "$shim" +done chmod +x "$TEST_ROOT/bin/"* export PATH="$TEST_ROOT/bin:$PATH" From e68f917bc4a685279ae0b615eaaac55645fdbd62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Boros?= Date: Tue, 13 Oct 2026 13:31:53 +0300 Subject: [PATCH 11/14] test(nix): source start_guard.sh in the start guard VM test Refs: MPG-1334 Co-Authored-By: Claude Sonnet 5.5 --- nix/ext/tests/pg-upgrade-start-guard.nix | 25 +++--------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/nix/ext/tests/pg-upgrade-start-guard.nix b/nix/ext/tests/pg-upgrade-start-guard.nix index 814a4b181c..e2b9154b8f 100644 --- a/nix/ext/tests/pg-upgrade-start-guard.nix +++ b/nix/ext/tests/pg-upgrade-start-guard.nix @@ -7,21 +7,11 @@ let dataOld = "/var/lib/postgresql/data"; dataNew = "/var/lib/postgresql/data-17"; - initiate = builtins.path { - path = ../../../ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh; - name = "initiate.sh"; + guard = builtins.path { + path = ../../../ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh; + name = "start_guard.sh"; }; - # initiate.sh runs the upgrade when sourced, so extract only the guard. - guard = pkgs.pkgsLinux.runCommand "start-guard.sh" { } '' - awk ' - /^PG_UPGRADE_(LOCK|PID|INITIATE|START)_[A-Z_]+=/ { print } - /^(block_postgres_start|start_pid_lock_watcher|unblock_postgres_start|assert_source_stayed_down)\(\) \{/ { printing = 1 } - printing { print } - printing && /^}/ { printing = 0 } - ' ${initiate} >$out - ''; - drive = pkgs.pkgsLinux.writeShellApplication { name = "drive"; runtimeInputs = with pkgs.pkgsLinux; [ @@ -38,15 +28,6 @@ let PGDATAOLD=${dataOld} PGDATANEW=${dataNew} log() { echo "[$(date +%T)] $*"; } - retry() { - local n=$1 i - shift - for i in $(seq 1 "$n"); do - "$@" && return 0 - sleep 1 - done - return 1 - } . ${guard} hold_block() { From 1d915f2c00a7f4426ed275c02c50ac10df028b35 Mon Sep 17 00:00:00 2001 From: "andres.suarez" Date: Tue, 13 Oct 2026 13:29:05 +0200 Subject: [PATCH 12/14] ci: retrigger From 40b636377833400400295de7f8bf067c6bfe3a90 Mon Sep 17 00:00:00 2001 From: "andres.suarez" Date: Tue, 13 Oct 2026 13:29:21 +0200 Subject: [PATCH 13/14] ci: retrigger Co-Authored-By: Claude Sonnet 5.5 From ac3bcc7cd7ec7b1d3f7740463d80f5b23601b65b Mon Sep 17 00:00:00 2001 From: "andres.suarez" Date: Thu, 15 Oct 2026 19:18:18 +0200 Subject: [PATCH 14/14] fix(pg_upgrade): address start guard review comments, ship start_guard.sh - Fail the upgrade when a postmaster.pid already exists in the old data directory once the new cluster is up (a postmaster ran during the copy). - assert_source_stayed_down fails when the lock watcher exited non-zero; tests poll with kill -0 so the exit status is still available. - Take the lock atomically via a temp name plus ln, and clean the temp name up in unblock_postgres_start. - Deploy start_guard.sh to /etc/adminapi/pg_upgrade_scripts and add it to the audit baseline; without it initiate.sh cannot source the guard. Co-Authored-By: Claude Sonnet 5.5 --- .../pg_upgrade_scripts/start_guard.sh | 49 +++++++++++---- ansible/tasks/internal/admin-api.yml | 1 + audit-specs/baselines/baseline.yml | 6 ++ tests/pg_upgrade/test-start-guard.sh | 63 ++++++++++++++++--- 4 files changed, 100 insertions(+), 19 deletions(-) diff --git a/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh b/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh index 47ecaf63d0..231a06040b 100755 --- a/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh +++ b/ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh @@ -17,6 +17,7 @@ PG_UPGRADE_LOCK_DROPIN_DIR="/run/systemd/system/postgresql.service.d" PG_UPGRADE_LOCK_DROPIN="$PG_UPGRADE_LOCK_DROPIN_DIR/pg-upgrade-lock.conf" PG_UPGRADE_PID_LOCK_MARKER="/run/pg-upgrade-source.pid" +PG_UPGRADE_PID_LOCK_TMP_NAME="postmaster.pid.pg-upgrade-lock" PG_UPGRADE_INITIATE_PID_FILE="/run/pg-upgrade-initiate.pid" PG_UPGRADE_START_GUARD="/run/pg-upgrade-start-guard.sh" PG_UPGRADE_LOCK_WATCHER_PID="" @@ -75,7 +76,11 @@ GUARD # Wait for the new cluster's temp server (the old one is down from then on, # its schema dump is done) and then claim the old data dir's postmaster.pid. -# It must not exist before: pg_upgrade aborts if it finds one at startup. +# It must not exist before: pg_upgrade aborts if it finds one at startup, so +# finding one now means a postmaster started during the copy. +# The watcher exits 0 when it took the lock or when there is nothing left to +# lock (initiate.sh and pg_upgrade are gone), and non-zero when it could not +# take the lock: assert_source_stayed_down turns that into a failed upgrade. start_pid_lock_watcher() { local initiate_pid=${BASHPID:-$$} ( @@ -91,22 +96,29 @@ start_pid_lock_watcher() { done upg_pid=$(pgrep -u postgres -f -- "--old-datadir=${PGDATAOLD}" | head -1) if [ -z "$upg_pid" ]; then - exit 0 + log "WARNING: pg_upgrade not found; not creating the pg_upgrade lock" + exit 1 fi if [ -e "$PGDATAOLD/postmaster.pid" ]; then - log "WARNING: $PGDATAOLD/postmaster.pid already exists; not creating the pg_upgrade lock" - exit 0 + log "ERROR: $PGDATAOLD/postmaster.pid already exists; a postmaster ran during the copy" + exit 1 fi printf '%s\n' "$upg_pid" >"$PG_UPGRADE_PID_LOCK_MARKER" || exit 1 - if ! ( - set -o noclobber - cat "$PG_UPGRADE_PID_LOCK_MARKER" >"$PGDATAOLD/postmaster.pid" - ); then - log "WARNING: could not create the pg_upgrade PID lock" + # Build the file aside and link it into place: the link is atomic and fails + # if the name exists, so a watcher killed by unblock_postgres_start can never + # leave a truncated postmaster.pid behind (cmp would not match it). + lock_tmp="$PGDATAOLD/$PG_UPGRADE_PID_LOCK_TMP_NAME" + if ! { + cat "$PG_UPGRADE_PID_LOCK_MARKER" >"$lock_tmp" && + chown postgres:postgres "$lock_tmp" && + chmod 0600 "$lock_tmp" && + ln "$lock_tmp" "$PGDATAOLD/postmaster.pid" + }; then + rm -f "$lock_tmp" + log "ERROR: could not create the pg_upgrade PID lock" exit 1 fi - chown postgres:postgres "$PGDATAOLD/postmaster.pid" - chmod 0600 "$PGDATAOLD/postmaster.pid" + rm -f "$lock_tmp" log "Locked $PGDATAOLD with pg_upgrade pid $upg_pid" ) & PG_UPGRADE_LOCK_WATCHER_PID=$! @@ -120,6 +132,8 @@ unblock_postgres_start() { PG_UPGRADE_LOCK_WATCHER_PID="" fi local pidfile="${PGDATAOLD:-/nonexistent}/postmaster.pid" + # a watcher killed between writing and linking leaves only this temp file + rm -f "${PGDATAOLD:-/nonexistent}/$PG_UPGRADE_PID_LOCK_TMP_NAME" # Callers run this under `||` where set -e is off, so report failures through # the return status: a failed daemon-reload leaves the block loaded in systemd. local rc=0 @@ -155,7 +169,18 @@ unblock_postgres_start() { # The source must have stayed down for the whole copy; if not, the copy is # inconsistent and the upgrade has to fail (the source data is untouched). assert_source_stayed_down() { - local state="" + local state="" watcher_rc=0 + # A watcher that is still polling is fine; one that is gone must have exited 0. + # `wait` reports a child's status only once, so nothing may wait on the watcher + # before this check (unblock_postgres_start does, and runs after it). 127 means + # the status is not available (already reaped), not that the watcher failed. + if [ -n "$PG_UPGRADE_LOCK_WATCHER_PID" ] && ! kill -0 "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null; then + wait "$PG_UPGRADE_LOCK_WATCHER_PID" || watcher_rc=$? + if [ "$watcher_rc" -ne 0 ] && [ "$watcher_rc" -ne 127 ]; then + log "ERROR: the pg_upgrade lock could not be taken; the copied data files may be inconsistent" + return 1 + fi + fi if [ -z "$IS_CI" ]; then state=$(systemctl is-active postgresql || true) fi diff --git a/ansible/tasks/internal/admin-api.yml b/ansible/tasks/internal/admin-api.yml index dcdc68a393..e7d30b71ee 100644 --- a/ansible/tasks/internal/admin-api.yml +++ b/ansible/tasks/internal/admin-api.yml @@ -74,6 +74,7 @@ - { file: "prepare.sh" } - { file: "pgsodium_getkey.sh" } - { file: "common.sh" } + - { file: "start_guard.sh" } - name: adminapi - create service file template: diff --git a/audit-specs/baselines/baseline.yml b/audit-specs/baselines/baseline.yml index d26d55543b..148dad8886 100644 --- a/audit-specs/baselines/baseline.yml +++ b/audit-specs/baselines/baseline.yml @@ -145,6 +145,12 @@ file: owner: "1006" group: "0" filetype: file + /etc/adminapi/pg_upgrade_scripts/start_guard.sh: + exists: true + mode: "0755" + owner: "1006" + group: "0" + filetype: file /etc/alternatives/README: exists: true mode: "0644" diff --git a/tests/pg_upgrade/test-start-guard.sh b/tests/pg_upgrade/test-start-guard.sh index 63c861fa8b..67fa160c7e 100644 --- a/tests/pg_upgrade/test-start-guard.sh +++ b/tests/pg_upgrade/test-start-guard.sh @@ -9,9 +9,9 @@ trap 'rm -rf "$TEST_ROOT"' EXIT SCRIPTS_DIR="$REPO_ROOT/ansible/files/admin_api_scripts/pg_upgrade_scripts" # cleanup and on_exit are part of initiate.sh itself, which cannot be sourced. -eval "$(awk ' +eval "$(awk -v status_file="$TEST_ROOT/pg-upgrade-status" ' /^(cleanup|on_exit)\(\) \{/ { printing = 1 } - printing { print } + printing { gsub("/tmp/pg-upgrade-status", status_file); print } printing && /^}/ { printing = 0 } ' "$SCRIPTS_DIR/initiate.sh")" @@ -110,7 +110,7 @@ setup() { export IS_LOCAL_UPGRADE PGVERSION PGDATAOLD PGDATANEW MOUNT_POINT LOG_FILE export POSTGRES_SHARE_DIR POST_UPGRADE_EXTENSION_SCRIPT POST_UPGRADE_POSTGRES_PERMS_SCRIPT export PG_UPGRADE_LOCK_DROPIN_DIR PG_UPGRADE_LOCK_DROPIN PG_UPGRADE_PID_LOCK_MARKER - export PG_UPGRADE_INITIATE_PID_FILE PG_UPGRADE_START_GUARD + export PG_UPGRADE_INITIATE_PID_FILE PG_UPGRADE_START_GUARD PG_UPGRADE_PID_LOCK_TMP_NAME export PG_UPGRADE_LOCK_WATCHER_PID PG_UPGRADE_START_BLOCKED CLEANUP_STARTED export SYSTEMCTL_LOG="$test_dir/systemctl.log" export FAIL_RELOAD_ONCE="$test_dir/fail-reload" @@ -177,11 +177,23 @@ test_unblock_reload_retry() { assert test "$(grep -c '^daemon-reload$' "$SYSTEMCTL_LOG")" -eq 3 } +# Not `wait`: it reports a child's status once, and assert_source_stayed_down +# needs it. +wait_for_watcher() { + local tries=0 + while kill -0 "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null; do + tries=$((tries + 1)) + assert test "$tries" -lt 50 + sleep 0.1 + done +} + test_owned_pid() { setup touch "$PGDATANEW/postmaster.pid" start_pid_lock_watcher - wait "$PG_UPGRADE_LOCK_WATCHER_PID" + wait_for_watcher + assert test ! -e "$PGDATAOLD/$PG_UPGRADE_PID_LOCK_TMP_NAME" assert cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$PGDATAOLD/postmaster.pid" assert_source_stayed_down unblock_postgres_start @@ -216,11 +228,48 @@ test_watcher_existing_pid() { touch "$PGDATANEW/postmaster.pid" printf '54321\n' >"$PGDATAOLD/postmaster.pid" start_pid_lock_watcher - wait "$PG_UPGRADE_LOCK_WATCHER_PID" + wait_for_watcher + # the watcher could not take the lock: the upgrade must fail + if assert_source_stayed_down; then + exit 1 + fi unblock_postgres_start assert test "$(cat "$PGDATAOLD/postmaster.pid")" = 54321 } +# pg_upgrade is gone by the time the new cluster's pid file shows up +test_watcher_no_pg_upgrade() { + setup + export PGREP_EMPTY=1 + touch "$PGDATANEW/postmaster.pid" + # initiate_pid is this shell, so the watcher does not abandon the wait + start_pid_lock_watcher + wait_for_watcher + unset PGREP_EMPTY + if assert_source_stayed_down; then + exit 1 + fi + assert test ! -e "$PGDATAOLD/postmaster.pid" +} + +# a watcher still polling is not a failure +test_watcher_still_polling() { + setup + start_pid_lock_watcher + assert_source_stayed_down + unblock_postgres_start + assert test -z "$PG_UPGRADE_LOCK_WATCHER_PID" +} + +# the watcher killed between writing and linking leaves no pid file behind +test_unblock_removes_lock_tmp() { + setup + printf '1\n' >"$PGDATAOLD/$PG_UPGRADE_PID_LOCK_TMP_NAME" + unblock_postgres_start + assert test ! -e "$PGDATAOLD/$PG_UPGRADE_PID_LOCK_TMP_NAME" + assert test ! -e "$PGDATAOLD/postmaster.pid" +} + test_watcher_abandoned() { local watcher_pid_file tries=0 setup @@ -361,8 +410,8 @@ test_exit_cleanup() { done } -export -f log retry block_postgres_start unblock_postgres_start assert_source_stayed_down cleanup on_exit -for test_name in test_dropin test_no_block_reset test_block_reload_failure test_unblock_reload_retry test_owned_pid test_foreign_pid test_watcher_existing_pid test_watcher_abandoned test_active_source test_start_guard test_dropin_expiry test_exit_cleanup; do +export -f log retry assert block_postgres_start unblock_postgres_start assert_source_stayed_down cleanup on_exit +for test_name in test_dropin test_no_block_reset test_block_reload_failure test_unblock_reload_retry test_owned_pid test_foreign_pid test_watcher_existing_pid test_watcher_no_pg_upgrade test_watcher_still_polling test_unblock_removes_lock_tmp test_watcher_abandoned test_active_source test_start_guard test_dropin_expiry test_exit_cleanup; do case_output="$TEST_ROOT/$test_name.log" trap 'exit_code=$?; cat "$case_output"; exit "$exit_code"' ERR (