Add support for flushing the pool

[robx]

"Flushing the pool" is close to what "releasing the pool" did in versions < 0.6, except it also ensures that in-flight connections don't get returned to the pool.

Does this seem like a useful feature? I fully understand if you'd prefer to keep the library simpler.

For context, I'm trying to upgrade PostgREST from hasql-pool 0.5, and PostgREST relies (brokenly) on being able to flush the pool via release. That's because it sets some per-connection variables, and needs to be able to update those values for new requests. (I make no claims as to whether that's a particularly sensible approach.)

This isn't necessarily the simplest/cleanest way to do it. An alternative I'm aware of would be to track e.g. a poolGeneration :: TVar Int and compare the acquisition generation to the current generation to determine whether to return a connection to the pool. That has the theoretical downside that Int is bounded...

[nikita-volkov]

Sorry for the delayed response.

Have you considered leaving the pool package as it is, but wrap it in something like the following:

data FlushablePool =
  FlushablePool
    {-| Pool size. -}
    Int
    {-| Connection settings. -}
    Hasql.Connection.Settings
    {-| Pool var. -}
    (MVar Hasql.Pool.Pool)

flush :: FlushablePool -> IO ()
flush (FlushablePool size settings var) = do
  pool <- takeMVar var
  Hasql.Pool.release pool
  newPool <- Hasql.Pool.acquire size settings
  putMVar var newPool

[robx]

Have you considered leaving the pool package as it is, but wrap it in something like the following:

Yes, I went for a similar approach (using an IORef) first here: https://github.com/PostgREST/postgrest/pull/2391/files#r939930348

But the approach is fundamentally flawed in that the effective connection limit becomes (number of pools) x (pool size). Given that postgresql connections are a pretty scarce resource, that doesn't really feel like a good solution.

EDIT: To be a bit more explicit: If the connection pool is fully loaded, and there are some long running queries, we'd exceed the limit on the connection count while any of the long running queries prior to releasing the pool are in flight.

Congrats on the pgenie release btw! (And I understand you've been busy)

[nikita-volkov]

But the approach is fundamentally flawed in that the effective connection limit becomes (number of pools) x (pool size). Given that postgresql connections are a pretty scarce resource, that doesn't really feel like a good solution.

Makes sense. Give me some time to review the PR.

Congrats on the pgenie release btw! (And I understand you've been busy)

Thanks :)

[robx]

Oh I should add that there's a follow-up change I want to make to allow timing out connection acquisition (I've filed it here PostgREST#4 for now, didn't want to keep piling on.)

I'd understand in general if you want to keep this package more bare-bones, particularly considering its recent simplification. It would be ok (even if not ideal) for us to "properly" fork as say postgrest-hasql-pool for a more fully featured (and likely less well considered) hasql pool library.

(Just mentioning this to warn you that this might not be the end of it, since that information might factor into deciding whether you want to merge this individual change.)

[nikita-volkov]

I'm okay with merging the so far discussed updates to the library. I see no point in breeding libraries when we agree on the direction.

Also since clearly you have a popular and actively maintained use-case if you wish I can add you as a co-maintainer. Let's just agree to keep initiating discussions like the one we're having now before making design changes.

Back to the PR. Here's a thought: why don't we make release be flush? Namely, make it so that using connections after releasing will guarantee that the connection will be fresh and won't fail with PoolIsReleasedUsageError (IOW, get rid of that error type altogether). If the pool is never used after releasing, it effectively means that all associated resources get released.

Regarding the timeout. Give a try to the 0.6 version. It was a major version that fixed the bugs and got rid of the resource-pool dependency, yet it retained the timeout functionality. There were no issues detected with that version, it just lacked testing. It was lingering as an unreleased branch for some time and then I forgot about it and accidentally wrote 0.7. So I released both giving the preference to the latest developments. 😅 Yeah. I know.

Any way, please give it some testing. Or even better, PR with some tests for it, if you will. If we discover that 0.6 is reliable, I'll be happy to use it as the base for future developments and extend it with flushing in whatever form we choose.

[robx]

Back to the PR. Here's a thought: why don't we make release be flush? Namely, make it so that using connections after releasing will guarantee that the connection will be fresh and won't fail with PoolIsReleasedUsageError (IOW, get rid of that error type altogether). If the pool is never used after releasing, it effectively means that all associated resources get released.

Yes that sounds great, somehow I took it as a given that release should keep working that way. I pushed a change to that effect.

Regarding the timeout. Give a try to the 0.6 version. It was a major version that fixed the bugs and got rid of the resource-pool dependency, yet it retained the timeout functionality.

I think it's a different kind of timeout: We'd like to be able to bail out of waiting for a connection to be made available by the pool, while to my understanding the old timeout behaviour was about timing out idle connections. I'll file my change for that so we can discuss it there.

[robx]

I'm at a bit of a loss how to name this well. I went for very explicit naming for the purposes of making the PR understandable for now, hopefully we can find something better?

[nikita-volkov]

Alright! I've merged all the changes and released it as 0.8. Nice work and thanks for your contributions!

[robx]

Whoa! Thanks