Upgrade hasql-pool to forked 0.7.2 #2444
Conversation
robx
force-pushed
the
upgrade-pool-fork
branch
3 times, most recently
from
da55577
to
cebef2f
Compare
|
I would like #2445 to be merged first, because I expect that there's something to be worked out between the plain cabal build and the nix build. (I.e., I'll want to add a And then this PR should wait for PostgREST/hasql-pool#2 to be merged, to ensure (a) that change is reviewed and (b) keep things somewhat clean, by only ever depending on main branch commits of the forked repo. |
robx
force-pushed
the
upgrade-pool-fork
branch
2 times, most recently
from
664f9f7
to
928fb1f
Compare
|
This should be good to go now. I was wondering whether I should write up some guidelines on dealing with forked dependencies such as this one (where? a new |
Yeah, I see new activity on |
The original test no longer makes sense once we drop pool timeouts with the hasql-pool upgrade. To somehow test that new connections have the settings, convert it to flush the pool instead.
…ehaviour Also fix documentation of AppState pool field.
robx
force-pushed
the
upgrade-pool-fork
branch
from
660d891
to
1829614
Compare
|
Chances are we'll merge nikita-volkov/hasql-pool#16 soon enough with a slightly simpler interface ( |
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. We depend on a PostgREST fork of 0.7.2 that gives us reliable flushing, compare PostgREST/hasql-pool#1 - hasql-pool 0.7 removes timing out of idle connections, so this change removes the db-pool-timeout option. 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. - See PostgREST#2422 for a discussion on depending on a forked dependency. Besides adding the dependency to the nix overlay, we're also adding it to stack.yaml and a new cabal.project to allow stack/cabal users to build the project.
robx
force-pushed
the
upgrade-pool-fork
branch
from
1829614
to
bf80771
Compare
Nice 🎉
Agree. |
|
Is this ideal to remove pool timeout? But if we remove this flag, then we will be wasting a lot of resources on the Postgres server per connection unnecessarily when there's minimal/no traffic. Does the upgraded |
Certainly not. It was removed upstream, though. So now, we would have to re-implement it ourselves. Not sure whether that's feasible. @robx @steve-chavez WDYT? |
Hm, yeah. Under this scenario the connection timeout seems ideal. Not sure how hard is to re-implement that in postgrest or hasql though. As a workaround, perhaps setting idle_session_timeout could work? |
If that works, that's much better than just a workaround. The question is, however, how hasql-pool deals with that connection loss. Will it reconnect immediately? Or would it only reconnect if the connection is required? |
|
@bhupixb Could you test the pool behavior with |
|
Some quick random ideas on this:
But if |
This will work IMO(haven't tested yet), but an RDS can have multiple applications running their DB on it, so setting the
@robx Can you please elaborate a little here, say my postgrest container is running and I added a sidecar to close idle connections. So what command should I send to postgrest for doing so? |
You can't really tell without trying. We'd expect the idle connections to be closed for sure - but what we don't know is whether PostgREST will restore all those idle connections immediately, or whether it will wait reconnecting until it needs them.
Just set it on the authenticator role only, to make it work for PostgREST only: ALTER ROLE authenticator SET idle_session_timeout = '...'; |
Here: https://postgrest.org/en/stable/schema_cache.html#schema-cache-reloading. A side-effect of schema-cache reloading is that connections are released. |
Just tried that, postgrest recovers but it gets noisy on the logs and the If the LISTEN channel is disabled( I guess the above is not a problem if you not set the |
I don't think setting this is good at all right now. I made a few tests with and later Only when the reconnection was successful, postgrest is responding regularly again. Surely, in that case we'd want PostgREST to just delay the request until the connection is up again? Seems to be related to #1932, too? |
I tend to agree. It seems reasonable (and mostly unavoidable -- we can't know if an old connection is still good) to automatically retry for certain error classes. It might make sense to do this in hasql-pool directly, not sure there. |
I think what should happen is:
Why is that? When the connection is terminated via |
You won't be able to tell that the connection is closed without writing to or reading from the socket, which needs to happen actively. It would be possible to preemptively try to read from the connection to see if the server terminated the connection abnormally (compare https://www.postgresql.org/docs/current/protocol-flow.html#id-1.10.6.7.11), but that's quite complicated with little fundamental benefit: Even if we have some reaper thread waiting for connections to terminate in order to remove them from the pool, there's a window where the server terminates the connection just after we took it from the pool, so for correct behaviour, we'd have to implement the retry anyway. The straightforward way (what feels like the "right" way to me), is to expect certain errors, such as a dead connection, and retry silently.
Yes, I think that's correct.
As noted above, only when reading from the socket. And then this still only covers certain active shutdown scenarios. It can also happen that network disruption causes a TCP socket to be in a state where we only see that the connection is broken once we write to it. So in general, no, the client doesn't realize this immediately. If it does something like (There's a number of connection settings that can play into this, e.g. around TCP keepalives, see https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-PARAMKEYWORDS.) |
This is a take on #2391 that uses our fork of hasql-pool with flushing support.