mightybyte / snaplet-postgresql-simple

BSD 3-Clause "New" or "Revised" License
40 stars 38 forks source link

Enable nested transactions and rollbacks? #56

Open kindaro opened 3 years ago

kindaro commented 3 years ago

So, SQL has two features that essentially do the same thing. First you have to use begin … rollback / commit, and then inside it you can use savepoint … rollback to savepoint / release savepoint, and you can do that any number of times, effectively creating arbitrary nesting of transactions.

This means that in principle transactions compose. But at present, if I wrap a snaplet that uses a transaction into another snaplet that also uses a transaction, it will be a mess: the transaction will start at the first begin, the inner begin will be ignored, then the inner rollback / commit will affect everything back to the first begin and the final rollback / commit will be ignored. Good for me that PostgreSQL emits a warning in such cases!

This is not in principle a problem — we only need to track in the HasPostgres monad what the current nesting level is. Then we can intelligently choose whether to use begin or savepoint. For bonus points, we may also carry around a source of fresh names for save points — this would be required for vanilla SQL, but PostgreSQL allows using the same name for nested save points, so it is not necessary.

Can we have this feature? I am considering writing it for a private project and if it goes well I may be able to contribute a patch.

kindaro commented 3 years ago

While at it, maybe we can also add some locking with an MVar, so that another thread that happens to have the same connexion does not interfere with a transaction… There is some conversation about it up the stream: lpsmith/postgresql-simple#202 — but it is inconclusive and we are in a better position to do this since we are high level and already carry around an environment.

mightybyte commented 3 years ago

My rough idea for this library was that it would live at a lower level of abstraction than what you're talking about. Now it is true that this package has a few withTransaction... functions dealing with transactions, but it also has the primitive begin, rollback, and commit functions. My initial thought is that if the included withTransaction... functions don't do what you want out of the box, then you should write your own transaction handling infrastructure outside of this package using the lower level primitives. MVars and separate threads definitely seem out of this package's scope. Are there any obstacles to you implementing this functionality in your app as opposed to in this package?

kindaro commented 3 years ago

Sure, if this does not align with your vision, I can find a way to handle it independently.

kindaro commented 3 years ago

So, I drafted a proof of concept and it is actually very simple.

There are no MVar s and no threads — this was a red herring. We simply add a Bool field to PostgresConn and use it to track whether we are inside a transaction, in which case we use save points instead. It is strictly more powerful:

I think it would be problematic to implement this on top of snaplet-postgresql-simple.

That is to say, the abstraction of snaplet-postgresql-simple does not permit the feature of nested transactions to be independently added on top.

This is why I would like you to take a look at this patch.

I completely understand if you do not want to add this feature. But it is small, mostly backwards compatible (only internal interface is changed) and does not add any more imports or dependencies. And most importantly, it removes a foot gun and makes more things possible.