Upgrade hasql-pool #2391
Upgrade hasql-pool #2391
Conversation
robx
force-pushed
the
upgrade-pool
branch
2 times, most recently
from
3dae457
to
25fc9bd
Compare
robx
force-pushed
the
upgrade-pool
branch
4 times, most recently
from
b602bed
to
f64c0bd
Compare
| toJSON SQL.PoolIsReleasedUsageError = JSON.object [ | ||
| "code" .= InternalErrorCode00, | ||
| "message" .= ("Use of released pool" :: Text), | ||
| "details" .= JSON.Null, | ||
| "hint" .= JSON.Null] |
There was a problem hiding this comment.
Is it possible for the user to get this error? AIUI, on usePool we try to recover from PoolIsReleasedUsageError immediately.
If it's possible, perhaps this momentary error should include a "Retrying.." message and a 503 status to clarify it's temporary.
There was a problem hiding this comment.
It's not expected to be visible to the user except during shutdown, so not temporary in that sense.
There was a problem hiding this comment.
I see. In that case maybe we should a have more user-facing error message, like:
| toJSON SQL.PoolIsReleasedUsageError = JSON.object [ | |
| "code" .= InternalErrorCode00, | |
| "message" .= ("Use of released pool" :: Text), | |
| "details" .= JSON.Null, | |
| "hint" .= JSON.Null] | |
| toJSON SQL.PoolIsReleasedUsageError = JSON.object [ | |
| "code" .= InternalErrorCode00, | |
| "message" .= ("The database connection pool is released. Shutting down.." :: Text), | |
| "details" .= JSON.Null, | |
| "hint" .= JSON.Null] |
|
@steve-chavez I believe I've addressed the review comments, could you have another look? |
|
|
||
| -- | Flush the connection pool so that any future use of the pool will | ||
| -- use connections freshly established after this call. In-use | ||
| -- connections aren't affected, they just won't be reused anymore. |
There was a problem hiding this comment.
There's one issue with the way we replace pools, in that there's a timespan where two pools are around (the old one with the in-process requests, and the new empty one). Until all the old requests are done, it's possible to exceed the configured pool size.
I tend feel it's ok to accept this while fixing the issue this addresses otherwise, and keep it in mind as something to be fixed down the line, but I can't really judge that.
To address this properly would have to happen in the pooling library. (Since hasql-pool is pretty small and I'm a bit doubtful of getting our changes upstreamed anytime soon, I'd probably inline Hasql.Pool, fix this for us, and file PRs upstream independently.)
There was a problem hiding this comment.
I've implemented this as a patch to hasql-pool here nikita-volkov/hasql-pool#16.
There was a problem hiding this comment.
Just to be clear, does nikita-volkov/hasql-pool#16 prevent exceeding the configured pool size?
There was a problem hiding this comment.
Just to be clear, does nikita-volkov/hasql-pool#16 prevent exceeding the configured pool size?
Yes (well unless there's a bug in the PR).
|
Q: With the new |
I don't expect a meaningful performance impact for There's more likely to be performance regressions lurking in the |
|
@robx I think this should be good to merge. Or should we wait on closing nikita-volkov/hasql-pool#16 or #2422? |
I think it's time to fork (not just for this issue but those other hasql issues too). Here's a clone of nikita-volkov/hasql-pool#16: PostgREST/hasql-pool#1. |
…tgREST#2401) This version of hasql-pool is a simplified rewrite that doesn't use the resource-pool package. The major API changes are that idle connections are no longer timed out (and the corresponding setting is gone), and that `release` makes the pool unusable, where it used to remain usable and only flushed idle connections. - This change removes the db-pool-timeout option, since new hasql-pool doesn't provide timing out of idle connections. Given that we were typically running with very high timeout settings, I don't anticipate the lack of timeout to introduce new issues, though we might want to consider introducing some retry-logic down the line when we encounter connection failures. - To recover the ability to flush idle connections, we add a somewhat painful pool wrapper that allows replacing the pool. This fixes PostgREST#2401 by ensuring that flushing the pool also prevents any active connections from being reused in the future.
The original test no longer makes sense since connections don't timeout. To somehow test that new connections have the settings, convert it to flush the pool instead.
|
Closing in favour of #2454. |
Preparatory work for #2348, as discussed with @steve-chavez.
This loses the
db-pool-timeoutconfig option (and timing out of idle connections at all), and fixes #2401.