fix(bootstrap): serialize concurrent migrations with a PostgreSQL advisory lock - #235
fix(bootstrap): serialize concurrent migrations with a PostgreSQL advisory lock#235robinnsc wants to merge 1 commit into
Conversation
| // whole migration. Taking it on the catalog database serializes every | ||
| // migrator, whichever database the migration statements themselves | ||
| // touch. | ||
| // |
There was a problem hiding this comment.
RDS and Aurora direct connections support session advisory locks fully; only a transaction-pooling proxy in between breaks it, and it breaks silently (the lock lands on an arbitrary pooled backend session, so you get false safety, not an error). Documentation alone leaves that silent. Cheap hardening: after acquiring, verify in a single statement (proxy-robust, since one statement maps to one backend) that this session holds the lock:
SELECT count(*) = 1 FROM pg_locks
WHERE locktype = 'advisory' AND classid = $1 AND objid = $2
AND pid = pg_backend_pid()
and error out at startup if false. That turns silent false safety into a hard, diagnosable failure. Suggestion, not blocking.
|
Ran the full verification pass on The lock mechanics themselves check out end to end: namespaced two-arg key, try-then-block with operator visibility, dedicated session connection that dies with the process, non-reentrancy guarded, release on all exit paths with connection close as backstop. I grepped the tree for lock-bypass vectors: only init and migrate invoke migrations and both are wrapped; serve never migrates. The init ordering holds too ( One substantive gap and two suggestions, inline below:
|
9442a65 to
f8625f0
Compare
|
Thanks, I followed the pg_locks suggestion through, but doing that exposed a flaw in the session lock version. Looks like acquiring a session lock and checking pg_locks in the next autocommit statement is not actually proxy-proof, the transaction pooling proxy can route both statements to the same arbitrary backend, make the check pass, and then route later work somewhere else. The release can also have the same problem in reverse. RDS Proxy is also different from PgBouncer here, it pins PostgreSQL sessions when the session level advisory lock functions are used, so naming it as unable to hold the lock wouldnt be accurate. I opted to change the mechanism instead, now the dedicated catalog connection opens an explicit transaction and takes pg_advisory_xact_lock for the whole migration interval. A transaction ooler has to retain one backend until that transaction ends, and PostgreSQL releases the lock automatically on rollback or connection loss. Release is a ROLLBACK, with connection close as the backstop. The pg_locks query remains, but only as an invariant check that the expected two integer, granted ExclusiveLock is present on the current backend. I also tightened the visibility part. The waiting line is explicitly flushed before the blocking lock call, and the contention test now reads and asserts that line while the external session still holds the lock, rather than only inspecting stdout after the process has completed. That makes the kubectl logs argument real rather than relying on how stdout happens to be buffered. The concurrency test no longer depends on two processes happening to reach the pending check at the same time. It now inserts the 002 ledger row in another uncommitted transaction. The first migrator cannot see that row during its pending check, so it applies 002 and then blocks deterministically when it tries to record the same filename. Only then does the test start the second migrator and assert that it stops at the advisory lock and emits the waiting message. Rolling back the blocker lets the first record and release; the second then acquires the lock, observes 002 as applied, and no-ops. With the migration lock removed, the second reaches the ledger barrier instead of printing the lock-wait message, so the test fails at the property it is intended to protect. On the per migration transaction gap, I'm feeling like hand rolling an outer transaction in this PR isnt quite the right fix. Three of the four migration files contain their own BEGIN and COMMIT. PostgreSQL does not create an inner transaction for that BEGIN, it warns that a transaction is already in progress, and the file’s COMMIT ends the one active transaction. Wrapping those files unchanged would therefore let their COMMIT end the wrapper before record_migration, putting the ledger write back outside it and giving us false atomicity. I removed the "safe" related wording as it was bit of a strong claim at this point. The actual recovery properties would be: catalog 001 normally avoids replay because it writes the expected catalog version, but the file itself is not idempotent; data 001 has the existing adoption guard; data 002 is repeatable against the expected schema; and replaying data 003 drops and recreates the token table, so it is not data-preserving. The absence of checksums in the current ledger is not a reason to call edits to shipped migrations safe, that is the class of problem ADR-0003 and #221 are intended to remove. Both apply sites now have a TODO(#221) stating that application and ledger recording are separate commits, spelling out those recovery limits, and requiring the sqlx adoption to remove the files’ internal transaction control and commit each migration with its ledger row before another migration lands. I used #221 as the implementation follow-up already in flight rather than opening a duplicate. The deterministic ledger barrier also gives the kill-mid-migration test a cleaner shape when #221 lands. Hold an uncommitted conflicting row in the migration ledger, let the migration reach and block on its ledger insert, kill the process there, then release the blocker and assert that both the schema work and ledger row rolled back. That targets the apply-versus-record boundary directly; an ACCESS EXCLUSIVE lock held from the beginning would also block the initial ledger read nd would not prove the same thing. Regarding the wait-timeout question, still feeling this PR shouldn't add a CLI timeout. A timeout converts ordinary contention into a failed or retried Job and loses the migrator’s place in the wait queue. Kubernetes can provide an explicit whole- ob bound with activeDeadlineSeconds when an operator wants one, backoffLimit controls retries rather than the duration of the current wait. If we later expose PostgreSQL lock_timeout, it needs to be applied to every migration connection to cover both the advisory-lock wait and blocking DDL, not just to the dedicated lock transaction. |
|
Nice work, and good call chasing the I verified the crash-release claim by hand rather than trusting it, using your own constants: mutual exclusion holds, killing the holder's backend with no rollback drops the granted count to zero, and the peer then acquires. Gates are clean on my side too (fmt, clippy Two things I want to credit because they are the easy ones to get wrong. The version read and pending check are inside the lock, not just the apply, which is what actually makes this correct. And the test uses an uncommitted Needs doing before merge: the branch has gone One ask: the wait is unbounded. After the try-lock fails you block on Offer: the crash-release property is the one thing the suite does not cover, and you are a step away. |
f8625f0 to
76d55f4
Compare
|
Thanks, addressed those comments:
|
Two replicas running `extenddb migrate` can both evaluate pending work before either records it, then apply the same migration concurrently. PostgreSQL can reject the duplicate DDL in its system catalogs, making an otherwise idempotent container startup unsafe. Serialize the complete migration decision and apply sequence with a namespaced PostgreSQL transaction-level advisory lock. The lock is held by an explicit transaction on a dedicated catalog connection, so it covers the version read and data-migration pending check, catalog and data migrations and final ledger observation. A peer acquires the lock only after the holder finishes, then observes no pending work. - Try `pg_try_advisory_xact_lock` first so a contending process can explain why it is waiting, then use `pg_advisory_xact_lock` for the blocking acquisition. - Keep the explicit transaction open for the full migration interval. Transaction-pooling proxies retain one backend until rollback. This avoids session locks split across pooled backend sessions. Direct PostgreSQL, RDS, Aurora, PgBouncer transaction pooling, and RDS Proxy follow the same transaction-scoped lock contract. - Bound the wait with a five-minute transaction-local `lock_timeout`. PostgreSQL SQLSTATE 55P03 becomes an actionable error naming the migration advisory lock and explaining that the holder may be wedged. - Flush waiting and acquired messages so container logs show progress while the process is still blocked rather than only after it exits. - Verify the granted row in `pg_locks`, including the two-integer key subtype, ExclusiveLock mode, and granted state. A key or lock-mode mismatch fails closed before migrations run. - Release by rolling back the dedicated transaction, then close the connection as a backstop. PostgreSQL also rolls the transaction back and releases it when the process or backend dies without cleanup. - Reject a second in-process acquire instead of deadlocking on a new transaction waiting for the lock already held by the first. - Scope the lock to the catalog database. Replicas sharing that catalog serialize even when migration statements touch the data database; deployments using different catalogs remain independent. - Lock init's schema phase too, so init and migrate cannot race. Two fresh init processes still do not queue. The second fails earlier while creating the already-existing catalog database. - Add default no-op lock methods to `Bootstrapper`. Existing external backends preserve current behavior until they opt into a backend lock. A MinimalBootstrapper test pins defaults and trait object safety. The advisory lock deliberately does not claim to make one migration atomic with its ledger write. The custom runner still applies SQL and records the filename in separate commits. Both apply sites carry a TODO for #221, which removes the migration files' internal BEGIN/COMMIT and lets sqlx commit each migration with its ledger row. Current recovery properties are narrower: catalog 001 is normally shielded by its version write, data 001 has an adoption guard, data 002 is repeatable, and replaying data 003 drops the token table. This gap must close before another migration relies on atomic recording. Add deterministic PostgreSQL CLI coverage: - Hold an uncommitted `schema_history` row so the first migrator commits 002's DDL and blocks exactly before its ledger insert. Start a second migrator and prove it waits. Release the barrier and verify both processes succeed, exactly one applies 002, and one ledger row exists. - Assert `gsi_pending` is committed while the 002 ledger row remains invisible, pinning the known apply-versus-ledger gap. - SIGKILL the holder without release, observe the waiting peer acquire the lock, then release the ledger barrier and verify recovery. This proves lock release on connection death. - Hold the lock from an external session and prove migrate explains the wait before the holder releases it, then completes successfully. Make subprocess reads unbuffered and cleanup exception-safe so tests cannot miss Python-buffered data or leak children after failure. Explicitly flush the Rust messages that the live tests observe. Integrate main, including container-readiness and SQLite changes. Resolve the CLI runner overlap as a union: lifecycle, container-readiness, and migration-concurrency files are excluded from the shared pytest run and executed together in the dedicated PostgreSQL CLI section. Verification on the final tree: - fmt and clippy `-D warnings` clean - release build clean - 669 workspace tests pass, with 3 ignored - 28 live PostgreSQL CLI tests pass, with 1 Unix-socket skip - all three migration-concurrency tests pass on PostgreSQL 16.10 - a one-second timeout mutation emits the exact diagnostic in 2.21s; the committed value is restored to five minutes and release rebuilt
76d55f4 to
d466d27
Compare
2 participants
What
Serializes concurrent
extenddb migrateruns with a namespaced PostgreSQL advisory lock, so replicas starting at the same time cannot race each other applying schema changes.migrateacquires the lock before checking what is pending and releases it on every exit path: success, the "everything is up to date" early return, and errors. The lock is held on a dedicated connection to the catalog database for the whole migration; if the process dies, PostgreSQL releases it when the connection closes.inittakes the same lock around its own schema work, so aninitcannot race amigrateon another replica. Two concurrentinitruns never reach the migrations — the second aborts earlier atcreate_catalog_dbbecause the database already exists — so this guards the narrower init-versus-migrate overlap.Bootstrappertrait gainsacquire_migration_lockandrelease_migration_lockwith default no-op bodies, so out-of-tree backends compile and behave exactly as they do today.Why
Two replicas running
migrateat once both evaluate which migrations are pending before either records anything, so both apply, and one will fail as a result PostgreSQL's system catalog rejecting concurrentCREATE TABLE IF NOT EXISTSfor the same table. It makes an idempotent container entrypoint, which unsafely runsmigrateon every start of every replicaTesting done
New
tests/test_cli_migrate_concurrency.py, against a real PostgreSQL:migrate --yesprocesses both exit 0, with exactly one printingApplying 002_gsi_pending.sqland the other reporting nothing to do. The migration lands exactly once. This asserts serialization directly rather than inferring it from a side effect.psycopg2session,migrateblocks (still running after 3s), then completes and reports that it waited once the lock is released. Contention is guaranteed rather than dependent on process timing.Both tests were confirmed to fail with the lock disabled — the race test on the duplicate
pg_type_typname_nsp_indexkey, the contention test on "migrate should block while the lock is held". A migration guard whose tests pass without the guard is worthless, so that is the main evidence here.A
MinimalBootstrapperunit test implements only the trait's required methods, so it stops compiling if a defaulted method ever loses its default — the same breakage an out-of-tree backend crate would hit — and pins object safety throughBox<dyn Bootstrapper>.devtools/run-testsexcludes the new file from the main pytest suite and runs it in the CLI section instead, alongsidetest_cli_lifecycle.py: like those tests it starts and stops its own servers and creates its own databases, so it cannot run in parallel against the shared instance the main suite uses.Decisions worth reviewer attention
The wait is unbounded.
pg_advisory_lockblocks until the lock is available, with no timeout. I think that is right: failing fast would make a normal multi-replica rollout flaky, which is the case this change exists to fix. The trade-off is that a wedged migrator blocks its peers indefinitely — in a Kubernetes init container, a pod that never starts. It is at least visible rather than silent, since a waiting migrator says so on stdout. If we want a bound instead, the options are a--lock-timeoutflag or a sessionlock_timeout.Why two trait methods rather than hiding the lock in the backend. Taking the lock inside the Postgres
run_catalog_migrations/run_data_migrationswould change no trait surface, but it does not fix the bug: the race is in the window between "which migrations are pending?" and "apply them", and those are separate trait calls made by the CLI, so a lock held only inside each apply call leaves the window open. A closure-takingwith_migration_lockavoids two methods but fights object safety underasync_trait. Note these are the first defaulted methods onBootstrapper.Deployment constraints, documented in the code:
transactionmode, RDS Proxy) cannot hold a session-level advisory lock at all, somigratemust talk to the database directly. This is the one I would most like a second opinion on, given RDS and Aurora are supported targets.Relationship to other work
sqlx::migrateadoption) has not landed, so the custom migration runner this serializes is still in place. If feat: adopt sqlx::migrate for PostgreSQL catalog and data migrations (ADR-0003) #221 lands first, this guard should be revisited —sqlx's migrator takes its own advisory lock, which may make part of this redundant. Fine to defer to that ordering if preferred.Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
Breaking changes
None for in-tree backends. The two new trait methods have default no-op bodies, so an out-of-tree
Bootstrapperimplementor compiles and behaves exactly as before — it simply gets no migration serialization, which is what it has today. TheMinimalBootstrappertest pins that guarantee.