Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A failed daemon reload can leave PostgreSQL permanently blocked, and the production-only guard lacks automated coverage.
1 open finding
What changed in this PR
Adds safeguards to keep the source PostgreSQL cluster stopped during pg_upgrade copy mode.
Changes:
- Adds systemd and PID-file startup guards.
- Adds post-copy validation and cleanup handling.
- Adds exit and signal cleanup traps.
| File | Description |
|---|---|
ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh |
Implements source-cluster startup prevention and cleanup. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+100
to
+105
| 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" | ||
| systemctl daemon-reload |
hunleyd
reviewed
Oct 7, 2026
| PG_UPGRADE_LOCK_DROPIN="$PG_UPGRADE_LOCK_DROPIN_DIR/pg-upgrade-lock.conf" | ||
| PG_UPGRADE_LOCK_WATCHER_PID="" | ||
|
|
||
| block_postgres_start() { |
Contributor
There was a problem hiding this comment.
alternatives to a drop-in that might be cleaner:
- systemctl disable --now postgresql (stops it, prevents start on boot)
- systemctl mask postgresql (prevents starts/stop/restarts right now)
- (do upgrade things)
- (for rollback) systemctl unmask postgresql (allows start/stop/restart again)
- systemctl enable --now postgresql (starts it and re-enables start on boot)
Contributor
Author
There was a problem hiding this comment.
Thanks for the suggestion. We looked into using mask/unmask, but ruled it out. postgresql.service is installed as a regular file under /etc/systemd/system and is managed by Salt, so applying a persistent mask would require replacing that file. That also means an unmask during rollback wouldn’t reliably restore the original unit.
Instead, we use a runtime drop-in under /run/systemd/system/postgresql.service.d/. This leaves the unit file untouched, is automatically cleared on reboot, and is explicitly removed by cleanup() and the EXIT trap.
We also keep the disable/stop from step 10 and use enable + restart during rollback, which is effectively equivalent to enable --now.
PostgreSQL Extension Dependency Analysis: PR #2530
SummaryNo extensions had dependencies with MAJOR version updates. Full Analysis ResultsPostgreSQL 15 Extension DependenciesPostgreSQL 17 Extension DependenciesOrioleDB 17 Extension Dependencies |
PostgreSQL Package Dependency Analysis: PR #2530
SummaryNo packages had MAJOR version updates. Full Analysis ResultsPostgreSQL 15 Dependency ChangesExtracting PostgreSQL 15 dependencies...
Runtime Closure Size
Raw Dependency ClosurePostgreSQL 17 Dependency ChangesExtracting PostgreSQL 17 dependencies...
Runtime Closure Size
Raw Dependency Closure |
valigula
changed the title
valigula
marked this pull request as ready for review
October 8, 2026 15:54
Contributor
Author
Real-host test evidence (local-dev, EC2, PG14 -> PG17)Runs 1-4 were done on the earlier version of the guard ( Setup: platform local-dev creating real EC2 projects (eu-central-1), source
Rollback coverage
Findings
Not yet checked / tested
|
Package version diff: none (aarch64-linux, x86_64-linux) |
This comment has been minimized.
This comment has been minimized.
brainrake
requested changes
Oct 9, 2026
brainrake
requested changes
Oct 9, 2026
Collaborator
|
Hooking it up to run in a check. |
brainrake
requested changes
Oct 9, 2026
Collaborator
|
Added to CI checks in |
brainrake
requested changes
Oct 9, 2026
Collaborator
|
Stacked a VM test PR on top, will go green once hardcoded /bin is fixed: #2548 |
Collaborator
|
Fixed some minor issues. #2548 should build fine on top of this now, feel free to merge it in here. |
This comment has been minimized.
This comment has been minimized.
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>
…t-during-copy' into fix/pg-upgrade-block-source-start-during-copy
Refs: MPG-1334 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Refs: MPG-1334 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…s 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>
Refs: MPG-1334 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
- 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>
…t-during-copy' into martonboros/mpg-1334-pg-upgrade-start-guard-vm-test
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>
Refs: MPG-1334 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…t-during-copy' into martonboros/mpg-1334-pg-upgrade-start-guard-vm-test
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
mmlb
force-pushed
the
fix/pg-upgrade-block-source-start-during-copy
branch
from
October 13, 2026 19:47
3cf4a88 to
0e603e7
Compare
Collaborator
|
Rebased on top of latest develop so CI cleans up its AMIs |
brainrake
requested changes
Oct 14, 2026
Collaborator
There was a problem hiding this comment.
We need to either move the new functions to common.sh or add the new start_guard.sh in a few places
In ansible/tasks/internal/admin-api.yml:
- { file: "common.sh" }
+ - { file: "start_guard.sh" }
In audit-specs/baselines/baseline.yml:
/etc/adminapi/pg_upgrade_scripts/prepare.sh:
exists: true
mode: "0755"
owner: "1006"
group: "0"
filetype: file
+ /etc/adminapi/pg_upgrade_scripts/start_guard.sh:
+ exists: true
+ mode: "0755"
+ owner: "1006"
+ group: "0"
+ filetype: file
Also: pg_upgrade refuses to start if the old data dir has a postmaster.pid. So when the watcher finds one after the new cluster is up, a postmaster ran in the copy window. Warning and continuing lets the copy finish. The end check only catches a postmaster that is still running. Added suggested changes for this.
| exit 0 | ||
| fi | ||
| if [ -e "$PGDATAOLD/postmaster.pid" ]; then | ||
| log "WARNING: $PGDATAOLD/postmaster.pid already exists; not creating the pg_upgrade lock" |
Collaborator
There was a problem hiding this comment.
Suggested change
| log "WARNING: $PGDATAOLD/postmaster.pid already exists; not creating the pg_upgrade lock" | |
| log "ERROR: $PGDATAOLD/postmaster.pid already exists; a postmaster ran during the copy" | |
| exit 1 |
pg_upgrade aborts on an old postmaster.pid at startup, so one here means a postmaster started mid-copy. Exit non-zero instead of running unlocked.
| # 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="" |
Collaborator
There was a problem hiding this comment.
Suggested change
| local state="" | |
| local state="" | |
| # the lock watcher exits non-zero when it could not take the lock | |
| if [ -n "$PG_UPGRADE_LOCK_WATCHER_PID" ] && ! kill -0 "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null && ! wait "$PG_UPGRADE_LOCK_WATCHER_PID"; then | |
| log "ERROR: the pg_upgrade lock could not be taken; the copied data files may be inconsistent" | |
| return 1 | |
| fi |
The check fails the upgrade on the watcher’s exit status. The end check misses a postmaster that started and stopped during the copy. The kill -0 skips a watcher that is still polling. wait reports a child’s status only once, so nothing should wait on it before this check.
| touch "$PGDATANEW/postmaster.pid" | ||
| printf '54321\n' >"$PGDATAOLD/postmaster.pid" | ||
| start_pid_lock_watcher | ||
| wait "$PG_UPGRADE_LOCK_WATCHER_PID" |
Collaborator
There was a problem hiding this comment.
Suggested change
| wait "$PG_UPGRADE_LOCK_WATCHER_PID" | |
| wait "$PG_UPGRADE_LOCK_WATCHER_PID" | |
| while kill -0 "$PG_UPGRADE_LOCK_WATCHER_PID" 2>/dev/null; do sleep 0.1; done | |
| if assert_source_stayed_down; then | |
| exit 1 | |
| fi |
Wait for the watcher with kill -0, not wait, so its status is still available. Assert the check fails.
…d.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>
hitmands
approved these changes
Oct 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
MPG-1334/prevent-postgres-from-restarting-during-the-upgrade-file-copy
Problem
During a PG14 → PG17
pg_upgrade(copy mode) of a 1.17 TB project, the source production Postgres was started again 9 minutes after step 10 stopped it and ran for ~3h45m whilepg_upgradewas copying the data files (SU-491229).The resulting PG17 data directory was internally inconsistent, including:
Despite this,
pg_upgradestill reported the upgrade as complete.Nothing prevented Postgres from being started again:
systemctl disable+stop.disableonly affects boot-time startup.postgresql.servicehasRestart=always.pg_upgradeonly checks for a running postmaster at startup.The leading candidate is the
supabase-admin-agentSalt run (reload_pg_server,service.running+enable: True), which restarted a stopped and disabled Postgres within ~7 seconds of the timer tick on staging.The fix does not depend on which caller starts Postgres.
Changes (
initiate.sh)Add two independent guards from step 10 until cleanup, plus a post-copy validation:
Self-expiring systemd drop-in (
block_postgres_start)After step 10's final stop, write:
/run/systemd/system/postgresql.service.d/pg-upgrade-lock.confwith:
The guard exits 255, which fails the start, only while the pid in one of the two files is alive: the
initiate.shbackground subshell ($BASHPID, since the main script exits right after launching it) or thepg_upgradelock pid. This blocks every systemd start, including Salt, adminapi, an ERR-trap restart, or a manual one.ExecConditionruns before the unit's ownExecStartPre(postgres_prestart.sh), so a refused start does not run it.The block expires by itself: if
initiate.shis SIGKILLed or OOM-killed (no trap can run), the next start afterpg_upgradeis gone passes the guard and theExecStartPostlines remove the drop-in, so neither the block norRestart=nooutlives the upgrade and no reboot is needed. The+prefix is required because the unit runs asUser=postgres: a non-root guard cannotkill -0the root-owned initiate pid and would treat it as dead, and could not remove files under/run.The drop-in lives under
/run, so a reboot also clears it. It is skipped in CI environments without systemd.pg_upgradePID lock (start_pid_lock_watcher)Start a background watcher that waits for the new cluster's
postmaster.pid. By this point, the old server's dump is complete, so the old data directory can be used for the lock.The watcher writes
pg_upgrade's own PID to:$PGDATAOLD/postmaster.pidAny postmaster, including one started directly with
pg_ctl, will refuse to start while that PID is alive. The file becomes stale automatically whenpg_upgradeexits.The lock cannot be created earlier because
pg_upgradeaborts ifpostmaster.pidalready exists at startup.Post-copy check (
assert_source_stayed_down)After
pg_upgradereturns, verify that:postgresqlis not active, andpostmaster.pidwas not written by a real postmaster.If either check fails, the upgrade fails.
The ERR trap then runs
cleanup(), which restarts the untouched source cluster. Copy mode does not modify the source data directory.Cleanup (
unblock_postgres_start)Called from
cleanup()before restarting the source, on both success and failure paths.Cleanup is idempotent and:
postmaster.pid.systemctl daemon-reloadandsystemctl reset-failed.Notes and risks
Salt will log one failed agent run per 10-minute tick while the copy is in progress. This is expected while the upgrade lock is active.
cleanup()and the guarded EXIT trap (on_exit) lift the block on every normal exit path (success, failedpg_upgrade, TERM/INT) and restore the source themselves, without the worker starting Postgres. SIGKILL/OOM cannot run any trap; there the guard expires on its own (see 1.). The worker's owncleanup_upgradethen recovers the project once its poll budget ends.Bash defers a TERM/INT trap until the foreground
pg_upgradereturns, unless the whole process group is killed.The window between the old server stopping and the watcher writing the PID lock is covered only by the drop-in.
A systemd start-limit burst (5+ starts in 10 s) makes
database-optimizations.servicereport "Dependency failed" for those 10 s. It is transient and does not affect the rollback restart.pg_upgrade's PID is found withpgrep -f -- --old-datadir=...rather than by binary name, so wrapper binaries do not affect PID detection.Not in this PR:
admin-mgr is-busyreturn true during upgrades.unlessguard onreload_pg_server.retry(3)).complete.shfail on vacuum/amcheck errors.Testing
https://app.notion.com/p/supabase/MPG-1334-guard-against-Postgres-restarting-during-the-pg_upgrade-file-copy-local-dev-test-report-3f35004b775f80f4a3ece159ef122732?source=copy_link
bash -n initiate.shbash tests/pg_upgrade/test-start-guard.sh(11 tests with a shimmed systemd/pgrep; not wired into CI, seenix/docs/testing-pg-upgrade-scripts.md)salt-enabledflipped totruewhere noted), full report in the Notion page above:start,enable --nowandrestartrefused, directpostgres -Drefused by the pid lock, Saltreload_pg_serverfails and Postgres stays down; both guards removed at step 13 and the source restarted by the script.pg_upgradekilled): ERR trap →cleanup failed, drop-in and lock removed, sourceactive/enabled, no worker start.initiate.shmid-copy: trap deferred untilpg_upgradereturned, thencleanup failed, no steps 11-13, source restored.postmaster.pidwhilepg_upgradewas frozen):assert_source_stayed_downfails the upgrade, rollback restores the source.initiate.shmid-copy: with the first version the source stayed blocked until a reboot; with the self-expiring guard a start works oncepg_upgradeis gone, without a reboot.ExecCondition=+,ExecStartPostself-removal) on the real systemd 245 via a harness that sources the realblock_postgres_start: refused while only the root-owned initiate pid is alive; after it dies the next start succeeds, the drop-in and pid files are removed andRestart=alwaysis back.+/ExecStartPostchanges.14.1.0.62, older unit and agent): not available in the test account.