Repository navigation
Conversation
hieblmi
left a comment
hieblmi
left a comment
There was a problem hiding this comment.
Found one migration lock-timeout regression. The regular and Postgres loopdb test suites pass; a focused contention test reproduces the issue described inline.
| return err | ||
| } | ||
| defer func() { | ||
| _ = conn.Close() |
There was a problem hiding this comment.
[P2] Preserve the migration lock timeout during connection cleanup
If another migrator acquires the advisory lock between WithConnection's ensureVersionTable check and Up(), the migration library returns ErrLockTimeout after its default 15 seconds while its background pg_advisory_lock query remains blocked. sql.Conn.Close() waits for in-flight queries, so this new deferred close prevents the timeout from reaching the caller until the competing lock is released, potentially hanging startup indefinitely.
I reproduced this by acquiring the competing advisory lock when the migration filesystem is opened, after ensureVersionTable has released it. With the library timeout shortened to 500 ms, the previous WithInstance path returned ErrLockTimeout after 501 ms; this helper was still blocked after 1.512 seconds and returned only after I explicitly released the competing lock.
Please ensure the pending advisory-lock query is bounded or cancelled before synchronously returning the connection to the pool, and add a contention regression test.
6c7fa05 to
2aaf7e6
Compare
|
@hieblmi: review reminder |
postgres_migrate.WithInstance checks out a dedicated connection that only the driver's Close method gives back, and Close also closes the database that the store keeps using. The store therefore never returned that connection to the pool. That's harmless while the pool is unbounded, but it would permanently take a slot from a bounded one. Run the migrations on a connection obtained from the pool and return it once they are done. The driver only holds session-level advisory locks while it migrates, so the connection can be reused afterwards. If the migrations give up waiting for the migration lock, their request for the lock stays pending on the connection. Closing the connection would wait for that request, so the lock timeout would only reach the caller once the competing migrator releases the lock. Hand the connection back in the background in that case, once the request is done and the lock it may have got is released.
NewPostgresStore never applied PostgresConfig.MaxOpenConnections, the postgres.maxconnections option, so the connection pool was unbounded and concurrent queries opened as many connections as they needed. The static address loop-in manager handles up to 128 sweep requests at once, and a burst of co-signing requests exceeded the 100 connections that a default Postgres server accepts. The requests failed with "sorry, too many clients already". Limit the pool to the configured number of connections, or to 25 if the option is unset, the same default that lnd and tapd use. No transaction waits for a second connection while it holds one, and the migrations no longer keep a connection checked out, so the limit can't deadlock the store, even with a single connection.
Open a Postgres store with the default limit, a configured limit and a single connection, and check the limit of its connection pool and that no connection is still checked out afterwards. Without the limit, the pool is unbounded. Without handing the migration connection back, one connection stays in use, and a single connection isn't enough to open the store. Also let the migrations time out on the migration lock while a competing migrator holds it, and check that they report the timeout while the lock is still held and hand their connection back without the lock once the competing migrator releases it. Closing the connection right away delays the timeout until the competing migrator releases the lock, and a connection handed back with the lock keeps blocking other migrators.
2aaf7e6 to
f0814ce
Compare
The
postgres.maxconnectionsoption (PostgresConfig.MaxOpenConnections) has been a no-op since the Postgres store was added:NewPostgresStorenever applied it, so the connection pool was unbounded and concurrent queries opened as many connections as they needed.This matters for the static address loop-in manager, which handles up to 128 sweep requests concurrently (
maxConcurrentSweepRequests). A burst of co-signing requests can open more connections than the Postgres server accepts (100 by default). This showed up in the Loop server's integration tests with a Postgres-backed client: sweep requests failed withFATAL: sorry, too many clients already (SQLSTATE 53300), the server's co-signing requests timed out, and its sweep batcher had to split the swaps into separate batches.Changes
WithInstancechecks out a dedicated connection that only the driver'sClosereturns, andClosealso closes the store's database, so the store never gave that connection back. Migrations now run on a connection taken from the pool (WithConnection), which is returned once they are done. The driver only holds session-level advisory locks while migrating, so the connection is safe to reuse.NewPostgresStorenow callsSetMaxOpenConnswith the configured value, or 25 if the option is unset, the same default as lnd and tapd. The flag description,sample-loopd.confand the release notes are updated.TestPostgresConnectionPool(built with thetest_db_postgrestag, so it runs in the Postgres unit test CI job) opens a store with the default limit, a configured limit and a single connection, and checks the pool's limit and that no connection is still checked out afterwards.Notes
FixFaultyTimestampsdrains its rows before it opens its transaction, and migrations no longer keep a connection checked out. A single connection is enough to open the store, which the test covers.Pull Request Checklist
docs/release-notes/release-notes-next.md, or apply theno-changeloglabel (required by CI)