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

Add support for the MySQL prepare command protocol #3862

Merged
merged 8 commits into from
Aug 9, 2019

Conversation

dcadevil
Copy link
Contributor

No description provided.

@dcadevil dcadevil changed the title Added support for the MySQL prepare command protocol Add support for the MySQL prepare command protocol Apr 27, 2018
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you able to get this working? According to https://dev.mysql.com/doc/internals/en/prepared-statements.html, this is an entirely different protocol end-to-end. This means that the results have to be returned differently. I don't see any code for those parts.

We considered implementing this protocol. However, given that the Prepare protocol never gained popularity, we decided not to support it.

@dcadevil
Copy link
Contributor Author

dcadevil commented May 9, 2018

image
I tested it and it works correctly.

@sougou
Copy link
Contributor

sougou commented May 9, 2018

Ahh ok. I see now. Github didn't show me all the changes...
This is very good work. However, we need more tests, lots more. All the conversion and protocol code needs to be covered. If you look at the tests for the regular protocol, equal amount is needed for the prepared statement protocol.

@dcadevil
Copy link
Contributor Author

dcadevil commented May 9, 2018

OK, I'll add test case.

@linuxgood1230
Copy link

Waiting for you merge to master
I want test.

@dcadevil
Copy link
Contributor Author

dcadevil commented Jun 1, 2018

@linuxgood1230 I'm very busy recently. After a few days I'll write test case. Thanks for your attention.

Signed-off-by: dcadevil <dcadevil@126.com>
Signed-off-by: dcadevil <dcadevil@126.com>
Signed-off-by: dcadevil <dcadevil@126.com>
Signed-off-by: dcadevil <dcadevil@126.com>
Signed-off-by: dcadevil <dcadevil@126.com>
Signed-off-by: dcadevil <dcadevil@126.com>
Signed-off-by: dcadevil <dcadevil@126.com>
@deepthi
Copy link
Member

deepthi commented Jun 10, 2019

Tests to be added:

  1. go/mysql/query_test.go

    • TestComPrepare
    • TestComStmtExecute
    • TestStmtSendLongData
    • TestComStmtReset
    • TestComStmtClose
  2. go/mysql/server_test.go

    • ComPrepare
    • ComStmtExecute
  3. go/vt/vtgate/executor_test.go

  4. go/vt/vtgate/plugin_mysql_server_test.go

    • ComPrepare
    • ComStmtExecute
  5. go/vt/vtqueryserver/plugin_mysql_server_test.go

    • ComPrepare
    • ComStmtExecute
  6. go/vt/vtgate/vtgate_test.go

  7. go/vt/vtgate/endtoend

@deepthi
Copy link
Member

deepthi commented Jun 10, 2019

@dcadevil I'm planning to write the testcases so that we can get this PR merged.

@dcadevil
Copy link
Contributor Author

@deepthi Thank you very much, if you need any help, please let me know.

@deepthi
Copy link
Member

deepthi commented Jun 17, 2019

@deepthi Thank you very much, if you need any help, please let me know.

hi @dcadevil, can you put in some examples of how this feature is intended to be used?
basically example syntax of prepare statement, and execute statement with bind variables.

Also, what client are you using to connect to vitess? The stock mysql client doesn't seem to work:

mysql> prepare my_stmt from 'select * from product';
ERROR 1105 (HY000): vtgate: http://localhost:15001/: unrecognized statement: prepare my_stmt from 'select * from product'

@dcadevil
Copy link
Contributor Author

hi, @deepthi, the MySQL prepare protocol implemented here is not the prepare in the SQL syntax.
You can refer to the this link: https://dev.mysql.com/doc/internals/en/prepared-statements.html

You can use java's jdbc driver client (note: useServerPrepStmts=true), or golang's go-mysql-driver(https://github.com/go-sql-driver/mysql) to test.

The basically example by golang can be found here: https://github.com/go-sql-driver/mysql/wiki/Examples

@saifalharthi
Copy link
Contributor

saifalharthi commented Jul 15, 2019

@dcadevil I am writing the unit tests for this PR and I am learning more about the MySQL protocol. I am trying to write tests for:

  • TestComStmtExecute
  • TestComStmtReset
    in go/mysql/query_test.go

and I am following the pattern of the tests. I noticed there are parse functions associated with them but no write functions. I am not very familiar with MySQL protocol but is there an example you can give to help test those functions properly?

Also, parseComStmtExecute fails, and it seems to not map well with the PrepreData struct. So, I am expecting that it expects a packet with different inputs. Can you give me an example that works with that function?

@dcadevil
Copy link
Contributor Author

@saifalharthi, writeBinaryRows function can be regarded as the write function of parseComStmtExecute function. In fact, this function is used to transform the execution result into MySQL protocol format result after ComStmtExecute execution.

In addition, it should be noted that the parameters of the parseComStmtExecute function contain a map of prepareData, the key of which is the statementID, which contains the statement and the number of parameters. So to use the parseComStmtExecute function, you need to set up the map.

The setting of another data parameter is rather complicated. I recommend using JDBC or golang's MySQL client driver to execute prepared statement program to get it. Of course, it is a bit cumbersome and complicated to do so.

@sougou sougou merged commit 2393ff7 into vitessio:master Aug 9, 2019
@tirsen
Copy link
Collaborator

tirsen commented Aug 10, 2019

I think this might solve our long standing issue with using the JDBC MySQL driver and Vitess: #4100

Packet 20/0x16 is COM_PREPARE_STMT which is what that complains about. I think the MySQL JDBC driver always uses prepare statements.

Checking now.

@@ -153,6 +153,24 @@ const (
// ComBinlogDump is COM_BINLOG_DUMP.
ComBinlogDump = 0x12

// ComPrepare is COM_PREPARE.
ComPrepare = 0x16
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is called COM_STMT_PREPARE in the MySQL codebase: https://dev.mysql.com/doc/internals/en/com-stmt-prepare.html

@tirsen
Copy link
Collaborator

tirsen commented Aug 10, 2019

This is really amazing and thanks for all the effort you've put into this.

I'm trying this out in one of our apps and I'm running into this issue:
#5075

Something wrong with the string encoding?

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

Successfully merging this pull request may close these issues.

6 participants