Release the connection when the statement pool declines to retain a statement - #8
Open
webpatser wants to merge 1 commit into
Open
Release the connection when the statement pool declines to retain a statement#8webpatser wants to merge 1 commit into
webpatser wants to merge 1 commit into
Conversation
Author
|
The amphp/postgres follow-up mentioned above is up: amphp/postgres#74. It makes the DEALLOCATE that this change can trigger after a DISCARD ALL reset tolerant instead of wedging the connection. |
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
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.
A pool with
maxConnections: 1deadlocks on the secondprepare()+execute()while the first (fully consumed) result is still referenced. More generally, N live prepared-and-executed statements starve a pool of N connections.Repro
Against amphp/mysql v3.1.1 with sql-common v2.0.4 (same behavior on amphp/postgres):
Mechanism
SqlStatementPool::push()guards against retaining statements when the pool is saturated, exactly as its docblock intends. But the decline is a barereturn: it neither closes the statement nor drops the reference the release closure inexecute()holds. The declinedSqlPooledStatementtherefore stays alive for as long as the caller references the result object, its connection stays checked out, andSqlCommonConnectionPool::pop()waits on$awaitingConnection, which only that statement's destruction could complete.Holding a fully consumed result should not pin a connection; that is the same contract #2 established for plain query results.
Fix
push()now close the statement, so its connection returns to the pool immediately.execute()drops its statement reference after pushing, so the closure itself cannot keep a declined statement (and its connection) alive through a long-lived result object.The retain path is unchanged; a retained statement behaves exactly as before.
Tests
SqlStatementPoolTestpreviously stubbed the pool withoutgetConnectionLimit()/getConnectionCount()/getIdleConnectionCount(), so the retention guards always declined with limit 0 and the retention logic was never exercised. Added two cases: a saturated pool must close the declined statement and prepare a fresh one on the next execute (fails before this change), and a pool with capacity must retain and reuse the statement without closing it.Heads-up for amphp/postgres: with declined statements now being closed, a
DEALLOCATEcan be sent for a statement the server already dropped via the pool'sDISCARD ALLreset. I have a follow-up PR for that ready and will link it here.