Add 'supported' way to upgrade postgresql-libpq Connection to a postgresql-simple Connection

[BardurArantsson]

Hi, I was wondering if you'd be receptive to adding an "officially support" way to upgrade a postgresql-libpq Connection to a postgresql-simple Connection object?

Use case: I have a library foo where the users of the library supply a callback which takes the connection as a parameter. My issue is that I'd like users of my foo library to be able to program their callback using the postgresql-simple API, but also want to a) keep using postgresql-libpq for my foo library, and b) avoid incurring a from foo on postgresql-simple.

Currently it's possible to achieve this conversion by importing the Database.PostgreSQL.Simple.Internal and using the below code. However, I'd really like an "officially sanctioned" way to do it to avoid the "don't depend on anything in Internal.*" situation.

My proposed implementation would be:

upgrade :: PQ.Connection -> IO Connection
upgrade c = do
  hnd <- newMVar c
  obj <- newMVar IntMap.empty
  cnt <- newIORef 0
  return $ Connection hnd obj cnt

What do you think?

(EDIT: I'd be happy to do a proper PR if you agree that this is desirable.)

[lpsmith]

I wouldn't be opposed to implementing something like this inside Internal, and keeping it as a stable member of the Internal module, much like withConnection currently is. Not sure I am a huge fan of the name, but I can accept upgrade as a name as well.

You might also want to examine the full connection initialization sequence in postgresql-simple, which this doesn't do.

As to making this a full, public member of the postgresql-simple API, I'm considerably more leery. The fact is that a libpq connection object is a complicated, stateful, very un-haskelly thing. Unfortunately, sharing a single connection object between multiple higher-level interfaces is asking for trouble, in the forms of subtle bugs and glitches such as (possibly temporary) deadlock, and memory faults. Often, the correctness of any given higher-level function also depends on the the "correctness" of other higher-level functions; see for example cocreature/hasql-notifications#1.

For example, IIRC, your CQRS library doesn't handle the issue that using a postgresql-libpq connection after it is explicitly closed is a memory fault. Unfortunately libpq conflates explicitly closing a connection with memory management, which doesn't play well with GHC's FFI.

[BardurArantsson]

Ah, right, my implementation was just a guess. Guess I should have actually looked at the existing code :).

I can certainly understand the wariness given the difficulty of using libpq connection objects safely. I think my use of the connection should be safe if we assume that the user's callback code doesn't do anything weird like e.g. issuing its own "COMMIT/ROLLBACK" statements or other weird things like that. There's only a single thread, etc. etc. (For context: The whole point here is that I want a transaction to span both my library code and the user's code. I also want to be maximally flexible -- even at the cost of safety. The latter means that I'll want to let the user execute arbitrary SQL.)

I wonder if I should reconsider my use of libpq and just use postgresql-simple instead. (Oh, how I wish there were a good TCP-based PostgreSQL client. That and a 'generic' Connection type like in JDBC. Oh, and I also want a pony!)

FWIW, I think I fixed the problems in my cqrs library by just deepseq'ing everything. The connection is only ever closed when connection pool decides.

If you don't mind I'll leave this open -- I think I might still write the conversion in the Internal.* module. I'll close if I decide against.

[lpsmith]

Ahh, well, I just glanced at your code some time ago, and briefly again as I wrote my response, so I apologize if I misunderstood the bigger picture. The memory fault issue is a common problem with haskell postgresql libraries, including groundhog, eariler versions of hasql, and hdbc-postgresql; I would re-introduce protections against that fault in postgresql-libpq except for the fact that it would introduce another level of indirection or two.

And yeah, leaving this open is fine by me. I would like to accept a patch for this; you could probably even split the connect function into two pieces and reimplement it in terms of upgrade. (Or maybe name it something like wrapLibpq, I dunno, it's not that important.)

[BardurArantsson]

Decided against. :/

I doesn't really appear super-practical to abstract anyway, so I might just switch to postgresql-simple wholesale.