Implement MySqlException.IsTransient

[roji]

dotnet/runtime#34817 introduces DbException.IsTransient for .NET 5.0, database-agnostic transient error detection. Not sure which transient errors Sqlite surfaces (e.g. locking/transactions?), these could be surfaced via this property.

MySQL and PostgreSQL both expose SQLSTATE codes directly from the database. If the actual codes and their transience overlap to a considerable amount, we should consider bringing some logic into DbException itself, i.e. provide a default implementation of IsTransient which identifies some hard-coded values in SqlState and returns true.

Here's the current Npgsql implementation for IsTransient.

[bgrainger]

Similar to npgsql, this could probably just be driven from the ErrorCode property. Error codes that could be considered transient would include:

• ConnectionCountError
• UnableToConnectToHost
• OutOfResources
• DiskFull

Because the default value of IsTransient is false, false negatives will occur. Depending on how important it is to avoid false positives, CommandTimeoutExpired could also be included; it may be transient if the database is busy servicing other clients, but not if the query is inherently slow.

[roji]

Which specific errors are classified as transient is of course a provider decision. However, the way I think about it is to err on the side of false positives, rather than conservatively erring on the side of false negatives. A false positive would typically mean a needless retry (which doesn't seem critical), whereas a false negative would mean throwing where a retry may have result in success.

[bgrainger]

@roji What are your thoughts on IsTransient and transaction deadlock? For example, MySqlErrorCode.LockWaitTimeout and LockDeadlock have an explicit message from the server that says to retry: "Lock wait timeout exceeded; try restarting transaction" or "Deadlock found when trying to get lock; try restarting transaction". However, simply retrying the last command is usually insufficient to (correctly) resolve this; the entire transaction needs to be retried, which may need more complicated logic than just retrying the statement that generated an IsTransient exception.

[roji]

That's an important question, I addressed it in the proposal:

In at least some cases, if an operation transiently fails within a transaction, the entire transaction must be rolled back and retried (including all previous operations). This would be out of scope of this proposal: DbException would only inform the consumers that the exception is transient, but the actual handling (rolling back, reissuing previous commands) would be the consumer's responsibility.

In other words, IsTransient=true doesn't in any way imply that the command alone should be retried for a possibly successful outcome - only that the error itself is (possibly) transient. It's indeed the responsibility of the layer above the ADO.NET provider to track commands within the transaction, and I'd expect that layer to rollback and retry the entire transaction when getting a transient error. This is exactly how EF Core behaves (also employing transaction savepoints).

Let me know if that makes sense to you.

[bgrainger]

Thanks, that's really helpful. (And sorry I missed that you had already explicitly addressed this.)

[bgrainger]

List of transient errors from Pomelo: https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/blob/d7e73b4bd27e9c719af914da9e12a005e9cc328e/src/EFCore.MySql/Storage/Internal/MySqlTransientExceptionDetector.cs#L23-L39

[roji]

Sure thing, and starting out from the Pomelo list sounds like a good idea.

It's interesting that a MySqlErrorCode is used - does that mean there are two error codes in MySql, i.e. the 5-character SqlState and also this? I wonder what the pros/cons of each are.

[bgrainger]

A user points out that this code path logs an error then throws an exception that will be IsTransient:

MySqlConnector/src/MySqlConnector/Core/ServerSession.cs

Lines 376 to 377 in 4cd45f6

	Log.Error("Session{0} connecting failed", m_logArguments);
	throw new MySqlException(MySqlErrorCode.UnableToConnectToHost, "Unable to connect to any of the specified MySQL hosts.");

Logging an error (that may be passed through to the client's logs) may be too serious in cases when we expect that user code will retry to handle this error.

I guess philosophically it's going to come down to who sets the definition of "Error": MySqlConnector, or the consuming application, and what counts as an error?

[roji]

It's definitely an interesting discussion, but it seems somewhat orthogonal to error transience. Even for a totally non-transient error, the application may catch the exception above in the stack and handle it (e.g. by connecting to another database), so it could be claimed that the low-level driver shouldn't be logging. However, that would basically mean that no driver error can ever be logged, since who knows whether the calling application consider it an error or not...

Pragmatically speaking, if a low-level component (like a DB driver) logs (and that generally seems like a good idea), I think it should do so regardless of what's going up the stack, and cannot assume that transient errors would actually be retried; it can't know if a retry strategy is configured. Users always have the option to selectively disable logging of certain messages based on rules configured in their logging library... It's also possible for the driver to be configurable as to whether it logs for transient errors, though I'd personally wait and see if lots of people actually request that.

[bgrainger]

I used Pomelo's list as the definition for IsTransient.

[roji]

Great to see this getting implemented... At some point when it's released, the Pomelo provider could simply call IsTransient on your exception.

[bgrainger]

Yes; I opened PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#1285 to track that.

[bgrainger]

Added in 1.3.0.