Configurable idle timeout - #1098

Ideally this would be NominalDiffTime. However, NominalDiffTime lacks a Read instance, and PostgresConf derives Read. I requested a Read instance here: haskell/time#130

These two functions are kind of annoying, naming wise. Something like createPostgresqlPoolModifiedWithVersionWithConf would be gross (createPostgresqlPoolModifiedWithVersion is pretty bad on its own tbh). I'm also somewhat worried that another callback will eventually be added here (Perhaps a callback for when a connection is destroyed, or some other Postgres specific thing?)

Possibly a separate data type is called for to hold these functions (PostgresConfCallbacks isn't quite right, but something like that), with a function to create a default data structure?

Hooks is another term that gets used for this concept a bit -

data PostgresConfHooks = PostgresConfHooks
  { getServerVersion :: IO (Maybe Double)
  , afterCreate :: IO ()
  }

createPostgresqlPoolWithHooks :: (PG.Connection -> PostgresConfHooks) -> ...

So, I was investigating this today and I was thinking this could be an opportunity to clean up the server version API. I have these complaints:

1. Double is a weird way to represent a version. It doesn't work for 3 digit versions (x.y.z) (though Postgres moved to just 2 digit versions recently which helps). I think something like NonEmpty Integer makes more sense, e.g. [10, 2]

2. The Maybe behavior feels weird to me. If you return Nothing, Persistent assumes the lowest possible version, not trying to do things like upsert. This isn't documented afaict, and would subtly degrade behavior if someone happened to return Nothing.

As long as we're adding a new API, I would propose something like:

getServerVersion :: IO (NonEmpty Integer)

In the case of Redshift users who can't get the server version (assuming that's still a thing), they can return a hard-coded value that matches whatever Postgres capabilities Redshift has, e.g. pure (10 :| [2]).

If there was something better suited than NonEmpty Integer I'd be fine with that too.

Implementation would be something like:

getServerVersion2 :: PG.Connection -> IO (NonEmpty Word)
getServerVersion2 conn = do
  [PG.Only version] <- PG.query_ conn "show server_version";
  case AT.parseOnly parseVersion (T.pack version) of
    Left err -> throwIO $ PostgresServerVersionError $ "Failed to parse Postgres version. Got: " <> version <> ". Error: " <> err
    Right versionComponents -> case NEL.nonEmpty versionComponents of
      Nothing -> throwIO $ PostgresServerVersionError $ "Empty Postgres version string. Got: " <> version
      Just neVersion -> pure neVersion

  where
    -- Partially copied from the `versions` package
    parseVersion = AT.decimal `AT.sepBy` AT.char '.' <* AT.endOfInput

Sounds great to me

Adding new fields could be considered a breaking change for persistent-postgresql

If they're exported, yeah, it's technically a breaking change.

I think having a separate data type for the connection pool configuration is nicer than 3 separate function parameters. Thoughts?

In an ideal world for me, there is a field of PostgresConf named poolConfig :: ConnectionPoolConfig. Unfortunately, PostgresConf already has an existing field for the pool size, so I broke out stripes + idle timeout to separate fields there.

But if adding the two new fields is a breaking change, possibly we could go all-out and replace the fields with a ConnectionPoolConfig field?

there are breaking changes, and then there is Breaking Changes, and, weeeeeell, I'd rather the easiest breaking change as possible. Making these types more abstract is a goal of #920 so we can leave a TODO here to clean it up with persistent-3.0 possibly.

Ok cool, that's helpful guidance 👍

I've tried to keep the order of stripes + idle timeout + pool size consistent, and in the same order as they are in resource-pool. I think it's somewhat unfortunate that it's easy to mix up stripes and pool size though.