diff --git a/docs/release-notes/release-notes-0.22.0.md b/docs/release-notes/release-notes-0.22.0.md index 750a20069cd..d162dde7ce1 100644 --- a/docs/release-notes/release-notes-0.22.0.md +++ b/docs/release-notes/release-notes-0.22.0.md @@ -138,6 +138,12 @@ ## Database +* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/11071) where a fresh + Postgres database could hang during startup migrations when a small + `db.postgres.maxconnections` was set. Each migration reserved a pool connection + that was never released, exhausting a small pool mid-startup. Migrations now + reuse a single connection. + ## Code Health ## Tooling and Documentation @@ -159,3 +165,4 @@ * Boris Nagaev * Erick Cestari * Jared Tobin +* Vandit Singh diff --git a/sqldb/migrations_test.go b/sqldb/migrations_test.go index 4e2b68e0ffa..3038f9d3b86 100644 --- a/sqldb/migrations_test.go +++ b/sqldb/migrations_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/golang-migrate/migrate/v4" "github.com/golang-migrate/migrate/v4/database" @@ -723,3 +724,52 @@ func TestMigrationConfigConsistency(t *testing.T) { } } + +// TestPostgresMigrationSmallConnPool is a regression test for +// https://github.com/lightningnetwork/lnd/issues/11007. Applying all migrations +// to a fresh Postgres database with a connection pool smaller than the number +// of migrations must not deadlock. Previously each migration created its own +// migrate driver, and every driver reserved a dedicated pool connection that +// was never released, so a small pool was exhausted mid-startup and the +// migration-tracker transaction blocked forever with no timeout or error. +func TestPostgresMigrationSmallConnPool(t *testing.T) { + t.Parallel() + + ctxb := t.Context() + + fixture := NewTestPgFixture(t, DefaultPostgresFixtureLifetime) + t.Cleanup(func() { + fixture.TearDown(t) + }) + + dbName := randomDBName(t) + _, err := fixture.db.ExecContext(ctxb, "CREATE DATABASE "+dbName) + require.NoError(t, err) + + // Cap the client pool well below the number of migrations so the old + // per-migration driver leak would exhaust it before the run completes. + cfg := fixture.GetConfig(dbName) + cfg.MaxConnections = 2 + + store, err := NewPostgresStore(cfg) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, store.DB.Close()) + }) + + // ApplyAllMigrations blocks forever when the pool is exhausted, so run + // it under a deadline instead of hanging the whole test binary. + done := make(chan error, 1) + go func() { + done <- store.ApplyAllMigrations(ctxb, GetMigrations()) + }() + + select { + case err := <-done: + require.NoError(t, err) + + case <-time.After(time.Minute): + t.Fatal("migrations deadlocked with a small connection pool " + + "(issue #11007)") + } +} diff --git a/sqldb/postgres.go b/sqldb/postgres.go index 70dba82a1a0..2eb44d4b885 100644 --- a/sqldb/postgres.go +++ b/sqldb/postgres.go @@ -9,6 +9,7 @@ import ( "strings" "time" + "github.com/golang-migrate/migrate/v4/database" pgx_migrate "github.com/golang-migrate/migrate/v4/database/pgx/v5" _ "github.com/golang-migrate/migrate/v4/source/file" // Read migrations from files. // nolint:ll _ "github.com/jackc/pgx/v5" @@ -158,15 +159,93 @@ func (s *PostgresStore) ApplyAllMigrations(ctx context.Context, return nil } - return ApplyMigrations(ctx, s.BaseDB, s, migrations) + dbName, err := getDatabaseNameFromDSN(s.cfg.Dsn) + if err != nil { + return err + } + + // Reuse a single migrate driver for every migration step. Creating + // one per step (as the per-call methods below do) reserves a pool + // connection each, so a small pool is exhausted and the + // migration-tracker transaction deadlocks with no timeout or error. + // See https://github.com/lightningnetwork/lnd/issues/11007. + // + // The driver runs on its own database handle, not the store's pool, and + // is closed once migrations finish. Its Close closes both the reserved + // connection and this handle, so nothing stays reserved on the store's + // pool afterwards, not even when there is nothing to migrate. + migrateDB, err := sql.Open("pgx", s.cfg.Dsn) + if err != nil { + return err + } + + driver, err := pgx_migrate.WithInstance( + migrateDB, &pgx_migrate.Config{}, + ) + if err != nil { + if cerr := migrateDB.Close(); cerr != nil { + log.Errorf("Error closing migration db: %v", cerr) + } + + return errPostgresMigration(err) + } + defer func() { + if cerr := driver.Close(); cerr != nil { + log.Errorf("Error closing migration driver: %v", cerr) + } + }() + + executor := &postgresMigrationExecutor{ + driver: driver, + dbName: dbName, + } + + return ApplyMigrations(ctx, s.BaseDB, executor, migrations) } func errPostgresMigration(err error) error { return fmt.Errorf("error creating postgres migration: %w", err) } +// postgresMigrationExecutor implements MigrationExecutor with a single reusable +// migrate driver, so all migration steps share one pool connection instead of +// each reserving (and leaking) its own. +type postgresMigrationExecutor struct { + driver database.Driver + dbName string +} + +var _ MigrationExecutor = (*postgresMigrationExecutor)(nil) + +// ExecuteMigrations runs the migrations up to the given target using the shared +// driver. +func (p *postgresMigrationExecutor) ExecuteMigrations( + target MigrationTarget) error { + + postgresFS := newReplacerFS(sqlSchemas, postgresSchemaReplacements) + return applyMigrations( + postgresFS, p.driver, "sqlc/migrations", p.dbName, target, + ) +} + +// GetSchemaVersion returns the current schema version using the shared driver. +func (p *postgresMigrationExecutor) GetSchemaVersion() (int, bool, error) { + return p.driver.Version() +} + +// SetSchemaVersion sets the schema version using the shared driver. +func (p *postgresMigrationExecutor) SetSchemaVersion(version int, + dirty bool) error { + + return p.driver.SetVersion(version, dirty) +} + // ExecuteMigrations runs migrations for the Postgres database, depending on the // target given, either all migrations or up to a given version. +// +// NOTE: This builds a fresh driver that holds a pool connection until the store +// is closed. Do not call it in a loop across many schema versions, or the pool +// is exhausted; ApplyAllMigrations reuses a single driver for that. See #11007. func (s *PostgresStore) ExecuteMigrations(target MigrationTarget) error { dbName, err := getDatabaseNameFromDSN(s.cfg.Dsn) if err != nil {