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

Weird result when using query instead of prep_exec #107

Closed
wqfish opened this issue Mar 27, 2020 · 7 comments
Closed

Weird result when using query instead of prep_exec #107

wqfish opened this issue Mar 27, 2020 · 7 comments
Assignees

Comments

@wqfish
Copy link

wqfish commented Mar 27, 2020

When debugging an issue in my application that uses mysql_async, I ran into a problem where I get different result by calling query and prep_exec.

I have reduced the program to the following to reproduce the issue:

Cargo.toml: https://gist.github.com/wqfish/7dddb18da14682556e5c33639eeb7b20
main.rs: https://gist.github.com/wqfish/c4a69477b90880c2c5731ce8f90d4747

The content of the db is pretty simple. The db mydb has a single table my_table that has two rows in it: https://gist.github.com/wqfish/21125e61fc305ba2958083fc795dbef7

The problem

With this program I can successfully load the two rows from mysql. However, if I comment out line 58 in main.rs and use line 57 instead, I only get one of the two rows. So changing prep_exec to query caused the result to be different.

Does this feel like a bug or I'm missing something?

@wqfish
Copy link
Author

wqfish commented Mar 27, 2020

Another way to demonstrate the problem is the following:

Let's just use query instead of prep_exec, so I can not get both rows with the above program. However if I make the following change to the code

diff --git a/main.rs b/main.rs
index 5c86d32..8c3ee75 100644
--- a/main.rs
+++ b/main.rs
@@ -53,7 +53,7 @@ async fn main() -> Result<(), mysql_async::error::Error> {
 
     let conn = conn.drop_query(r"USE mydb").await.unwrap();
 
-    let q = "SELECT b, c, d, e FROM my_table";
+    let q = "SELECT a, b, c, d, e FROM my_table";
     let result = conn.query(q).await.unwrap();
     // let result = conn.prep_exec(q, ()).await.unwrap();
 
@@ -70,7 +70,7 @@ async fn main() -> Result<(), mysql_async::error::Error> {
 
     let (_ /* conn */, loaded_structs) = result
         .map_and_drop(|row| {
-            let (b, c, d, e): (_, _, _, _) = mysql_async::from_row(row);
+            let (a, b, c, d, e): (u64, _, _, _, _) = mysql_async::from_row(row);
             MyStruct { b, c, d, e }
         })
         .await

(Basically I select one more column from the DB.) Then I get both rows.

@wqfish
Copy link
Author

wqfish commented Mar 27, 2020

I spent some time debugging and tracked down to this code where we parse the packet to determine if there are more results. It seems that the code returned None when it's supposed to return Some. I don't really know the format of the packets to debug further.

@blackbeam
Copy link
Owner

Hi. Thanks for report!

Could you please test this using the latest commit on master?

@wqfish
Copy link
Author

wqfish commented Mar 28, 2020

@blackbeam Yeah it seems to be working now on latest master. Thanks for the quick fix!

@wqfish
Copy link
Author

wqfish commented Mar 30, 2020

Before I close this issue, do you mind explaining why blackbeam/rust_mysql_common#17 fixes this? I don't know much about mysql and all I can tell is that I'm not using floats in my code here.

@blackbeam
Copy link
Owner

Before I close this issue, do you mind explaining why blackbeam/rust_mysql_common#17 fixes this?

Oh. No, your problem is fixed by 8dd31c1.

The blackbeam/rust_mysql_common#17 is just another breaking change that I wanted to include into mysql_common v0.21.0 release.

@wqfish
Copy link
Author

wqfish commented Mar 30, 2020

Oh that makes sense. Thanks!

@wqfish wqfish closed this as completed Mar 30, 2020
facebook-github-bot pushed a commit to facebookexperimental/rust-shed that referenced this issue Apr 6, 2020
Summary:
Update mysql_async to the latest upstream version.

This version contains the IPv6 address fix
(blackbeam/mysql_async#95) that is required for using
mysql_async with a local myrouter binary on a tw task with IPT enabled.

Remove the override that was used to include this fix before it was merged
upstream.

mysql_async has moved to async/await, so update the common/rust/shed/sql crate
to be able to interface with new style futures by using .compat().

Further, 0.23 fixed a bug that makes `prep_exec` and `query` behave differently
that was found by wqfish and fixed upstream:
blackbeam/mysql_async#107

As part of the aforementioned fix, a new mysql Value variant was added:
`Value::Double`.  Update sql_ext to be able to map between `Value::Double` and
sqlite's `REAL` type.

NOTE: I have disabled LSAN as it doesn't interact well with tokio-0.2, causing
LSAN to complain after tests have run.

Reviewed By: farnz

Differential Revision: D20366749

fbshipit-source-id: 90d1ccc364313c979d52d3e84ccb9ee66d6dc21c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants