Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement TransactionScope without XA Transactions #254

Closed
bgrainger opened this issue May 4, 2017 · 19 comments
Closed

Implement TransactionScope without XA Transactions #254

bgrainger opened this issue May 4, 2017 · 19 comments

Comments

@bgrainger
Copy link
Member

MySQL XA Transactions have some restrictions that may impact users (who use TransactionScope) migrating from Connector/NET to this library.

We could provide an alternate implementation of IEnlistmentNotification (that uses regular MySQL Transactions), which would be more compatible with Connector/NET. This would probably need to be an opt-in connection string option so that we get correct behaviour by default.

Please 👍 this issue if it affects you.

(Split from #13.)

@bgrainger
Copy link
Member Author

#546 contains a test case that should be used to verify backwards compatibility with Connector/NET when this is implemented.

@Timovzl
Copy link

Timovzl commented Nov 13, 2018

With Connector/NET still not being async, I too am eager to migrate, but hindered by the current inability to combine transaction scopes with replication.

I believe the most essential part is this: If within a connection a new connection is requested with the exact same connection string as one already in use in that scope, instead the one already in use must be used. This is how Connector/NET seems to achieve single-host/non-XA transactions.

Also, I am curious how this would affect the existing XA behavior. If I understand correctly, it should work fine within an XA transaction as well. Is there a reason for an XA transaction to use different connections if the connection string is identical?

@bgrainger
Copy link
Member Author

If within a connection a new connection is requested with the exact same connection string as one already in use in that scope, instead the one already in use must be used. … Is there a reason for an XA transaction to use different connections if the connection string is identical?

This is just a side-effect of new MySqlConnection(connectionString) always opening a "new" connection to the server. (It may be a reused pooled connection, not a new connection created just-in-time, but it's different from all other new MySqlConnection(connectionString) connections that have been created (and not disposed) by the current thread.)

If you're proposing the following, it would be quite unexpected behaviour from an API perspective and probably have a lot of unexpected side-effects and bugs:

using (TransactionScope...)
{
    using (var connection1 = new MySqlConnection(connectionString))
    using (var connection2 = new MySqlConnection(connectionString))
    {
        // connection1 and connection2 represent the same physical connection to the server
        await connection1.OpenAsync();
        // is connection2 implicitly open now? do I have to open it explicitly still?
        await connection2.OpenAsync();
        // what if I call connection1.Close()? is it reference-counted?

@Timovzl
Copy link

Timovzl commented Nov 14, 2018

This is just a side-effect of new MySqlConnection(connectionString) always opening a "new" connection to the server. (It may be a reused pooled connection, not a new connection created just-in-time, but it's different from all other new MySqlConnection(connectionString) connections that have been created (and not disposed) by the current thread.)

I see your point, yes. That does seem like a natural side-effect.

Admittedly, what to do with OpenAsync() is not entirely clear in the scenario I propose. I would say, do as you normally would. Always open it. It may even initially represent its own state as closed, even though the underlying connection may be actually open. Only once we OpenAsync(), it actually takes a look, and either opens the underlying connection or determines that it is already open.

This seems to me like the way to achieve non-XA transactions. I believe it is how Connector/NET does it. Do you have any alternatives in mind?

What I was originally wondering is if this behavior would be an issue to XA transactions. I guess it's a breaking change, since two separate connections could have two readers open, whereas if it is really just a single connection underneath, the first reader must be closed before the second can be opened.

As such, being non-XA will have to be a setting on the connection string, because we cannot achieve it by simply constraining ourselves to a single distinct connection string within a transaction.

@bgrainger
Copy link
Member Author

bgrainger commented Nov 15, 2018

@Timovzl Does your code look like this?

using (var transactionScope = new TransactionScope())
{
	using (var conn1 = new MySqlConnection(connectionString))
	{
		conn1.Open();

		using (var conn2 = new MySqlConnection(connectionString))
		{
			conn2.Open();
			// ...
		}
	}
}

Or this?

using (var transactionScope = new TransactionScope())
{
	using (var conn1 = new MySqlConnection(connectionString))
	{
		conn1.Open();
	}

	using (var conn2 = new MySqlConnection(connectionString))
	{
		conn2.Open();
	}
}

You wrote earlier "If within a connection a new connection is requested", which made me think you were talking about example 1; however, that isn't actually supported by Connector/NET, so this wouldn't be a compatibility/migration problem.

But if you meant opening two separate, non-nested connections (example 2), I think this could be supported by creating a special, one-entry connection pool for the TransactionScope. When a MySqlConnection is disposed, it would go back to that connection pool, ready to be reused if another connection is opened within the transaction. Then it would stay open until the TransactionScope is completed, at which point the transaction would be committed.

Would this fit your usage of TransactionScope?

@bgrainger
Copy link
Member Author

Similar issue in SqlClient: dotnet/corefx#32056

I would've thought if its the same connection string, we don't need to enlist as distributed transaction.

@Timovzl
Copy link

Timovzl commented Nov 19, 2018

@bgrainger I misspoke. I meant scenario two, within a single transaction scope opening, using, and closing a connection, and then opening, using, and closing an identical connection.

I like your idea. I would suggest one alteration. It would be nice to still have actual pooling behavior throughout the application. If this one-entry connection pool is only used for one transaction scope, that could be inefficient. Is it possible to do something like this: Borrow a connection from a regular pool, put it in a one-entry pool for the duration of the (outer) transaction scope, and when that one-entry pool is disposed, return the connection to the regular pool? Or perhaps there is another approach to avoid making a new connection for every (outer) transaction scope.

@bgrainger
Copy link
Member Author

It would be nice to still have actual pooling behavior throughout the application. If this one-entry connection pool is only used for one transaction scope, that could be inefficient.

Agreed. Particularly if TransactionScope is used around most/all DB calls, then giving each Transaction its own single-entry connection pool would essentially defeat connection pooling.

As it turns out, the MySqlConnection (and ServerSession) is already being kept open behind the scenes to perform the final XA COMMIT; it shouldn't be too difficult to borrow that session (i.e., physical connection) for subsequent MySqlConnections that are opened.

This would still use XA Transactions on the server, but would allow multiple sequential MySqlConnection objects to be opened and closed within the scope of one transaction. While that wasn't the initial description of this case, I believe it would satisfy the primary goal (which is compatibility with Connector/NET usage) without requiring an additional connection option and seamlessly permitting an upgrade to distributed transactions.

@bgrainger
Copy link
Member Author

Reopening #546 to track this work (of reusing the ServerSession). I'm hopeful this will resolve the major problems people are encountering with TransactionScope in MySqlConnector.

This case will remain open to track actually switching out the underlying transaction mechanism for non-XA transactions (most likely with an opt-in connection string option).

@Timovzl
Copy link

Timovzl commented Nov 30, 2018

This would still use XA Transactions on the server [...]

What does this entail?

Using Percona Xtradb Cluster, I'm limited to MySQL version 5.7.23, which is lower than the version that makes XA Transactions work with replication (5.7.7). I'm wondering if the server will see it as an XA Transaction and refuse it, or process it as with Connector/NET.

@bgrainger
Copy link
Member Author

5.7.23 is a later release than 5.7.7 so you should be fine.

@Timovzl
Copy link

Timovzl commented Dec 4, 2018

Before migrating, I'm still curious what the effect is of doing a single-host XA Transaction, compared to a regular database transaction. Is the commit optimized to one-phase if there is only one host involved? Are there other consequences?

Also, is there a way to forbid multi-host transactions? For now, I would like to throw an exception on accidental multi-host transactions, because at this point they really would be accidental.

@Timovzl
Copy link

Timovzl commented Dec 4, 2018

Sadly, with Percona Xtradb Cluster, the single-host XA Transaction does not work:

MySqlException: This version of MySQL doesn't yet support 'XA with wsrep replication plugin'

Even opening a single connection, and only once, the exception occurs as soon as I try to open it from within a TransactionScope. Apparently the cluster really demands a regular database transaction.

@bgrainger bgrainger self-assigned this Dec 5, 2018
@bgrainger
Copy link
Member Author

@Timovzl I've published a possible fix for this issue. Can you please install version 0.48.0-rc2, add UseXaTransactions=false to your connection string, and report whether it works with your code?

@Timovzl
Copy link

Timovzl commented Dec 7, 2018

Maestro, I salute you! UseXaTransactions allows me to open a connection to the cluster from within a TransactionScope.

Furthermore, here are some test results on various combinations:

  • Two connection strings, both with UseXaTransactions=False: NotSupportedException on opening the second connection. Test successful.
  • Two connection strings, the second with UseXaTransactions=False: NotSupportedException on opening the second connection. Test successful.
  • Two connection strings, the first with UseXaTransaction=False: no exception. Test failed.

I believe the last two scenarios should not produce a different result (only because of their order). The transaction cannot truly span the two connections if one of them has UseXaTransactions=False, so this is programmer error, and we need the exception.

(I cannot test actually writing to the cluster outside of production, but reading works just fine.)

@bgrainger
Copy link
Member Author

Thanks for pointing this out; I've pushed a fix in 7798487 and it'll be in the final release of 0.48.0.

@bgrainger
Copy link
Member Author

Implemented in 0.48.0.

@Timovzl
Copy link

Timovzl commented Dec 10, 2018

I'm afraid the intended fix made it worse:

- Two connection strings, both with UseXaTransactions=False: no exception. Test failed.
- Two connection strings, the second with UseXaTransactions=False: no exception. Test failed.
- Two connection strings, the first with UseXaTransaction=False: no exception. Test failed.

All three scenarios should throw, rather than (seemingly) succeed. The transaction behaves as if it spanned the two connections, while in actuality they can succeed or fail independently.

@Timovzl
Copy link

Timovzl commented Dec 10, 2018

I'm afraid the intended fix made it worse [...]

Sorry, scratch that! I had a supression transaction scope that I forgot to remove again.

All invalid combinations now correctly throw a NotSupportedException.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants