To get access to the underlying backend, use the RawPostgresql type

This isn't sufficient for instrumentation of existing codebases unfortunately. The value lets us wrap existing connections without requiring pervasive downstream changes.

This doesn't feel right to me. My understanding of this behavior is:

1. We create this Vault.Key PG.Connection lazily, whenever the first time we call this function. Then the value is cached due to GHC's applicative-normal-form optimization. So, we generate exactly one Key Connection here, and it is a global constant.
2. When we call createBackend or getSimpleConn (whichever one comes first) then we create the underlyingConnectionKey. If we call getSimpleConn first, then we'll get Nothing in that function. If we call createBackend afterwards, then the Key Conn already exists, and we stuff the Conn into the Vault.
3. So, I think the problem with this, is that we are overwriting the underlying Conn with every call to createBackend. Is this a problem?

withPostgresqlPool delegates to withSqlPool open' which itself calls return $ constructor (createBackend logFunc ver smap) conn. Which means that, on every new SqlBackend that is created, we end up overwriting the value here, which means that getSimpleConn is going to get the most recent PG.Conn that has been created.

OK, so this is probably not an issue. While we do generate a single Key Conn, this is used to index into the SqlBackend's internal Vault, not some globally available Vault. And, since there's only ever exactly one, then we can easily recover the PG.Connection for a given SqlBackend.

This is a bit gnarly but it shouldn't have any problems.

Points 1 and 2 are intentional. We need a stable key in order to be able to reference the connection from external instrumentation tooling. AFAICT, there's no way you could call getSimpleConn prior to constructing a backend via createBackend (at least without digging into internals), so you're guaranteed by virtue of the order of operations to have the key be initialized and able to return the underlying PostgreSQL connection. Worst case scenario, you can't prove that the connection is to a PostgreSQL database and don't do enhanced instrumentation.

Regarding 3, I don't see how we're overwriting the underlying call in createBackend, since it's just an immutable function that constructs a backend from its constituent parts?

This is nice!

To make backwards compatibility easier, I'd want to hide this constructor/accessors in an .Internal module, and expose an API for setting/modifying/adding hooks. Eg:

module Database.Persist.SqlBackend.SqlPoolHooks 
  ( SqlPoolHooks
  , defaultSqlPoolHooks
  , setAlterBackend
  , modifyAlterBackend
  , addAlterBackendHook
  , addRunBeforeHook
  , addRunAfterHook
  -- etc..
  )
  where

module Database.Persist.SqlBackend.SqlPoolHooks.Internal where

data SqlPoolHooks m backend = SqlPoolHooks
    { alterBackend :: backend -> m backend
    -- ^ Alter the backend prior to executing any actions with it.
    , runBefore :: backend -> Maybe IsolationLevel -> m ()
    -- ^ Run this action immediately before the action is performed.
    , runAfter :: backend -> Maybe IsolationLevel -> m ()
    -- ^ Run this action immediately after the action is completed.
    , runOnException :: backend -> Maybe IsolationLevel -> UE.SomeException -> m ()
    -- ^ This action is performed when an exception is received. The
    -- exception is provided as a convenience - it is rethrown once this
    -- cleanup function is complete.
    }

really would like to minimize additions to the public API that require breaking changes to modify (eg data constructor direct exposure)

Hmm. We may be able to avoid making this a breaking change, if there's a way to go from IORef (Map Text Statement) to a StatementCache in the function that translates a MkSqlBackendArgs into a SqlBackend.

OK, yeah, this can definitely be a minor version bump! Which means there's no urgency to figure out the other breaking changes and try to bundle them together. I think this is the only breaking change in the PR.

I want to mention that I'd like to ensure that alternative statement caches can be provided too. In particular, the end goal here is that we could for example provide counters of the statement cache to detect memory leaks (real situation that can occur for long-lived postgresql connections currently AFAICT). Or as another example, we might want to provide an LRU version of the statement cache that can evict. That's why this was introduced as a breaking change. That could technically be deferred til a later date.

You should be able to write setConnStatementCache :: StatementCache -> SqlBackend -> SqlBackend, without breaking this.

In hindsight, I wish I made this a Maybe field - or even omit it from the list entirely. There's an obvious default, though it does mean that mkSqlBackend :: MkSqlBackendArgs -> IO SqlBackend needs to be done to produce the cache instead.

So, for a 2.13 release, can we avoid breaking this?

Then, for 2.14, we can either remove this (and only use the setConnStatementCache) or make it a Maybe field, and also make mkSqlBackend in IO.

blessed API 🙌🏻

codebase uses 4 space indent

The only thing here that needs IO is the newIORef, so if we extract+purify,

mkSimpleStatementCache :: IORef (Map k v) -> MkStatementCache`

meaning that we can go from IORef (Map Text Statement) to a StatementCache purely, and can therefore do it in mkSqlBackend.