Change mysql error detection strategy #224

Merged
merged 1 commit into from Nov 10, 2011

Projects

None yet

2 participants

@woahdae
Contributor
woahdae commented Nov 10, 2011

Previous to this commit, some mysql connection errors were not detected
by the client.

This commit changes from implicit error detection via checking
mysql_field_count() to explicit detection via mysql_errno().

http://dev.mysql.com/doc/refman/5.0/en/null-mysql-store-result.html

Note that this was discovered while investigating #66

Also, from the bottom of the mysql_field_count() docs, it notes:

An alternative is to replace the mysql_field_count(&mysql)
call with mysql_errno(&mysql). In this case, you are checking
directly for an error from mysql_store_result() rather than
inferring from the value of mysql_field_count() whether the
statement was a SELECT.

While neither documentation says it explicitly, it seems that mysql_errno() will return errors that the strategy based on mysql_field_count() won't.

It is certainly my experience that there are connection errors happening that only checking mysql_errno() detects (see #66)

@brianmario brianmario and 1 other commented on an outdated diff Nov 10, 2011
ext/mysql2/client.c
@@ -310,10 +310,13 @@ static VALUE rb_mysql_client_async_result(VALUE self) {
result = (MYSQL_RES *)rb_thread_blocking_region(nogvl_store_result, wrapper, RUBY_UBF_IO, 0);
+ fprintf(stderr, "Thread id: %i\n", mysql_thread_id(wrapper->client));
@brianmario
brianmario Nov 10, 2011 Owner

could we ditch this?

@woahdae
woahdae Nov 10, 2011 Contributor

crap, that's what I get for not QR'ing myself. Yes.

@brianmario
Owner

Sweet! You think you could whip up a test for this? I know the case is a little funky but I think we might be able to force an exception be raised for this.

@woahdae
Contributor
woahdae commented Nov 10, 2011

If you have any testing ideas, I'm game. I thought about it, but couldn't think of an easy way to reproduce it, at least just operating through the ruby client library. I didn't crack open the test suite, though, so I'll do that and maybe I'll get an idea about how to get a connection error out of mysql in there.

Suggestions?

@woahdae woahdae Change mysql error detection strategy
Previous to this commit, some mysql connection errors were not detected
by the client.

This commit changes from implicit error detection via checking
mysql_field_count() to explicit detection via mysql_errno().

http://dev.mysql.com/doc/refman/5.0/en/null-mysql-store-result.html
a609ea1
@woahdae
Contributor
woahdae commented Nov 10, 2011

[removed the printf in a force push]

@brianmario
Owner

Sweet, I'll go ahead and merge and try to think up a test - if you have any ideas send another PR.
Thanks again!

@brianmario brianmario merged commit 1a14f06 into brianmario:master Nov 10, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment