Make thread-safe wrapper around getStmtConn.

[merijn]

Refer to #981.

[merijn]

This check is basically useless, we can instead rely on the MVar to track if the query is alive and "refresh" it if someone tries to use it again, but this complicates the Acquire logic and I'm too tired to fix it right now.

[merijn]

Running concurrent queries is threadsafe and works with this PR, but concurrent inserts result in utter garbage due to the two stage ROWID based method of returning the key that was just inserted (for SQLite and presumably the same of MySQL).

[parsonsmatt]

Thanks for all the work on this stuff. I'll try to get to it soon!

[parsonsmatt]

This change in behavior is complicated enough that I'd want to gate it behind a major version bump, which means I also want to collect other breaking changes into a big release.

I could be convinced to release it as a patch version, but only with sufficient testing or correctness guarantees. I'd hate to release a version and have someone's application deadlock unexpectedly. However, if there application would have otherwise blown up or done something bad, then I might be okay with it too.

[merijn]

Yeah, this PR was/is more of a proof-of-concept/starting point

[parsonsmatt]

@merijn Now that it's the new year, how are you feeling about this? I've reviewed it a few times and I feel good about the change.

[merijn]

I think this one is still a bit dicy, since as I commented before concurrent inserts basically corrupt the "safe" keys you get back. I think it can be made to work, but it might require overhauling the way inserts are done in all the SQL backend, so that's quite some work.

[parsonsmatt]

@merijn Do you think you'll have time to do this in the next week or two?

I'm happy to accept an improvement to the status quo, even if it doesn't completely solve the problem, provided that the remaining issues can be documented.

[ygale]

I am opposed to merging this PR.

This is a breaking change for all persistent users to implement a backend-specific feature. Please implement this as part of the SQLite backend, not as part of persistent.

[merijn]

This PR (as is) can't be merged, since it doesn't actually work on any backend and I don't have time to get things working anytime soon. That said, this is also wrong:

to implement a backend-specific feature.

I haven't tested to confirm, but I can't see a way the current implementation isn't broken for all persistent backends. Now, admittedly the other backends rely on connection pools more, so are less likely to run into breakage as they don't run into issues with concurrent queries from separate threads. However, that's not all that's broken, running multiple queries in a conduit from the same thread is also horribly broken, and half the ConduitT based API in persistent will happily leak unfinished queries wasting memory in the database.

[ygale]

You are referring to the selectSource family of methods, I suppose. Hmm, those are indeed problematic. But the prepared statement cache is not the issue for most backends, and changing it won't help. There needs to be a way to block re-use of a connection until the active query finishes. This is not a simple problem.

EDIT: Moving discussion back to the issue.

[codygman]

There needs to be a way to block re-use of a connection until the active query finishes. This is not a simple problem.

My team is affected by this and it results in the postgres error:

libpq: failed (another command is already in progress

From code like:

withDB1
  . Conduit.runConduit
  $ (Persist.selectSource [] [] .| Conduit.mapM_
      (\stuff ->
        runMyMonadT env . withDB2 . void $ Persist.upsert myModel)
    )