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

Placeholders with multiple statements in binary protocol do not work #916

Closed
CAFxX opened this issue Jan 22, 2019 · 9 comments
Closed

Placeholders with multiple statements in binary protocol do not work #916

CAFxX opened this issue Jan 22, 2019 · 9 comments

Comments

@CAFxX
Copy link

CAFxX commented Jan 22, 2019

Issue description

It seems that using multiple statements and placeholders is not fully supported yet.

From a cursory look this seems to me very similar to #769. That ticket is closed with the reason that the binary protocol does not support multi resultsets, but as I wrote in #769 (comment) that should not be the case. It was suggested by @methane that #769 is a different issue, so I opened this issue instead.

Example code

The following code unexpectedly returns error during db.Query. If the commented-out db.Query line is used instead, or if interpolateParams=true is used in the DSN, then the query succeeds.

package main

import (
        "database/sql"
        "fmt"
        _ "github.com/go-sql-driver/mysql"
)

func main() {
    db, err := sql.Open("mysql", "root:root@tcp(127.0.0.1:3306)/test?multiStatements=true")
    if err != nil {
        panic(err)
    }
    defer db.Close()
    
    // rows, err := db.Query("SET @v = 1; SELECT @v; SELECT @v+1;") // this works
    rows, err := db.Query("SET @v = ?; SELECT @v; SELECT @v+1;", 1) // this does not work

    if err != nil {
        panic(err)
    }
    defer rows.Close()

    var r1, r2 int

    if !rows.Next() {
        panic("missing row")
    }
    rows.Scan(&r1)

    if !rows.NextResultSet() {
        panic("missing result set 2")
    }
    if !rows.Next() {
        panic("missing row")
    }
    rows.Scan(&r2)

    fmt.Println(r1, r2) // 1 2
}

Error log

panic: Error 1064: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'SELECT @v; SELECT @v+1' at line 1

Configuration

Driver version (or git SHA):
c45f530

Go version: run go version in your console
1.11.2

Server version: E.g. MySQL 5.6, MariaDB 10.0.20
MySQL 5.7

Server OS: E.g. Debian 8.1 (Jessie), Windows 10
Ubuntu 16.04

@CAFxX CAFxX changed the title Placeholders with multiple statements Placeholders with multiple statements in binary protocol do not work Jan 22, 2019
@peergynt
Copy link
Contributor

@CAFxX would the query above even use the binary protocol? This looks like a straight text query.
I believe that the binary protocol would be used with a prepared statement.

@CAFxX
Copy link
Author

CAFxX commented Jan 22, 2019

@peergynt my understanding is that the binary protocol is used automatically by the driver when you use placeholders and you don't set interpolateParams=true (at least, that's my understanding from reading #769 (comment)):

You need to use interpolateParams=true to use text protocol, at your own risk.

@peergynt
Copy link
Contributor

@CAFxX I believe that the issue here is that the driver uses a prepared statement under the hood when you use the placeholder but a prepared statement with multiple statements is not valid in MySQL:
https://dev.mysql.com/doc/refman/8.0/en/prepare.html

The text must represent a single statement, not multiple statements.

@methane
Copy link
Member

methane commented Jan 22, 2019

    rows, err := db.Query("SET @v = ?; SELECT @v; SELECT @v+1;", 1) // this does not work

This is duplicate of #769.
Both of #769 and this are about "multi statement", not "multi resultset".

"multi statement" depends on "multi resultset". But "multi resultset" doesn't depend on "multi statement".

Note that there are CLIENT_MULTI_RESULTS, CLIENT_PS_MULTI_RESULTS, CLIENT_MULTI_STATEMENTS, but there isn't CLIENT_PS_MULTI_STATEMENT.
https://dev.mysql.com/doc/internals/en/capability-flags.html

@methane methane closed this as completed Jan 22, 2019
@methane
Copy link
Member

methane commented Jan 23, 2019

I think stored procedure is required to return "multi resultset" from "single statement".
But I never use stored procedure. I am not interested in it.

If you really need it, please try to use it.

@CAFxX
Copy link
Author

CAFxX commented Jan 23, 2019

@methane I think it would be less surprising if the driver fell back to the text protocol automatically when its choice of protocol fails (as in this case, where the driver's internal choice of the binary protocol causes a valid MySQL query to fail).

It is unfortunate that this use case is currently not working, as multiple statements are very effective at slashing (multi)query latency.

@methane
Copy link
Member

methane commented Jan 23, 2019

It is unfortunate that this use case is currently not working, as multiple statements are very effective at slashing (multi)query latency.

You can use interpolateParams=true.

@CAFxX
Copy link
Author

CAFxX commented Jan 23, 2019

I would, but does this still apply?

You need to use interpolateParams=true to use text protocol, at your own risk.

If that configuration is insecure then I guess it shouldn't be used, right? If that's the case, it means the use case is not supported, correct?


On a completely unrelated note, I just realized we are just a few floors apart, as I'm also working in Roppongi. 🤣

@methane
Copy link
Member

methane commented Jan 23, 2019

If that configuration is insecure then I guess it shouldn't be used, right?

I think it's secure as far as it is used correctly. If you find security hole, file an issue.
But we can not write down all difference between PS and interpolateParams.
For example, I never tried SELECT "foo ? bar" on PS. In case of interpolateParams, ? is treated as placeholder.

I don't say you should, or should not use it. You should think it by your brain.

If that's the case, it means the use case is not supported, correct?

If you determined to not use it, correct. MySQL doesn't support multi statement in prepared statement. We can't support what MySQL doesn't support.

BTW, new MySQL protocol (X protocol) support pipelining. It works like batching multi statements in one query. But there are no production ready Go driver for the protocol.


On a completely unrelated note, I just realized we are just a few floors apart, as I'm also working in Roppongi. 🤣

Ah, I use Mercari well. There are some ex-KLab in your company. Mercari is one of the best tech companies in Japan.
I believe there are some Go and MySQL experts in your office.

Additionally, there should be some PHP experts in your office too. The interpolateParams is very similar to "PDO::ATTR_EMULATE_PREPARES" in PHP. ref

You can discuss about interpolateParams with experts in your office.

cenkalti added a commit to cenkalti/dalga that referenced this issue Aug 20, 2020
* Remove SKIP LOCKED from Front() to support MariaDB.

* Make the SKIP LOCKED optimization configurable.

* Added basic ability to mock time.

* Client with Schedule method.

* Full client.

* Refactor test.

* Added working test for the client.

* Export the clock.

* Separate out the UpdateInstance call into 2 prepared statements, due to go-sql-driver/mysql#916

* Added TestAddJob for table.

* Fully removed dropTables

* Update internal/table/table.go

* Update internal/table/table_test.go

Co-authored-by: Cenk Altı <cenkalti@gmail.com>
cenkalti pushed a commit to cenkalti/dalga that referenced this issue Aug 24, 2020
* Remove SKIP LOCKED from Front() to support MariaDB.

* Make the SKIP LOCKED optimization configurable.

* Added basic ability to mock time.

* Client with Schedule method.

* Full client.

* Refactor test.

* Added working test for the client.

* Export the clock.

* Separate out the UpdateInstance call into 2 prepared statements, due to go-sql-driver/mysql#916

* Added TestAddJob for table.

* Fully removed dropTables

* Compiling, working on tests.

* Cast injected times to DATETIME to prevent loss of time.

* Revert extraneous logging.

* Added iso8601 schedule tests, not all passing yet.

* Recurring tests pass.

* All tests pass together.

* Speed up tests.

* Timezone support passing tests.

* Added UTC test.

* Fix deadlocks.

* Added failing test for AddJob returning a timezoned job.

* Fixed failing test.

* Added passing test to ensure Front returns timezoned job.

* Added failing test to ensure Get returns a timezoned job.

* Fixed failing test.

* Removed extraneous DB call.

* Style and consistency cleanup.

* Add note about UseClock as per #13
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

3 participants