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

Prepared Statements Bind Variable Error #5255

Closed
PrismaPhonic opened this issue Oct 1, 2019 · 6 comments
Closed

Prepared Statements Bind Variable Error #5255

PrismaPhonic opened this issue Oct 1, 2019 · 6 comments

Comments

@PrismaPhonic
Copy link
Contributor

PrismaPhonic commented Oct 1, 2019

Overview of the Issue

When using the most popular MySQL client library in Rust (https://docs.rs/mysql/16.1.0/mysql/) prepared statements (using the libraries prep_exec method) produces errors some of the time, whereas using normal queries does not. I've tested this against normal MySQL, where standard MySQL never errors on prep_exec usage.

Details

Here's an example of a raw query, where arguments are handled with a simple formatted string and sent over to vitess as a query:

impl HandlesQuery<&GetPupperQuery> for VitessPupperQueriesHandler {
    type Result = Result<Option<Pupper>, mysql::Error>;

    fn handle(&mut self, query: &GetPupperQuery) -> Self::Result {
        let r: Option<(u64, String, String)> =
            match self.conn.query(
                format!(r"SELECT p.id, p.name, p.image
                FROM puppers AS p
                WHERE p.id = '{}'", &query.id)
            ) {
                Ok(mut qr) => {
                    if let Some(row_result) = qr.next() {
                        let row = row_result?;
                        Some(mysql::from_row::<(u64, String, String)>(row))
                    } else {
                        None
                    }
                },

                // Underlying MySQL error type unrelated to existence of puppers in db.
                Err(e) => {
                    return Err(e);
                }
            };

        if let Some(pup) = r {
            return self.pup_with_rating(pup);
        }

        // Didn't find a pupper :-(
        Ok(None)
    }
}

Here is that same implementation using prep_exec:

impl HandlesQuery<&GetPupperQuery> for VitessPupperQueriesHandler {
    type Result = Result<Option<Pupper>, mysql::Error>;

    fn handle(&mut self, query: &GetPupperQuery) -> Self::Result {
        let r: Option<(u64, String, String)> =
            match self.conn.prep_exec(
                r"SELECT p.id, p.name, p.image
                FROM puppers AS p
                WHERE p.id = ?", (&query.id,)
            ) {
                Ok(mut qr) => {
                    if let Some(row_result) = qr.next() {
                        let row = row_result?;
                        Some(mysql::from_row::<(u64, String, String)>(row))
                    } else {
                        None
                    }
                },

                // Underlying MySQL error type unrelated to existence of puppers in db.
                Err(e) => {
                    return Err(e);
                }
            };

        if let Some(pup) = r {
            return self.pup_with_rating(pup);
        }

        // Didn't find a pupper :-(
        Ok(None)
    }
}

Error Message

Here is the error message I get some of the time when using prep_exec:

MySqlError { ERROR 1105 (HY000): vtgate: <hidden for security resaons> vtg1: bind variable is nil }

For what it's worth this is actually a different error message than what I was getting before. The schema prior to this had no primary key, and no indexes (only vindex). In that version of the application prep_exec resulted in 50% IO error (https://docs.rs/mysql/16.1.0/mysql/enum.Error.html#variant.IoError). That schema queried on the puppers name, which was not indexed. The error rate now is much lower (roughly 10%) and is this bind variable is nil error. Might be worth looking into handling of prepared statements in cases where the are no indexes.

@derekperkins
Copy link
Member

@PrismaPhonic Have you tried a recent build of Vitess? Prepared statement support was added at the beginning of August. #5018

@saifalharthi
Copy link
Contributor

@derekperkins Peter @PrismaPhonic is an engineer at PlanetScale. He was testing Prepared Statements.

@derekperkins
Copy link
Member

Ah, my bad.

@PrismaPhonic
Copy link
Contributor Author

Just to clarify that the primary problem of 50% IO error occurred when the schema had no primary key, and no indexes (only vindex on the name, which wasn't indexed).

@deepthi
Copy link
Member

deepthi commented Nov 23, 2019

@PrismaPhonic should this issue be closed now? Putting Fixes #<number> in the PR description will auto-close an issue when the PR is merged.

@PrismaPhonic
Copy link
Contributor Author

@PrismaPhonic should this issue be closed now? Putting Fixes #<number> in the PR description will auto-close an issue when the PR is merged.

Yes it should be. Closing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants