Upgrade hasql-pool #2391

robx · 2022-07-27T10:30:55Z

Preparatory work for #2348, as discussed with @steve-chavez.

This loses the db-pool-timeout config option (and timing out of idle connections at all), and fixes #2401.

test/io/test_io.py

src/PostgREST/Workers.hs

src/PostgREST/AppState.hs

steve-chavez · 2022-08-05T18:04:53Z

src/PostgREST/Error.hs

+  toJSON SQL.PoolIsReleasedUsageError = JSON.object [
+    "code"    .= InternalErrorCode00,
+    "message" .= ("Use of released pool" :: Text),
+    "details" .= JSON.Null,
+    "hint"    .= JSON.Null]


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.

It's not expected to be visible to the user except during shutdown, so not temporary in that sense.

I see. In that case maybe we should a have more user-facing error message, like:

Suggested change

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]

robx · 2022-08-08T07:43:50Z

@steve-chavez I believe I've addressed the review comments, could you have another look?

robx · 2022-08-08T07:47:48Z

src/PostgREST/AppState.hs

+
+-- | 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'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.)

I've implemented this as a patch to hasql-pool here nikita-volkov/hasql-pool#16.

Just to be clear, does nikita-volkov/hasql-pool#16 prevent exceeding the configured pool size?

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).

steve-chavez · 2022-08-08T17:41:30Z

Q: With the new flushPool, do see any performance impact in comparison to the old releasePool?

robx · 2022-08-08T18:22:30Z

Q: With the new flushPool, do see any performance impact in comparison to the old releasePool?

I don't expect a meaningful performance impact for flushPool. The extra cost should be just looking up an IORef.

There's more likely to be performance regressions lurking in the hasql-pool rewrite, though the new version is simple enough that I don't expect anything there either. (The current hasql-pool version relies on an old resource-pool release, which uses STM just the same as the new hasql-pool, so I don't see any reason for new overhead as compared to the old version. Judging by the recent rewrite of resource-pool with non-STM for "better throughput and latency" there might be some room for improvement though.)

steve-chavez · 2022-08-24T00:38:06Z

@robx I think this should be good to merge. Or should we wait on closing nikita-volkov/hasql-pool#16 or #2422?

robx · 2022-08-24T07:06:46Z

@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.

…ehaviour

…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.

robx · 2022-08-29T15:09:46Z

Closing in favour of #2454.

robx commented Jul 27, 2022

View reviewed changes

test/io/test_io.py Show resolved Hide resolved

robx commented Jul 27, 2022

View reviewed changes

test/io/test_io.py Outdated Show resolved Hide resolved

steve-chavez reviewed Jul 27, 2022

View reviewed changes

src/PostgREST/Workers.hs Outdated Show resolved Hide resolved

robx mentioned this pull request Jul 29, 2022

tests: add tests for statement_timeout #2394

Merged

robx force-pushed the upgrade-pool branch 2 times, most recently from 3dae457 to 25fc9bd Compare July 29, 2022 12:00

This was referenced Aug 1, 2022

refactor code interacting with pool for consistency, and misc fixes #2398

Merged

Database connections can outlive SIGUSR1 reconfiguration #2401

Closed

robx force-pushed the upgrade-pool branch 4 times, most recently from b602bed to f64c0bd Compare August 4, 2022 14:13

robx marked this pull request as ready for review August 4, 2022 14:14

robx force-pushed the upgrade-pool branch from f64c0bd to f5766ca Compare August 4, 2022 14:21

robx commented Aug 4, 2022

View reviewed changes

src/PostgREST/AppState.hs Show resolved Hide resolved

robx commented Aug 4, 2022

View reviewed changes

src/PostgREST/AppState.hs Outdated Show resolved Hide resolved

robx force-pushed the upgrade-pool branch from 8ae478e to bce28bd Compare August 4, 2022 14:45

steve-chavez reviewed Aug 5, 2022

View reviewed changes

robx commented Aug 8, 2022

View reviewed changes

This was referenced Aug 10, 2022

Add pool stats to /metrics admin endpoint #2129

Closed

Add totalWaiters to PoolStats avanov/pool#3

Closed

robx mentioned this pull request Aug 11, 2022

RFC: allow forked dependencies, drop hackage release #2422

Closed

steve-chavez approved these changes Aug 24, 2022

View reviewed changes

robx added 2 commits August 24, 2022 09:21

refactor: rename releasePool to flushPool and document its intended b…

4b24599

…ehaviour

refactor: more accurate naming and documentation for connectionStatus

bcb9a1d

robx added 7 commits August 24, 2022 09:21

fix AppState field documentation

022e7ca

tests: update app settings test

057e326

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.

address linter complaints

b589495

comment that in-use connections aren't affected

67cbe5b

add test to validate that requests survive SIGUSR1

fcb3e5b

log flushed pool retry

e3726b0

robx force-pushed the upgrade-pool branch from 7479503 to e3726b0 Compare August 24, 2022 07:21

robx mentioned this pull request Aug 24, 2022

Upgrade hasql-pool to forked 0.7.2 #2444

Merged

robx added 4 commits August 24, 2022 09:43

bump hasql-pool

2f9c60e

unbumb hasql-pool

1623591

dontCheck hasql-pool

72f61ab

style

84f4b4a

robx closed this Aug 29, 2022

robx deleted the upgrade-pool branch August 29, 2022 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade hasql-pool #2391

Upgrade hasql-pool #2391

robx commented Jul 27, 2022 •

edited

Loading

steve-chavez Aug 5, 2022

robx Aug 8, 2022

steve-chavez Aug 8, 2022 •

edited

Loading

robx commented Aug 8, 2022

robx Aug 8, 2022 •

edited

Loading

robx Aug 8, 2022

steve-chavez Aug 8, 2022

robx Aug 8, 2022 •

edited

Loading

steve-chavez commented Aug 8, 2022

robx commented Aug 8, 2022

steve-chavez commented Aug 24, 2022

robx commented Aug 24, 2022

robx commented Aug 29, 2022

Upgrade hasql-pool #2391

Upgrade hasql-pool #2391

Conversation

robx commented Jul 27, 2022 • edited Loading

steve-chavez Aug 5, 2022

Choose a reason for hiding this comment

robx Aug 8, 2022

Choose a reason for hiding this comment

steve-chavez Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

robx commented Aug 8, 2022

robx Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

robx Aug 8, 2022

Choose a reason for hiding this comment

steve-chavez Aug 8, 2022

Choose a reason for hiding this comment

robx Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

steve-chavez commented Aug 8, 2022

robx commented Aug 8, 2022

steve-chavez commented Aug 24, 2022

robx commented Aug 24, 2022

robx commented Aug 29, 2022

robx commented Jul 27, 2022 •

edited

Loading

steve-chavez Aug 8, 2022 •

edited

Loading

robx Aug 8, 2022 •

edited

Loading

robx Aug 8, 2022 •

edited

Loading