globalsign / mgo

The MongoDB driver for Go
Other
1.97k stars 231 forks source link

Alternative support for Mongo Transactions in v4 #334

Open jameinel opened 5 years ago

jameinel commented 5 years ago

Needs some testing around multiple version handling, and proper error messages if you try to use transactions with Mongo versions that don't support them.

We'll also need more testing around multi threading, etc. This was mostly a proof of concept that it could be done without a separate 'Transaction' object that users need to worry about. It also supports having Query be transaction aware.

This does make sure that Start/Commit/AbortTransaction all do essentially what you want, and it doesn't involve any client-side changes. If you call Session.StartTransaction then all Update/Insert/Remove/Find requests will be done in that context, and won't be visible to other sessions.

There is also still a fair bit more that can be done wrt things like WriteConcern. I mostly wanted to get feedback if people like how this is structured and where it is going.

As far as I can tell, while there is a txnNumber associated with a given transaction, Mongo doesn't actually support having >1 open transaction for a given session, so it isn't worth tracking transaction objects for users.

This is hopefully a simpler method to support Mongo Transactions than #280.

It isn't complete yet, but it is ready for initial feedback before I commit much more time to it.

duskglow commented 5 years ago

Hi, I'm the author of #280. I'm not sure which is better, honestly, but some feedback really quick: I think the txnNumber does need to be kept track of. You're correct that mongo doesn't support more than one open transaction, but when you start a new one, it just supercedes the old one, and the old one cannot be committed anymore. So what happens if you, for example, start the transaction twice?

I ran into some issues with that, and it turns out that if you leave a transaction open, and restart it or start a new one, then committing has some interesting issues.

I like your approach to backwards compatibility, but what of the cases where you want to do some things in a transaction and some things outside of a transaction? Collections cannot be automatically created in a transaction, for example, so if you want that functionality you have to be able to stop using transactions. I imagine that an abort or commit could close the current transaction and reset it back to not using transactions, but that's something that needs to be taken into account.

I had created the transaction object for the purpose of keeping track of multiple transactions, say, in multiple sessions, but in hindsight, that might be overkill...

Will say that yours has one huge advantage over mine: it passes all the tests. To be honest, I haven't yet figured out how to get the harnesses working properly with 4.x, so...

jameinel commented 5 years ago

You can create a separate session if you want some changes in a txn and some without. I have worked more on it to handle thread safety, etc. I need to reverse the changes to propose them here but you can see them at:

https://github.com/juju/mgo/pull/6

Clone shares the underlying Socket but not the Session, Copy creates a new Socket so mongo can do the queries in parallel. Session should be a relatively cheap object. So rather than create another thing to worry about, just use it.

There is more we could do if we wanted to help enforce threading issues. But fundamentally txn and multithreading aren't great. You have to have ordering on Start/Commit and when you do queries or you really don't have a txn. Also, our use case doesn't need multithreaded queries during a txn. So I made the code go test -race safe but not "race" safe. (Fundamentally you have to know which request is the first one and wait/assume it succeeds for that "startTransaction" to be accepted before sending any other requests.)

John =:->

On Thu, Mar 14, 2019, 00:26 Russell iller notifications@github.com wrote:

Hi, I'm the author of #280 https://github.com/globalsign/mgo/pull/280. I'm not sure which is better, honestly, but some feedback really quick: I think the txnNumber does need to be kept track of. You're correct that mongo doesn't support more than one open transaction, but when you start a new one, it just supercedes the old one, and the old one cannot be committed anymore. So what happens if you, for example, start the transaction twice?

I ran into some issues with that, and it turns out that if you leave a transaction open, and restart it or start a new one, then committing has some interesting issues.

I like your approach to backwards compatibility, but what of the cases where you want to do some things in a transaction and some things outside of a transaction? Collections cannot be automatically created in a transaction, for example, so if you want that functionality you have to be able to stop using transactions. I imagine that an abort or commit could close the current transaction and reset it back to not using transactions, but that's something that needs to be taken into account.

I had created the transaction object for the purpose of keeping track of multiple transactions, say, in multiple sessions, but in hindsight, that might be overkill...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/globalsign/mgo/pull/334#issuecomment-472591535, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMYfR0HiIhRntx1_fZtXXzNCeX1_fhrks5vWV7zgaJpZM4bXP0q .

jameinel commented 5 years ago

BTW I forgot to say that I really do appreciate your feedback. I think I have good arguments for the structure but as it is a critical layer I want to make sure it is correct.

John =:->

On Thu, Mar 14, 2019, 00:26 Russell iller notifications@github.com wrote:

Hi, I'm the author of #280 https://github.com/globalsign/mgo/pull/280. I'm not sure which is better, honestly, but some feedback really quick: I think the txnNumber does need to be kept track of. You're correct that mongo doesn't support more than one open transaction, but when you start a new one, it just supercedes the old one, and the old one cannot be committed anymore. So what happens if you, for example, start the transaction twice?

I ran into some issues with that, and it turns out that if you leave a transaction open, and restart it or start a new one, then committing has some interesting issues.

I like your approach to backwards compatibility, but what of the cases where you want to do some things in a transaction and some things outside of a transaction? Collections cannot be automatically created in a transaction, for example, so if you want that functionality you have to be able to stop using transactions. I imagine that an abort or commit could close the current transaction and reset it back to not using transactions, but that's something that needs to be taken into account.

I had created the transaction object for the purpose of keeping track of multiple transactions, say, in multiple sessions, but in hindsight, that might be overkill...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/globalsign/mgo/pull/334#issuecomment-472591535, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMYfR0HiIhRntx1_fZtXXzNCeX1_fhrks5vWV7zgaJpZM4bXP0q .

domodwyer commented 5 years ago

Hi @jameinel

Sorry for the long, long delay!

This looks pretty sweet - I haven't got a >v4.0 mongo server to test against at the moment, but the unit tests look pretty comprehensive. I'll try stand one up and get a run.

Have you managed to try this in any real world usage?

Dom

jameinel commented 5 years ago

We're looking to incorporate this into Juju (github.com/juju/juju). We've been using it in some testing. It still needs some work to be able to substitute for mgo/txn, though I have started that work. That layer needs a bunch more testing for concurrency/error handling.

On the plus side, server side transactions are showing at least 10x faster than mgo/txn. So that is very promising.

John =:->

On Mon, Apr 1, 2019, 18:04 Dom notifications@github.com wrote:

Hi @jameinel https://github.com/jameinel

Sorry for the long, long delay!

This looks pretty sweet - I haven't got a >v4.0 mongo server to test against at the moment, but the unit tests look pretty comprehensive. I'll try stand one up and get a run.

Have you managed to try this in any real world usage?

Dom

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/globalsign/mgo/pull/334#issuecomment-478593056, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMYfQvigKUqbjW38GDVlAaJwhCT50mKks5vchH-gaJpZM4bXP0q .