sqldb: reuse one migrate driver across migrations to avoid pool exhaustion - #11071
sqldb: reuse one migrate driver across migrations to avoid pool exhaustion#11071Vandit1604 wants to merge 2 commits into
Conversation
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟢 Low (2 files)
AnalysisThis PR modifies To override, add a |
Lrifton92
left a comment
There was a problem hiding this comment.
Verified the mechanism: with maxconnections=2 on a fresh DB, master leaks one sql.Conn per driver (GetSchemaVersion + first ExecuteMigrations) and executor.ExecTx blocks on BeginTx with no deadline, so the new test does time out on master and pass here. Sequential use only, and the driver's Lock/Unlock are balanced by Migrate.Migrate, so sharing it is safe. The SQLite driver keeps the *sql.DB directly, so leaving it alone is correct.
One behavioural point I think is worth fixing before merge, plus a few smaller ones:
- Steady-state regression. On master, a fully-migrated DB creates zero drivers (
GetDatabaseVersionsucceeds, the loop skips everything, so neitherGetSchemaVersionnorExecuteMigrationsis ever called). With this PRApplyAllMigrationsalways builds the driver, so every restart now pins one pool connection for the life of the process even when there is nothing to migrate. Two options: (a) create the driver lazily on first use inside the executor; or (b) open a dedicated*sql.DBfor the migrate driver (sql.Open("pgx", s.cfg.Dsn)) anddefer driver.Close()at the end ofApplyAllMigrations—pgx_migrate.Postgres.Close()closes both the conn and its own db, so nothing is reserved afterwards and nothing leaks paststore.Close(). (b) also removes the need for themaxconnections == 1guard. PostgresConfig.Validateis shared with the kvdb-postgres backend and withskipmigrations=true, wheremaxconnections=1does not hit this deadlock. If the guard stays, it belongs inApplyAllMigrationswhere the requirement actually exists.- Nit: "0 (unlimited)" is not accurate for the native store —
NewPostgresStoremaps 0 todefaultMaxConns(25). - Nit:
TestPostgresMigrationSmallConnPoolnever closesstore.
Follow-up (not for this PR): sqldb/v2/postgres.go has the same per-call driver pattern.
| // that the store keeps using. The driver therefore holds one pool | ||
| // connection for the lifetime of the store. That is a deliberate | ||
| // trade-off: one reserved connection instead of one per migration step. | ||
| driver, err := pgx_migrate.WithInstance(s.DB, &pgx_migrate.Config{}) |
There was a problem hiding this comment.
On master a fully-migrated DB creates no driver at all (GetDatabaseVersion hits, the loop skips). This now reserves one pool connection on every startup, migration or not. Either create the driver lazily on first use, or give it its own *sql.DB and defer driver.Close() here — pgx_migrate.Postgres.Close() closes the conn and the db it was built on, so with a private db that is exactly what we want and nothing stays reserved.
| // driver and needs a second for the migration-tracker transaction, so | ||
| // a pool capped at one connection deadlocks on startup (see #11007). | ||
| // Zero means unlimited, which is fine. | ||
| if p.MaxConnections == 1 { |
There was a problem hiding this comment.
This config also drives kvdb-postgres and the skipmigrations path, where maxconnections=1 was accepted and doesn't deadlock. Consider checking this in ApplyAllMigrations instead. Also "0 (unlimited)" isn't right for the native store: NewPostgresStore turns 0 into defaultMaxConns (25).
| cfg := fixture.GetConfig(dbName) | ||
| cfg.MaxConnections = 2 | ||
|
|
||
| store, err := NewPostgresStore(cfg) |
There was a problem hiding this comment.
t.Cleanup(func() { require.NoError(t, store.DB.Close()) }) to match NewTestPostgresDB.
ed6eba7 to
29d08c0
Compare
|
Thanks for the careful review, and good catch on the steady-state regression. You're right that a fully-migrated DB built no driver before, and my version pinned a connection on every restart. Went with your option (b): the driver now runs on its own Left |
On a fresh Postgres database, applying the startup migrations could hang when db.postgres.maxconnections was small. Each migration step built a new golang-migrate driver, and every driver reserved a pool connection for its lifetime that was never released. With around 18 migrations a small pool was exhausted mid-startup, and the migration-tracker transaction then blocked forever with no timeout or error. ApplyAllMigrations now builds a single migrate driver on its own dedicated database handle and reuses it for every step, closing it once migrations finish. Its Close closes both the reserved connection and the handle, so nothing stays reserved on the store's pool afterwards, even when there is nothing to migrate. Add a regression test that applies all migrations with a two-connection pool, which deadlocks before this change and passes after.
Document the Postgres migration connection-reuse fix under the Database section, and add the author to the contributors list.
29d08c0 to
2af1cb0
Compare
Fixes #11007.
Problem
On a fresh Postgres database,
lndhangs during startup migrations whendb.postgres.maxconnectionsis small. Each migration step creates a newgolang-migratedriver withpgx_migrate.WithInstance, and that driver checks out one connection from the pool and holds it until it is closed. The drivers are never closed, so every migration keeps another connection for the rest of startup. There are 18 migrations, so a fresh database can need around 20 connections. Once the pool is used up, the migration-tracker transaction waits for a connection that never frees, with no timeout or error, and startup never finishes.@ziggie1984 diagnosed this in #11007 and already wrote a fix in #10426 (commit 214af56). That PR also changes the default connection count and has been open a while, so this is the connection-reuse part on its own, plus the regression test asked for there.
Fix
ApplyAllMigrationsnow creates one migrate driver and reuses it for every step, so all migrations share a single connection instead of one each. The driver is not closed, because closing it would close the shared*sql.DBthe store keeps using, so it holds one connection for the life of the store. That is the trade-off for not exhausting the pool.With the fix a migration run needs two connections: one for the shared driver and one for the migration-tracker transaction. So
maxconnections=1would still deadlock.PostgresConfig.Validatenow rejectsmaxconnections=1(0 still means unlimited), so that fails at startup with a clear message instead of hanging.The per-call
ExecuteMigrations/GetSchemaVersion/SetSchemaVersionmethods are kept because the test harness uses them, with a note not to call them in a loop.I did not touch the sqlite path. Its
WithInstancekeeps the*sql.DBdirectly and its lock is an in-process flag, so it never holds a connection open and is not affected by this bug.Testing
TestPostgresMigrationSmallConnPoolapplies all migrations withmaxconnections=2under a timeout. It deadlocks (times out) onmasterand passes with this change. The fullsqldbpackage passes.