Improve `InvalidCastException` message for `MySqlDataReader.GetX()`

[bgrainger]

trafficstars

From https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/issues/1571#issuecomment-1017955409

If MySqlDataReader.GetX() is called for the wrong data type, or when the underlying data is NULL, it will throw InvalidCastException: Unable to cast object of type 'System.DBNull' to type 'System.String'. or InvalidCastException: Unable to cast object of type 'System.Int32' to type 'System.String'..

It's suggested that the column name (if GetX(string) is called) or ordinal number (for GetX(int)) be added to the exception message to help novice users diagnose the issue. Note that any implementation should be exceedingly careful to not regress performance: GetX is called successfully millions of times per day making the (rare) failure have a better message is not worth making the common success case slower.

[bgrainger]

I added the following methods (based on my proposed implementations) for testing:

	public int GetInt32PassName(int ordinal) => GetResultSet().GetCurrentRow().GetInt32(ordinal, null);
	public int GetInt32PassName(string name) =>
		GetResultSet().GetCurrentRow().GetInt32(GetOrdinal(name), name);

	public int GetInt32TryCatch(int ordinal)
	{
		try
		{
			return GetInt32(ordinal);
		}
		catch (InvalidCastException ex)
		{
			throw new InvalidCastException(ex.Message + $" for column {ordinal}");
		}
	}
	public int GetInt32TryCatch(string name)
	{
		try
		{
			return GetInt32(GetOrdinal(name));
		}
		catch (InvalidCastException ex)
		{
			throw new InvalidCastException(ex.Message + $" for column {name}");
		}
	}

The implementation of GetInt32(int, string) was fairly straightforward: just add the column ordinal or name to the exception message.

Benchmark results (for a call to GetInt32(int) that succeeds):

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19044.1499 (21H2)
Intel Core i7-10875H CPU 2.30GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.200-preview.22055.15
  [Host] : .NET 6.0.2 (6.0.222.6406), X64 RyuJIT

Method	Mean	Error	StdDev	Ratio
GetInt32	16.30 ns	0.119 ns	0.099 ns	1.00
TryCatch	18.67 ns	0.222 ns	0.185 ns	1.15
PassName	18.22 ns	0.185 ns	0.173 ns	1.12

Being 12-15% slower in the successful case is an unacceptable slowdown.

[bgrainger]

A revised implementation (for GetInt32(int)) that just changes the exception message (to include the column ordinal):

Method	Mean	Error	StdDev	Ratio	RatioSD
GetInt32	15.66 ns	0.197 ns	0.175 ns	1.00	0.00
Exception	15.95 ns	0.232 ns	0.193 ns	1.02	0.02
TryCatch	17.88 ns	0.078 ns	0.069 ns	1.14	0.01
PassName	17.00 ns	0.122 ns	0.108 ns	1.09	0.01

Benchmarks for GetInt32(string):

Method	Mean	Error	StdDev	Ratio	RatioSD
GetInt32	26.15 ns	0.344 ns	0.305 ns	1.00	0.00
Exception	26.56 ns	0.263 ns	0.246 ns	1.01	0.02
TryCatch	28.70 ns	0.499 ns	0.467 ns	1.10	0.02
PassName	26.83 ns	0.490 ns	0.409 ns	1.03	0.02

PassName isn't too much slower for GetInt32(string) but would make GetInt32(int) 9% slower or require the implementation to be doubled. TryCatch adds too much overhead for both methods.

It does seem that the implementation could be rewritten to include the column ordinal in the exception message (in all cases) without a significant hit to performance. This is a little less helpful for users using the string overload, but may still provide a small benefit to tracking down the problem.

[mguinness]

Upvote from me, even just the ordinal position would be very helpful when tracking down issues.

[lauxjpn]

Let me throw two approaches in here, that are overkill for just this particular issue, but could be worth considering for better debugging support in general without sacrificing perf:

• There could be two packages, one for production usage and one for debugging/development. The code base would use #if conditions to add the debugging functionality.
• There could be an additional package, that uses a library like Harmony to augment a couple of calls with additional functionality.

Such a debugging approach could then provide additional logging, improved exception messages, etc.

[bgrainger]

There could be two packages, one for production usage and one for debugging/development.

Aside from the problem of bifurcating the ecosystem (will there now be two versions of Pomelo?), my suspicion is that everyone would choose the "production" package, and we'd be in the same situation we are now. Issues like #1092 and #1010 (as well as the high number of click-throughs on URLs currently in exception messages) show that there are a large number of users who need extra information when something goes wrong. These users likely don't know that they are the ones who should use the "debugging" package.

If I were to make two packages (which I don't think is a good idea), they would probably be "MySqlConnector" (best for debugging, has more runtime checks) and MySqlConnector.HighPerformance.IKnowWhatImDoing (all runtime checks stripped out); the latter could be used for TechEmpower Framework Benchmarks and other truly high-performance application needs.

[lauxjpn]

Yeah, I just thought I mention it. This is definitely a controversial subject with no perfect outcome either way. For larger systems, like e.g. Windows itself, it is not uncommon to have dedicated checked builds (Microsoft term for debug builds), so that you can debug in a complex environment with as much assistance as possible. But whether approaches like these are worth it on a package level is questionable. And so far I have not seen wide adaptation on nuget.org for going in this direction.