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

Additional issues with `r2d2-diesel` and MySQL #728

Closed
kardeiz opened this Issue Feb 17, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@kardeiz
Contributor

kardeiz commented Feb 17, 2017

Thanks for PR 727 and the new release (0.11.1)! Your speed and willingness to help is amazing!

Unfortunately, I'm encountering further issues when using r2d2 with MySQL. Whenever I try to run a query, like:

users::table.limit(5).load::<User>(&*conn).expect("Users load failed")

I get

DatabaseError(__Unknown, "Commands out of sync; you can\'t run this command now")

When I get a connection directly using MysqlConnection::establish, everything works fine.

I dug around a little bit, and it is definitely related to the query in the is_valid health check. I put together an alternate impl for ::r2d2::ManageConnection where is_valid always returned Ok(()), and it worked fine. (Of course, that is not a viable solution for the managed connection.)

@sgrif

This comment has been minimized.

Member

sgrif commented Feb 17, 2017

I suspect we need to move

unsafe {
while ffi::mysql_next_result(self.0) != -1 {
self.did_an_error_occur()?;
}
}
to
self.did_an_error_occur()
. Testing will be a bit of a pain. I will explore it further tomorrow.

@kardeiz

This comment has been minimized.

Contributor

kardeiz commented Feb 20, 2017

@sgrif, I think this just has to do with not consuming/flushing the query result. If I change

conn.execute("SELECT 1").map(|_| ()).map_err(Error::QueryError)

to

sql::<::diesel::types::Bool>("SELECT 1")
    .get_result::<bool>(conn)
    .map(|_| ())
    .map_err(|_| Error::QueryError)

it works fine.

I don't know if this is the most efficient way to do a health check. MySQL has a ping mechanism, but I don't know all the considerations around it, and it wouldn't work for a generic Connection in r2d2-diesel.

Or maybe there is just some way to flush/drop the row/value without using it?

@Ameobea

This comment has been minimized.

Ameobea commented Apr 8, 2017

I'm getting this same issue; unable to perform any queries using r2d2-diesel and MySQL. Is there a fix on the radar for this? I've tried to look into a solution on my own but I'm very unfamiliar with diesel and databases in general.

Ameobea added a commit to Ameobea/r2d2-diesel that referenced this issue Apr 9, 2017

Made MySQL specific to fix bug with queries
 - Used fixed detailed in issue (diesel-rs/diesel#728) to fix issue with MySQL

sgrif added a commit that referenced this issue Aug 5, 2017

Flush pending results in `MysqlConnection#execute` and `batch_execute`
Whenever any call to `mysql_query` (the fake one) or `mysql_real_query`
(make sure you don't use the fake one) contains anything which could
return a result, the caller is responsible for what is effectively
flushing the network buffer. MySQL is not reading any packets from the
response until after we call things like `mysql_next_result`. If we
don't ensure that we've cleared those packets out, we'll get an error
indicating that commands are being run out of sync. This means that any
migration containing a `SELECT` will fail, and our `SELECT 1` ping from
r2d2-diesel won't work.

Fixes #728

sgrif added a commit that referenced this issue Aug 5, 2017

Flush pending results in `MysqlConnection#execute` and `batch_execute`
Whenever any call to `mysql_query` (the fake one) or `mysql_real_query`
(make sure you don't use the fake one) contains anything which could
return a result, the caller is responsible for what is effectively
flushing the network buffer. MySQL is not reading any packets from the
response until after we call things like `mysql_next_result`. If we
don't ensure that we've cleared those packets out, we'll get an error
indicating that commands are being run out of sync. This means that any
migration containing a `SELECT` will fail, and our `SELECT 1` ping from
r2d2-diesel won't work.

Fixes #728

@sgrif sgrif closed this in #1075 Aug 5, 2017

@onesk onesk referenced this issue Sep 12, 2018

Closed

r2d2 mysql 8.0 can not work. #1826

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment