fix: MPG-1334 keep source postgres down during pg_upgrade copy (#2530) · supabase/postgres@8574e6c · GitHub
Skip to content

Commit 8574e6c

Browse files
valigulabrainrakeclaude
authored
fix: MPG-1334 keep source postgres down during pg_upgrade copy (#2530)
* fix: keep source postgres down during pg_upgrade copy * fix: address review findings in pg_upgrade source-start guard * test: cover pg_upgrade start guard and harden exit cleanup * 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 <noreply@anthropic.com> * test(nix): run pg_upgrade start guard test in flake check Refs: MPG-1334 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(nix): add pg_upgrade start guard VM test Refs: MPG-1334 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * test(nix): tidy pg_upgrade start guard VM test Refs: MPG-1334 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * test(nix): source start_guard.sh in the start guard VM test Refs: MPG-1334 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * ci: retrigger * ci: retrigger Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Márton Boros <martonboros@gmail.com> Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
1 parent 3d0f24b commit 8574e6c

8 files changed

Lines changed: 919 additions & 12 deletions

File tree

‎ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh‎

Lines changed: 59 additions & 12 deletions

‎ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh‎

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
#! /usr/bin/env bash
2+
3+
# Start guard for initiate.sh: keeps the source Postgres down during the pg_upgrade
4+
# file copy. Sourced by initiate.sh and tests/pg_upgrade/test-start-guard.sh.
5+
# Expects log, IS_CI, PGDATAOLD and PGDATANEW from the caller.
6+
7+
# From step 10 until cleanup, nothing may start the source postgres: pg_upgrade
8+
# copies the data files without a postmaster holding the directory, and a start
9+
# in that window (Salt's `service.running`, adminapi, Restart=always, ...)
10+
# yields a copy that is inconsistent with the dump. Two independent guards:
11+
# 1. a runtime systemd drop-in that makes `systemctl start` fail while this
12+
# script or pg_upgrade is alive (a SIGKILLed script must not leave the
13+
# source blocked until a reboot);
14+
# 2. a postmaster.pid in the old data dir holding pg_upgrade's own PID, which
15+
# makes any postmaster (systemd or direct pg_ctl) refuse to start while
16+
# pg_upgrade lives. The file goes stale when pg_upgrade exits.
17+
PG_UPGRADE_LOCK_DROPIN_DIR="/run/systemd/system/postgresql.service.d"
18+
PG_UPGRADE_LOCK_DROPIN="$PG_UPGRADE_LOCK_DROPIN_DIR/pg-upgrade-lock.conf"
19+
PG_UPGRADE_PID_LOCK_MARKER="/run/pg-upgrade-source.pid"
20+
PG_UPGRADE_PID_LOCK_TMP_NAME="postmaster.pid.pg-upgrade-lock"
21+
PG_UPGRADE_INITIATE_PID_FILE="/run/pg-upgrade-initiate.pid"
22+
PG_UPGRADE_START_GUARD="/run/pg-upgrade-start-guard.sh"
23+
PG_UPGRADE_LOCK_WATCHER_PID=""
24+
PG_UPGRADE_START_BLOCKED=""
25+
26+
block_postgres_start() {
27+
# systemd needs absolute paths, and a drop-in naming a missing binary would
28+
# silently never expire: fail here instead.
29+
local sh_path rm_path systemctl_path
30+
sh_path=$(type -P sh) || {
31+
log "ERROR: sh not found in PATH"
32+
return 1
33+
}
34+
rm_path=$(type -P rm) || {
35+
log "ERROR: rm not found in PATH"
36+
return 1
37+
}
38+
systemctl_path=$(type -P systemctl) || {
39+
log "ERROR: systemctl not found in PATH"
40+
return 1
41+
}
42+
mkdir -p "$PG_UPGRADE_LOCK_DROPIN_DIR"
43+
# initiate_upgrade runs in a background subshell: $BASHPID is its own pid, $$ is
44+
# the already exited parent.
45+
printf '%s\n' "${BASHPID:-$$}" >"$PG_UPGRADE_INITIATE_PID_FILE"
46+
# Refuse a start only while the pid in one of the files is alive, so a SIGKILLed
47+
# script stops blocking once pg_upgrade is gone too. Run through sh by the
48+
# drop-in because /run is mounted noexec.
49+
cat >"$PG_UPGRADE_START_GUARD" <<'GUARD'
50+
for pid_file in "$@"; do
51+
pid=$(cat "$pid_file" 2>/dev/null) || continue
52+
# not a number: some shells' kill treat it as a job name, not an error
53+
case "$pid" in
54+
'' | *[!0-9]*) continue ;;
55+
esac
56+
if kill -0 "$pid" 2>/dev/null; then
57+
exit 255
58+
fi
59+
done
60+
exit 0
61+
GUARD
62+
# ExecCondition runs before the unit's own ExecStartPre (postgres_prestart.sh,
63+
# as root), which therefore does not run for a refused start. Exit 255 fails the
64+
# start, 1-254 would only skip it silently. The unit does not run as root, so
65+
# `+` is needed to see root-owned pids and to remove files under /run. A start
66+
# that gets past the guard means this script and pg_upgrade are gone: the
67+
# ExecStartPost lines then remove the drop-in so Restart=no does not outlive it.
68+
printf '[Service]\nExecCondition=+%s %s %s %s\n' \
69+
"$sh_path" "$PG_UPGRADE_START_GUARD" "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_PID_LOCK_MARKER" >"$PG_UPGRADE_LOCK_DROPIN"
70+
printf 'ExecStartPost=-+%s -f %s %s %s %s\nExecStartPost=-+%s daemon-reload\nRestart=no\n' \
71+
"$rm_path" "$PG_UPGRADE_LOCK_DROPIN" "$PG_UPGRADE_INITIATE_PID_FILE" "$PG_UPGRADE_PID_LOCK_MARKER" "$PG_UPGRADE_START_GUARD" \
72+
"$systemctl_path" >>"$PG_UPGRADE_LOCK_DROPIN"
73+
PG_UPGRADE_START_BLOCKED=1
74+
systemctl daemon-reload
75+
}
76+
77+
# Wait for the new cluster's temp server (the old one is down from then on,
78+
# its schema dump is done) and then claim the old data dir's postmaster.pid.
79+
# It must not exist before: pg_upgrade aborts if it finds one at startup, so
80+
# finding one now means a postmaster started during the copy.
81+
# The watcher exits 0 when it took the lock or when there is nothing left to
82+
# lock (initiate.sh and pg_upgrade are gone), and non-zero when it could not
83+
# take the lock: assert_source_stayed_down turns that into a failed upgrade.
84+
start_pid_lock_watcher() {
85+
local initiate_pid=${BASHPID:-$$}
86+
(
87+
# set -E would otherwise run cleanup() from this subshell
88+
trap - ERR EXIT
89+
set +e
90+
until [ -e "$PGDATANEW/postmaster.pid" ]; do
91+
# initiate.sh SIGKILLed and pg_upgrade gone: nothing left to lock
92+
if ! kill -0 "$initiate_pid" 2>/dev/null && ! pgrep -u postgres -f -- "--old-datadir=${PGDATAOLD}" >/dev/null; then
93+
exit 0
94+
fi
95+
sleep 0.2
96+
done
97+
upg_pid=$(pgrep -u postgres -f -- "--old-datadir=${PGDATAOLD}" | head -1)
98+
if [ -z "$upg_pid" ]; then
99+
log "WARNING: pg_upgrade not found; not creating the pg_upgrade lock"
100+
exit 1
101+
fi
102+
if [ -e "$PGDATAOLD/postmaster.pid" ]; then
103+
log "ERROR: $PGDATAOLD/postmaster.pid already exists; a postmaster ran during the copy"
104+
exit 1
105+
fi
106+
printf '%s\n' "$upg_pid" >"$PG_UPGRADE_PID_LOCK_MARKER" || exit 1
107+
# Build the file aside and link it into place: the link is atomic and fails
108+
# if the name exists, so a watcher killed by unblock_postgres_start can never
109+
# leave a truncated postmaster.pid behind (cmp would not match it).
110+
lock_tmp="$PGDATAOLD/$PG_UPGRADE_PID_LOCK_TMP_NAME"
111+
if ! {
112+
cat "$PG_UPGRADE_PID_LOCK_MARKER" >"$lock_tmp" &&
113+
chown postgres:postgres "$lock_tmp" &&
114+
chmod 0600 "$lock_tmp" &&
115+
ln "$lock_tmp" "$PGDATAOLD/postmaster.pid"
116+
}; then
117+
rm -f "$lock_tmp"
118+
log "ERROR: could not create the pg_upgrade PID lock"
119+
exit 1
120+
fi
121+
rm -f "$lock_tmp"
122+
log "Locked $PGDATAOLD with pg_upgrade pid $upg_pid"
123+
) &
124+
PG_UPGRADE_LOCK_WATCHER_PID=$!
125+
}
126+
127+
# Idempotent: cleanup() can run before step 10 or more than once.
128+
unblock_postgres_start() {
129+
if [ -n "$PG_UPGRADE_LOCK_WATCHER_PID" ]; then
130+
kill "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null || true
131+
wait "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null || true
132+
PG_UPGRADE_LOCK_WATCHER_PID=""
133+
fi
134+
local pidfile="${PGDATAOLD:-/nonexistent}/postmaster.pid"
135+
# a watcher killed between writing and linking leaves only this temp file
136+
rm -f "${PGDATAOLD:-/nonexistent}/$PG_UPGRADE_PID_LOCK_TMP_NAME"
137+
# Callers run this under `||` where set -e is off, so report failures through
138+
# the return status: a failed daemon-reload leaves the block loaded in systemd.
139+
local rc=0
140+
if [ -f "$PG_UPGRADE_PID_LOCK_MARKER" ]; then
141+
if [ -f "$pidfile" ] && cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$pidfile"; then
142+
rm -f "$pidfile" || rc=1
143+
fi
144+
if [ "$rc" -eq 0 ]; then
145+
rm -f "$PG_UPGRADE_PID_LOCK_MARKER" || rc=1
146+
fi
147+
fi
148+
if [ -z "$IS_CI" ]; then
149+
if [ -e "$PG_UPGRADE_LOCK_DROPIN" ]; then
150+
PG_UPGRADE_START_BLOCKED=1
151+
rm -f "$PG_UPGRADE_LOCK_DROPIN" || rc=1
152+
rm -f "$PG_UPGRADE_INITIATE_PID_FILE" || rc=1
153+
rmdir --ignore-fail-on-non-empty "$PG_UPGRADE_LOCK_DROPIN_DIR" || true
154+
fi
155+
systemctl daemon-reload || rc=1
156+
# Only after a successful reload: with the drop-in still loaded, a missing
157+
# guard exits 127, which systemd treats as a silent skip of the start.
158+
if [ "$rc" -eq 0 ]; then
159+
rm -f "$PG_UPGRADE_START_GUARD" || rc=1
160+
fi
161+
if [ "$rc" -eq 0 ] && [ -n "$PG_UPGRADE_START_BLOCKED" ]; then
162+
systemctl reset-failed postgresql || true
163+
PG_UPGRADE_START_BLOCKED=""
164+
fi
165+
fi
166+
return $rc
167+
}
168+
169+
# The source must have stayed down for the whole copy; if not, the copy is
170+
# inconsistent and the upgrade has to fail (the source data is untouched).
171+
assert_source_stayed_down() {
172+
local state="" watcher_rc=0
173+
# A watcher that is still polling is fine; one that is gone must have exited 0.
174+
# `wait` reports a child's status only once, so nothing may wait on the watcher
175+
# before this check (unblock_postgres_start does, and runs after it). 127 means
176+
# the status is not available (already reaped), not that the watcher failed.
177+
if [ -n "$PG_UPGRADE_LOCK_WATCHER_PID" ] && ! kill -0 "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null; then
178+
wait "$PG_UPGRADE_LOCK_WATCHER_PID" || watcher_rc=$?
179+
if [ "$watcher_rc" -ne 0 ] && [ "$watcher_rc" -ne 127 ]; then
180+
log "ERROR: the pg_upgrade lock could not be taken; the copied data files may be inconsistent"
181+
return 1
182+
fi
183+
fi
184+
if [ -z "$IS_CI" ]; then
185+
state=$(systemctl is-active postgresql || true)
186+
fi
187+
if [ "$state" = "active" ] || [ "$state" = "activating" ] || [ "$state" = "reloading" ]; then
188+
log "ERROR: postgresql.service is ${state} after pg_upgrade; the copied data files may be inconsistent"
189+
return 1
190+
fi
191+
if [ -f "$PGDATAOLD/postmaster.pid" ] && ! cmp -s "$PG_UPGRADE_PID_LOCK_MARKER" "$PGDATAOLD/postmaster.pid"; then
192+
log "ERROR: a postmaster ran in $PGDATAOLD during pg_upgrade; the copied data files may be inconsistent"
193+
return 1
194+
fi
195+
}

‎ansible/tasks/internal/admin-api.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@
7474
- { file: "prepare.sh" }
7575
- { file: "pgsodium_getkey.sh" }
7676
- { file: "common.sh" }
77+
- { file: "start_guard.sh" }
7778

7879
- name: adminapi - create service file
7980
template:

‎audit-specs/baselines/baseline.yml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,12 @@ file:
145145
owner: "1006"
146146
group: "0"
147147
filetype: file
148+
/etc/adminapi/pg_upgrade_scripts/start_guard.sh:
149+
exists: true
150+
mode: "0755"
151+
owner: "1006"
152+
group: "0"
153+
filetype: file
148154
/etc/alternatives/README:
149155
exists: true
150156
mode: "0644"

‎nix/checks.nix‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -623,6 +623,21 @@
623623
psql_17_cli = pkgs.runCommand "run-check-harness-psql-17-cli" { } (
624624
lib.getExe (makeCheckHarness self'.packages."psql_17_cli/bin" { isCliVariant = true; })
625625
);
626+
pg_upgrade_start_guard =
627+
let
628+
src = lib.fileset.toSource {
629+
root = ../.;
630+
fileset = lib.fileset.unions [
631+
../tests/pg_upgrade/test-start-guard.sh
632+
../ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh
633+
../ansible/files/admin_api_scripts/pg_upgrade_scripts/start_guard.sh
634+
];
635+
};
636+
in
637+
pkgs.runCommand "pg-upgrade-start-guard-test" { } ''
638+
bash ${src}/tests/pg_upgrade/test-start-guard.sh
639+
touch $out
640+
'';
626641
# Portable CLI bundle portability checks
627642
psql_17_cli_portable =
628643
pkgs.runCommand "psql_17_cli_portable-portability-check"

‎nix/docs/testing-pg-upgrade-scripts.md‎

Lines changed: 14 additions & 0 deletions

0 commit comments

Comments
 (0)