Uh oh!
There was an error while loading. Please reload this page.
feat: adopt sqlx::migrate for PostgreSQL catalog and data migrations (ADR-0003) - #221
Open
yesyayen wants to merge 8 commits into
Open
feat: adopt sqlx::migrate for PostgreSQL catalog and data migrations (ADR-0003)#221yesyayen wants to merge 8 commits into
yesyayen wants to merge 8 commits into
Conversation
yesyayen
requested review from
LeeroyHannigan, amrith, c33howard, jcshepherd and pdf-amzn
as code ownersJuly 22, 2026 14:10
Replace the homegrown filename-tracked runner with sqlx's migrator for both the catalog and data databases (ADR-0003). Each database tracks applied migrations, with per-file checksums, in _sqlx_migrations; editing an applied migration is now a hard error instead of a silent no-op. - Enable the sqlx `migrate` feature. - run_catalog_migrations / run_data_migrations call sqlx::migrate!().run(). - Delete schema_history DDL, CATALOG_MIGRATIONS/DATA_MIGRATIONS, is_migration_applied, record_migration; drop BEGIN/COMMIT from migration files (sqlx wraps each in a txn). - Rehome the catalog_version write to a separate step after the catalog migrator runs. - migrate runs both migrators unconditionally so checksum validation always fires. - Bump CATALOG_VERSION to 0.1.0 (breaking: existing catalogs re-init). - Add .gitattributes pinning *.sql to LF; update the upgrade manual. Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
Add a tripwire pinning the embedded migration counts and CATALOG_VERSION (ADR-0003). Rewrite the two CLI lifecycle tests that queried the removed schema_history table to assert on sqlx's _sqlx_migrations ledger. Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
…migrations A stateless unit test pins each shipped migration's sqlx checksum, so `cargo test` (the PR runner, no database) fails if an already-applied migration file is edited. This catches in CI what sqlx otherwise only enforces at runtime against a live catalog. ADR-0003. Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
A catalog created by the old filename-tracked runner (has schema_history, no _sqlx_migrations) cannot be adopted in place: re-running 001 fails on a non-idempotent CREATE INDEX. Per ADR-0003 the upgrade path is destroy + init, so `migrate` now detects a pre-sqlx catalog up front and refuses with that directive instead of failing later on cryptic DDL. Aligns the admin guide with the upgrade manual, notes pre-1.0 semver, and restores the .sql suffix on the pending-migration report. Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
…ective Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
…s changelog Transactional migrations roll back completely on a mid-apply crash (no dirty row); re-running migrate retries. Dirty state only applies to -- no-transaction migrations, of which there are none. Also note Version History is the changelog. Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
…guard The ADR's operational note said a mid-apply crash leaves a dirty migration; for transactional migrations sqlx rolls back fully (no dirty row) and re-run retries. Also note why the pre-sqlx guard checks only the catalog. Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
After a refused migrate, _sqlx_migrations must not exist, proving the guard fired before sqlx could create its ledger. Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
8 tasks
8 tasks
robinnsc added a commit
that referenced
this pull request
Aug 4, 2026
…isory lock Two replicas running `extenddb migrate` at once race each other: both evaluate which migrations are pending before either records anything, both apply, and one fails with a duplicate pg_type_typname_nsp_index key when the concurrent CREATE TABLE IF NOT EXISTS statements collide in PostgreSQL's system catalog. That makes an idempotent container entrypoint, which runs migrate on every start of every replica, unsafe. Take a namespaced session-level advisory lock around the migration step so migrators serialize. The second blocks, then finds the schema already applied and no-ops. The lock is held on a dedicated connection to the catalog database for the duration and released on every path, including the "nothing to do" early return and error returns; if the process dies, PostgreSQL releases it when the connection closes. - A migrator that must wait tries the lock first and prints why it is waiting, instead of sitting silent for as long as the other migration takes. - Acquiring is not re-entrant: a second acquire would open a second connection and block on the lock the first holds, deadlocking against itself, so it is rejected. A failed unlock is reported rather than swallowed, though closing the connection releases the lock anyway so it is never fatal. - Advisory locks are scoped to a database, so migrators serialize only if they share a catalog database — they do, since it comes from the same connection string. - After acquiring, verify against pg_locks that this session holds the lock, and fail hard if it does not. A transaction-pooling proxy (pgbouncer in transaction mode, RDS Proxy) puts the lock on an arbitrary pooled backend, which otherwise leaves migrators unserialized while looking safe. One statement maps to one backend even through such a proxy, so the check detects it. Direct RDS and Aurora connections pass. - init takes the same lock around its own schema work, so it cannot race a migrate running on another replica. Two concurrent inits cannot reach the migrations at all: the second aborts earlier at create_catalog_db because the database already exists, so this guards the narrower init-versus-migrate overlap. - The Bootstrapper trait gains acquire_migration_lock and release_migration_lock with default no-ops, so out-of-tree backends compile unchanged. A new MinimalBootstrapper test implements only the required methods, so it stops compiling if a defaulted method loses its default, and pins object safety. - tests: add tests/test_cli_migrate_concurrency.py asserting that two concurrent `migrate --yes` runs both succeed with exactly one applying the migration and the other observing it as done, and that a migrate blocked on a lock held from an external session waits rather than proceeding, then completes and reports that it waited. Both tests fail when the lock is disabled. - devtools/run-tests: exclude the new file from the main pytest suite and run it in the CLI section instead, alongside test_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. Rebased onto the post-#218 layout: cmd_migrate and cmd_init now live in crates/app. The guard is unaffected by that move and by the registry removal; sqlx::migrate (#221) is not yet adopted, so the custom migration runner this serializes is still in place.
robinnsc added a commit
that referenced
this pull request
Aug 6, 2026
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
robinnsc added a commit
that referenced
this pull request
Aug 7, 2026
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
github-merge-queueBot
pushed a commit
that referenced
this pull request
Aug 7, 2026
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
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Replace the homegrown PostgreSQL migration runner with sqlx's built-in migrator
(
sqlx::migrate!) for both the catalog and data databases. sqlx checksums eachmigration file and refuses to run when an applied file changed.
extenddb migratenow runs both migrators; the
catalog_versionwrite moves to a separate stepafter the catalog migrator runs. Deletes the
schema_historytable and thehomegrown runner. Bumps
CATALOG_VERSION0.0.2 -> 0.1.0.Also adds a stateless unit test that pins each shipped migration's checksum, so
editing an already-shipped (immutable) migration
.sqlfails the CI test job,catching the edit at PR time, not just at runtime.
Why
The old runner tracked migrations by filename with no checksum, so editing an
already-shipped migration was a silent no-op that caused schema drift (the
incident behind ADR-0003). sqlx's
_sqlx_migrationstable closes that gap.Closes # n/a (tracked by ADR-0003)
Testing done
cargo build,cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings,cargo test --workspaceall green. Tripwire pins migration counts + CATALOG_VERSION.migration_checksums_are_pinnedpins each migration's SHA-384 checksum.Editing an already-shipped migration file changes its checksum and fails
cargo test --workspace(the GitHub PR test runner, no database required), so anedit to an immutable migration is caught in CI, not only at runtime. Verified by
editing a migration and observing the test flip to FAILED, then reverting.
idempotent migrate, version-gate rejection + migrate repair, editing an applied
migration -> loud checksum failure ("migration 2 was previously applied but has
been modified"), destroy + re-init.
_sqlx_migrationscreated in both databases;schema_historygone.Review follow-up
Addressed external review of the branch:
old runner (has
schema_history, no_sqlx_migrations) cannot be adopted in place:re-running
001fails on a non-idempotentCREATE INDEX. Per ADR-0003 the upgradepath is
destroy+init, somigratenow detects a pre-sqlx catalog up front andrefuses with that directive (verified: it bails before touching anything, version
untouched), instead of failing later on cryptic DDL. Added a CLI lifecycle test for it.
(
destroy+initfor the 0.1.0 sqlx adoption)..sqlsuffix on the pending-migration report; noted the checksum test'scoupling to sqlx's SHA-384 algorithm.
Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)ADR / RFC: docs/adr/0003-catalog-migration-mechanism.md
Breaking changes
Upgrading from a pre-sqlx catalog requires
destroy+init, which drops bothdatabases (all items wiped, not just schema). Acceptable at v0.1 with no dependent
catalogs.
CATALOG_VERSION0.0.2 -> 0.1.0. Documented in the upgrade manual.