Zero-downtime expand/contract migration for the tenant_id PK widening (#33)#64
Merged
Merged
Conversation
Restructure Migrate to run DDL outside a wrapping transaction under a session-level advisory lock (pg_advisory_lock on a dedicated connection), so CREATE UNIQUE INDEX CONCURRENTLY is usable and lock_timeout still makes a contended statement fail fast. The expand phase builds each composite (project_id, tenant_id, ...) key as a UNIQUE INDEX CONCURRENTLY, leaving the old narrow PK intact so old and new binaries interoperate during a rolling deploy. Tenant-scoped secondary partial indexes are likewise built CONCURRENTLY. A new Contract method (workspace migrate --contract) promotes those indexes to PRIMARY KEY USING INDEX and drops the old PK, as a deliberate out-of-band step after the fleet is on the new binary. Add an operational test that seeds a populated table and asserts the expand holds no sustained ACCESS EXCLUSIVE lock and does not block concurrent writes, plus expand idempotency and contract promotion, all in an isolated schema.
Describe workspace migrate (expand) and workspace migrate --contract, the auto-migrate guard for large databases, and the rolling-deploy ordering. Use a non-cancellable release context for the migration advisory unlock.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 513fd6b63f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| unlock := func() { | ||
| _, _ = conn.Exec(releaseCtx, "SELECT pg_advisory_unlock($1)", migrateAdvisoryLockKey) | ||
| } | ||
| if _, err := conn.Exec(ctx, "SET lock_timeout = '"+migrateLockTimeout+"'"); err != nil { |
There was a problem hiding this comment.
When auto-migrate runs on boot, lockAndPrepare sets lock_timeout with session scope on a connection acquired from the same pool used by request traffic, but the deferred cleanup only releases the advisory lock. After Migrate succeeds that connection is returned to the pool still capped at 5s, so any later repository call that happens to borrow it can fail while waiting on normal row/table locks even though migration is over. Previously SET LOCAL was scoped to the migration transaction; reset lock_timeout in the cleanup before releasing the connection.
Useful? React with 👍 / 👎.
Migrate/Contract now run on a short-lived pgx.Connect connection instead of borrowing from the request-serving pool, so the per-session lock_timeout can never leak onto a connection that later serves traffic; closing the dedicated connection releases the advisory lock and discards its session GUCs. Bound the advisory-lock acquisition: pg_advisory_lock is an unbounded wait that lock_timeout does not cover, so a stuck/long out-of-band migrator would hang every booting replica forever. Acquire via pg_try_advisory_lock polling against a 30s deadline and fail fast with a clear error instead. Replace the stale workspaces_personal_uniq index without a uniqueness gap: build the tenant-scoped index CONCURRENTLY under a temp name first, then drop the stale index and rename the temp in one short transaction. Emit a structured migrate_contract_pending WARN after expand for any table whose PK is not yet composite, so the expanded-but-not-contracted state is observable; silent on a fresh (born-composite) database.
Parse the migrate subcommand with the flag package so an unknown or typo'd flag errors with a non-zero exit instead of silently running expand; accept -contract/--contract. Log migrate_start/migrate_complete with an explicit phase=expand|contract. Wrap the auto-migrate-on-boot expand in a bounded context so a replica blocked on the migration lock fails readiness and reschedules rather than hanging on context.Background() forever; out-of-band migrate keeps an unbounded context. Pass the logger into the store so the contract-pending WARN is emitted.
Add tests on a populated isolated schema: expand-only steady state resolves upserts and same-id-different-tenant writes against the composite unique index while the PK stays narrow, and cross-tenant id reuse only succeeds after contract; the migrate_contract_pending WARN fires after expand on an upgrading DB and not on a fresh one; the migration lock_timeout does not leak onto pooled connections; and the advisory-lock acquisition fails fast when the lock is held. Document the dedicated-connection no-leak property, the bounded-lock failure mode, the phase logging, the contract-pending signal, the gap-free index swap, and that cross-tenant reuse activates only after contract.
Boot-path auto-migrate is now opt-in. A large existing DB's first deploy can no longer livelock on a bounded CONCURRENTLY expand build inside the boot window; migrations become a deliberate operator step. Small/dev DBs opt in explicitly.
Export postgres.ErrMigrationLockHeld so the boot path can classify it. A lock-held expand is transient and benign (another actor is migrating; an expanded-but-not-contracted schema serves fine), so the service starts without migrating and logs an alertable migrate_lock_contended instead of a hard exit. A boot-window deadline logs a distinct migrate_boot_timeout; any other schema/DDL error stays fatal.
…ting README: auto-migrate is opt-in (false by default); fresh/small and dev run workspace migrate explicitly or set GATEWAY_POSTGRES_AUTO_MIGRATE=true, the recommended prod path is out-of-band expand then migrate --contract. Replace the misleading fails-readiness wording with the start-without-migrating / hard-exit behavior. docker-compose sets GATEWAY_POSTGRES_AUTO_MIGRATE=true so docker compose up still creates the dev schema.
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
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.
Closes #33 — removes the ACCESS EXCLUSIVE availability hazard in the tenant_id PK widening (flagged by the #27 gate, deferred for pre-launch), before the tables grow large.
Migrate restructured: DDL now runs statement-by-statement on a dedicated pooled connection (not one wrapping tx), so
CREATE UNIQUE INDEX CONCURRENTLYis usable. Cross-replica serialization moved from tx-scopedpg_advisory_xact_lockto a session-levelpg_advisory_lock(released via WithoutCancel);lock_timeoutset per session for fail-fast. The in-placeALTER TABLE … ADD PRIMARY KEYDO-blocks are gone.Expand (boot/
workspace migrate, idempotent, safe on populated tables): for each of the 5 tables, if the PK doesn't yet include tenant_id, build the composite key asCREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTSand leave the old PK intact — no ACCESS EXCLUSIVE, old+new binaries interoperate (no 42P10). Fresh DBs get the composite PK from CREATE TABLE (expand is a no-op). The two tenant-scoped secondary indexes also moved to CONCURRENTLY. An INVALID leftover index from a failed concurrent build is dropped + rebuilt (IF NOT EXISTS alone wouldn't).Contract (separate, explicit
workspace migrate --contract, never on boot): promotes each composite index to PRIMARY KEY viaADD CONSTRAINT … PRIMARY KEY USING INDEX(short metadata lock, adopts the prebuilt index) and drops the old PK. Run after the whole fleet is on the new binary.Auto-migrate guard:
GATEWAY_POSTGRES_AUTO_MIGRATE=falselets a large-DB operator run expand + contract out of band. Two-phase runbook documented in README.Operational test (acceptance criterion): seeds 2000 rows, then during expand a second connection (a) fails on any sustained AccessExclusiveLock on the table in pg_locks and (b) asserts 50 concurrent INSERTs each complete <1s — proving writes aren't blocked. Plus idempotency (3× expand = no-op) and contract-promotes-to-PK. All existing postgres conformance stays green.
No proto change ·
go test -race ./...✓ (real Postgres) · lint 0 ✓.Closes #33