Mongo Server Side Transaction handles #6

jameinel · 2019-02-28T16:10:25Z

This introduces the ability to StartTransaction/CommitTransaction/AbortTransaction from a mgo.Session object.

I haven't done any multithreaded testing yet, but I have confirmed the basic behavior around transactions and visibility with other sessions.

This is built on
#5
So the diff is a bit noisy right now.

This should allow supporting more versions of Mongo than the existing mgo driver.

Copying over the filterDBs changes and EnsureIndex fixes from globalsign/mgo.

Currently Abort is not supported, and it only supports Insert operations, but the basic layout is plausible and seems like it will work.

Interestingly, you can't see the documents that you've inserted in the session that inserted them...

Need to pass in lsid and txnNumber to enable visibility of queries for things that you just created.

Looks like we finally found the right attributes for where to put lsid and txnNumber. We now see the right visibility for updates and insertions. Still need to do Upsert/UpdateAll, etc.

Instead, all the places that were putting a transaction on the op before submitting it, just use the code path that submits the request to deal with the transaction.

SimonRichardson · 2019-03-01T13:52:48Z

session.go

+
+func (s *Session) CommitTransaction() error {
+	if len(s.sessionId.Data) == 0 {
+		// XXX: error?, shouldn't commit if we never called s.ensureSessionId


I'd agree with this comment, you're in a wrong state if you hit this..

Yeah, I was waiting until we have actual tests covering these cases before doing the work, since it is often easy to accidentally not test the error paths.

SimonRichardson · 2019-03-01T13:52:57Z

session.go

+		return nil
+	}
+	if s.transaction == nil || !s.transaction.started {
+		// XXX: error?, no active transaction


SimonRichardson · 2019-03-01T13:53:10Z

session.go

+		return nil
+	}
+	if s.transaction.finished {
+		// XXX: logic error, we shouldn't be able to get here


SimonRichardson · 2019-03-01T13:53:53Z

session.go

+		// XXX: logic error, we shouldn't be able to get here
+		return nil
+	}
+	// XXX: Python has a retry tracking 'retryable' errors around finishTransaction


That makes sense, sounds reasonable.

SimonRichardson · 2019-03-01T13:54:29Z

session.go

+		return nil
+	}
+	// XXX: do we want to track a transaction STATE, like they do in Python with states of:
+	// NONE, STARTING, IN_PROGRESS, ABORTED, COMMITTED, COMMITTED_EMPTY


What does that give us?

It is mostly a case of "is it more understandable if you have a sequence of states" vs just booleans.

That said, I've even taken out the 'finished' state. Because while I could track that in the driver, there really isn't a reason to. The source of truth is going to be the Mongo Server, and it will happily tell you if you're using it wrong.
And all the cases where the driver was trying to 'be helpful' looking at them, were around misusing the driver (eg, concurrent requests during a transaction, which
a) Juju itself doesn't need
b) Doesn't feel very safe anyway, as it is messing with the concept of transaction
)

achilleasa · 2019-03-06T14:31:58Z

session.go

@@ -3222,6 +3256,10 @@ type findCmd struct {
 	OplogReplay         bool        `bson:"oplogReplay,omitempty"`
 	NoCursorTimeout     bool        `bson:"noCursorTimeout,omitempty"`
 	AllowPartialResults bool        `bson:"allowPartialResults,omitempty"`
+	LSID                bson.D      `bson:"lsid,omitempty"`
+	TXNNumber           int64       `bson:"txnNumber,omitempty"`


Any reason not to use a uint64 here?

The actual number is being transmitted as a bson.Long:
https://docs.mongodb.com/manual/reference/bson-types/
Which is a 64-bit integer. I don't know if uint64 maps correctly to that type. I do know that plain 'int' doesn't (I believe it maps to type 16, 32-bit integer).
I'd also say, if you ever have more than 9e18 transactions, other stuff is probably breaking. :)

achilleasa · 2019-03-06T14:41:14Z

session.go

+	if err := s.ensureSessionId(); err != nil {
+		return err
+	}
+	if s.transaction != nil {


I think there is a potential data race here when StartTransaction gets called concurrently and you don't have an active txn: if two competing go-routines reach this if block at the same time they may both keep going and overwrite s.transaction.

A lock or CompareAndSwapPointers is probably needed here to be safe.

Session already has some Mutex objects, so I think I'll just use that. I was waiting until I felt comfortable with a test case that helps prove that it is threadsafe.

I think that fundamentally, StartTransaction is not a threadsafe action, because you aren't making any guarantees to the other threads as to whether they are in a transaction or not. but I want to make sure not to corrupt internal data structures if it ever did get called from multiple threads.

Is it possible to run multiple transactions within a single session (concurrently)?

It isn't possible in the 'mongo' driver, as the transaction start/stop is hidden inside the transaction (which is what I'm doing in the Go driver).
The Python driver says it isn't possible.
I cheated a bit with the Go driver and hacked some stuff together and you get this:

... value *mgo.QueryError = &mgo.QueryError{Code:225, Message:"Cannot start transaction 1 on session 64b9481a-b555-41f8-a0a0-15e3ab449c60 - 47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU= because a newer transaction 2 has already started.", Assertion:false} ("Cannot start transaction 1 on session 64b9481a-b555-41f8-a0a0-15e3ab449c60 - 47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU= because a newer transaction 2 has already started.")

FWIW, even if it was possible to have multiple txns on a single session, having the txn at the session level would mean that if you need parallel txns, you can just create another session. (Session.Copy()).

The test case looked like:

func (s *S) TestStartTransactionTwice(c *C) { session, coll := s.setupTxnSessionAndCollection(c) defer session.Close() c.Assert(session.StartTransaction(), IsNil) txn1 := session.GetTransaction() c.Assert(coll.Insert(bson.M{"a": "a"}), IsNil) // c.Assert(session.StartTransaction(), ErrorMatches, "transaction already started") c.Assert(session.StartTransaction(), IsNil) c.Assert(coll.Insert(bson.M{"b": "b"}), IsNil) txn2 := session.GetTransaction() session.SetTransaction(txn2) c.Assert(session.CommitTransaction(), IsNil) session.SetTransaction(txn1) c.Assert(session.CommitTransaction(), IsNil) var res bson.M c.Assert(coll.Find(bson.M{"a": "a"}).Select(bson.M{"a": 1, "b": 1, "_id": 0}).One(&res), IsNil) c.Check(res, DeepEquals, bson.M{"a": "a"}) c.Assert(coll.Find(bson.M{"b": "b"}).Select(bson.M{"a": 1, "b": 1, "_id": 0}).One(&res), IsNil) c.Check(res, DeepEquals, bson.M{"b": "b"}) }

achilleasa · 2019-03-06T14:49:52Z

session.go

+	}
+	// XXX: Python has a retry tracking 'retryable' errors around finishTransaction
+	err := s.finishTransaction("commitTransaction")
+	// TODO(jam): 2019-02-28 do we need a mutex around this since Insert/Update/etc are potentially different goroutines?


I think you need to hold a lock within the method just to be safe.

Here is an example that may cause things to blow up: for whatever (e.g logic bug) reason we get 2 go-routines calling Commit. One of them may reach L4930 (if s.transaction.finished) and get context-switched while the second one actually commits the txn and sets s.transaction = nil. This will cause an NPE when the first go-routine attempts to evaluate the is-finished predicate.

achilleasa · 2019-03-06T14:50:38Z

session.go

+	return nil
+}
+
+func (s *Session) AbortTransaction() error {


Same comment about holding locks while code is executing in this method.

In the current code, there is a mutex around the operation, and all the logic is pulled into the common 'finishTransaction', which just takes whether it is finishing with a Commit or with an Abort.

I also got rid of 'txn.finished'. I considered doing a mutex on Transaction, so that we get Start exactly right (if you call StartTransaction, and then 2 threads each try to do the first operation, you might get 2 'startTransaction' messages sent to the Mongo server). But I also realized, we don't actually care, because we won't be doing that, and I'd rather have people get told 'you're doing it wrong' than trying to make it work.

Also handle if a Session is Closed with an active transaction. Rework a lot of the Commit/Abort code paths, and make use of the Session.m object to properly handle Start/Abort/Commit. Needs tests around Insert/Update/Upsert/Remove/Find and Start/Commit/Abort sequencing.

These don't actually work because while there may not be a *data* race in memory, they race eachother for things like 'who is the first to start the transaction'. They also potentially race around 'is the transaction finished'. This might be good enough, as we don't really want to try to make it possible to do things multithreaded, we just don't want the memory structures to get corrupted. One caveat is that the TXN isn't actually started until the first operation, so it may be that we want to support an outer routine calling StartTransaction, but it forks off a bunch of helper routines that are all issued inside the transaction. If we want that, then the best thing is to probably put a Mutex on the 'transaction' object itself. And then we need to hold the mutex while the query is being performed, so that we know we really have issued 'startTransaction' on the remote side.

achilleasa · 2019-03-08T09:17:29Z

session.go

@@ -1609,12 +1628,23 @@ func (s *Session) Clone() *Session {
 // Close terminates the session.  It's a runtime error to use a session
 // after it has been closed.
 func (s *Session) Close() {
+	s.m.RLock()
+	txnActive := s.transaction != nil && s.transaction.started


Latching txnActive here will reduce but not entirely prevent the possibility for 2 go-routines obtaining the same value and then both trying to call AbortTransaction().

How about changing this to use a write lock instead, then move the if txnActive block inside the critical section and call out to something like s.abortTransactionWhileHoldingLock? AbortTransaction can be modified to just grab the write lock and call the private method which does the real abort.

It is currently non-trivial to hold the actual lock while doing this. Because calling Session.Run() needs to grab the write lock to set up the connection to the backend.
Session.Run() calls DB.Run() calls Session.acquireSocket taking the write lock and doing a lot of logic about what socket to talk to,
DB.Run then also calls DB.run() which again grabs a read-lock to handle some of the parameters.
In general, it isn't meant to be re-entrant and holding the lock for the whole time.
I could certainly do all the work to get it right, but for our purposes, we don't actually care. We don't really want to be doing multithreaded transaction work. All the code that is dealing with transactions that we will be setting up right now is going to be single-threaded. (If you need concurrency, then create another Session, and you'll even have separate TXNs available.)

This is mostly about making it nice for when you accidentally mess up your test case, and you have a proper defer Session.Close() but you forget to abort the transaction. Without this sort of patch, the test hangs. I assume because the socket doesn't close because it has an active transaction.

Makes sense, thanks for the clarification.

Clone doesn't copy the session or transaction, that way we don't have to worry about txn updates, but also we certainly think that Copy() should have its own transaction identity. And it makes sense that Clone should too. Especially we don't want Copy during a txn to behave differently than Copy outside of a transaction. Get rid of txn.finished. Let the server decide if the transaction is active.

jameinel · 2019-03-13T10:17:39Z

!!build!!

jameinel · 2019-03-13T10:20:10Z

!!build!!

jameinel · 2019-03-13T10:23:46Z

!!build!!

jameinel · 2019-03-13T10:26:55Z

!!build!!

jameinel · 2019-03-13T10:31:21Z

!!build!!

jameinel · 2019-03-13T10:36:13Z

!!build!!

jameinel · 2019-03-13T10:38:13Z

!!build!!

jameinel · 2019-03-13T10:40:56Z

!!build!!

jameinel · 2019-03-13T10:51:59Z

!!build!!

jameinel · 2019-03-13T11:04:25Z

!!build!!

jameinel · 2019-03-13T11:42:19Z

!!build!!

jameinel · 2019-03-13T11:47:18Z

!!build!!

jameinel · 2019-03-13T11:48:40Z

!!build!!

jameinel · 2019-03-13T12:05:22Z

!!build!!

jameinel · 2019-03-13T12:42:49Z

!!build!!

jameinel · 2019-03-13T13:15:16Z

!!build!!

jameinel · 2019-03-13T13:51:55Z

!!build!!

jameinel · 2019-04-10T08:55:45Z

I'm going to go ahead and merge this to master, since it shouldn't affect you if you aren't using txns.
$$merge$$

jameinel · 2019-04-10T08:56:00Z

!!build!!

jameinel added 11 commits February 27, 2019 16:07

Use the globalsign testing harness.

a71e200

This should allow supporting more versions of Mongo than the existing mgo driver.

Fix mixed whitespace issues.

9f9c39e

More tests that fail with Mongo 4.0.

2ba6a22

Copying over the filterDBs changes and EnsureIndex fixes from globalsign/mgo.

The rest of the tests pass on Mongo 4.0

d4f6294

Update Session to support tracking transactions.

a4146d4

Currently Abort is not supported, and it only supports Insert operations, but the basic layout is plausible and seems like it will work.

Implement AbortTransaction.

e763a98

Interestingly, you can't see the documents that you've inserted in the session that inserted them...

Start trying to implement query side of transactions.

2232dc5

Need to pass in lsid and txnNumber to enable visibility of queries for things that you just created.

Implement support for Find and Update.

79b643f

Looks like we finally found the right attributes for where to put lsid and txnNumber. We now see the right visibility for updates and insertions. Still need to do Upsert/UpdateAll, etc.

Cleanup the tests a bit.

482a2df

Implement support for UpdateAll and Upsert

73f9a11

Change away from annotating Ops.

a15af30

Instead, all the places that were putting a transaction on the op before submitting it, just use the code path that submits the request to deal with the transaction.

SimonRichardson approved these changes Mar 1, 2019

View reviewed changes

achilleasa reviewed Mar 6, 2019

View reviewed changes

jameinel added 2 commits March 7, 2019 20:41

achilleasa reviewed Mar 8, 2019

View reviewed changes

jameinel mentioned this pull request Mar 14, 2019

Alternative support for Mongo Transactions in v4 globalsign/mgo#334

Open

jujubot merged commit ece5c27 into juju:master Apr 10, 2019

Mongo Server Side Transaction handles #6

Mongo Server Side Transaction handles #6

Conversation

jameinel commented Feb 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Mar 13, 2019

jameinel commented Apr 10, 2019

jameinel commented Apr 10, 2019