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

Fixes to MySQL pipelining #1236

Merged
merged 3 commits into from
Sep 20, 2022

Conversation

tsegismont
Copy link
Contributor

Fixes #1234, fixes #1235

@tsegismont
Copy link
Contributor Author

@BillyYccc in #873 you made a comment about ordering issues, can you elaborate? I've early closing in this PR and enabled pipeling for testing, it seems the suite passes. Did you notice issues in cases not tested here?

@BillyYccc
Copy link
Member

The client will automatically send prepared statement close command after executing query only if the client uses a one-shot prepared query (i.e. using API client#preparedQuery with statement cache disabled)

Before supporting pipelining we have an ordering issue in the close statement implementation, since the network protocol states the statement close command is a fire & forget command(see https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_com_stmt_close.html)
when we send a close statement command we immediately pop it from the request queue, this works fine when the pipelining limit is 1 however it will break the request queue order in pipelining mode

pipelining limit = 1 : prep | exec | close | prep | exec | close

pipelining limit > 1 encoding order: prep exec close prep exec close
pipelining limit > 1 decoding order: prep close(* decoding immediately after sending close, the exec resp is not ready) exec prep exec close

(you can check the PR changes in https://github.com/eclipse-vertx/vertx-sql-client/pull/1168/files#diff-4c41cc49d53e95241af08ca5af42809a1c616304d4971c84ae3bf58f1123ef61L32 and https://github.com/eclipse-vertx/vertx-sql-client/pull/1168/files#diff-4c41cc49d53e95241af08ca5af42809a1c616304d4971c84ae3bf58f1123ef61L32)

I think we cannot simply remove the auto closing statement code in this PR otherwise it will leave the statement leaked, I wonder if the issue happens when using the client with a plain MySQL server or only with the SQLProxy, can you share a reproducer?

@tsegismont
Copy link
Contributor Author

I think we cannot simply remove the auto closing statement code in this PR otherwise it will leave the statement leaked,

I believe it doesn't, the statement is closed when all results have been decoded:

@Override
protected void handleAllResultsetDecodingCompleted() {
closePreparedStatement();
super.handleAllResultsetDecodingCompleted();
}
private void closePreparedStatement() {
MySQLPreparedStatement ps = (MySQLPreparedStatement) this.cmd.ps;
if (ps.closeAfterUsage) {
sendCloseStatementCommand(ps);
}
}

I wonder if the issue happens when using the client with a plain MySQL server or only with the SQLProxy, can you share a reproducer?

It happens only with ProxySQL. It was originally reported in quarkusio/quarkus#27386. I created a tool to setup Percona cluster with ProxySQL in front https://github.com/tsegismont/pxc-with-proxysql

Note that in this PR there is a change that enables pipelining in MySQLPipeliningTest and MySQLPipeliningQueryTest. Before that change it wasn't working because the connect options copy constructor ignored pipeliningLimit.

I have verified with Wireshark that pipelining is actually working with this PR.

Copy link
Member

@BillyYccc BillyYccc left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@vietj vietj left a comment

Choose a reason for hiding this comment

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

Need a test if we can afford it.

@tsegismont
Copy link
Contributor Author

@vietj yes, there's a test in the making ;-)

Signed-off-by: Thomas Segismont <[email protected]>
The proxy might not support it.

Signed-off-by: Thomas Segismont <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants