Skip to content

Conversation

@neoeinstein
Copy link
Contributor

@neoeinstein neoeinstein commented Jun 20, 2020

Joins the MySQL connection setup statements into a single statement to prevent issues with ProxySQL, which requires special handling of connections that might be shared between multiple backends.

Original:

Splits the MySQL connection setup statements into discrete executions to
prevent issues with proxies, such as ProxySQL, which may need to balance
and share connections between different backends.

Fixes #422

Signed-off-by: Marcus Griep marcus@griep.us

@mehcode
Copy link
Member

mehcode commented Jun 20, 2020

That's unfortunate. As these are ran on every new connection I'd prefer to keep the single execute when we can. Can you conditionally do this based on the server supporting multiple statements?

Something like

if conn.stream.capabilities.contains(Capabilities::MULTI_STATEMENTS) {
  // can send all statements at once
} else {
  // need to send them one at a time
}

@neoeinstein
Copy link
Contributor Author

I'll check to see what capabilities are being returned.

@neoeinstein neoeinstein force-pushed the 422-split-set-statement-execution branch from 98a918a to e2f18d7 Compare June 20, 2020 14:28
@neoeinstein
Copy link
Contributor Author

I threw in a log statement to check out what capabilities were being advertised:

Jun 20 10:13:02.571 INFO sqlx_core::mysql::connection: Capabilities: FOUND_ROWS | CONNECT_WITH_DB | PROTOCOL_41 | TRANSACTIONS | SECURE_CONNECTION | MULTI_STATEMENTS | MULTI_RESULTS | PS_MULTI_RESULTS | PLUGIN_AUTH

With MULTI_STATEMENTS in there, that won't work, and in fact the issue is not with multi-statements in general, but with SETs in multi-statements.

I considered adding a new option to the MySQL builder, but I think the more prudent course would be to have a build feature, so I've updated the PR to use that instead.

@neoeinstein neoeinstein changed the title fix(mysql): split connection setup commands feat(mysql): add proxysql-compat feature Jun 20, 2020
@mehcode
Copy link
Member

mehcode commented Jun 21, 2020

I did some digging. It does look like ProxySQL is mostly behaving correctly. There just isn't a capability to show "hey we support multiple non-CRUD statements".

However SET supports multiple assignment within one expression because of that. SET x=y, z=w, a=2.

See: https://dev.mysql.com/doc/refman/5.6/en/set-variable.html#set-variable-multiple-assignments

Can you try tweaking the statement to be formed like that and see if ProxySQL is okay with it?

@neoeinstein
Copy link
Contributor Author

I did some tweaks as recommended, and am currently doing a long-run to verify that I'm not getting any errors.

I did notice that, if I don't include one semicolon at the end, ProxySQL didn't seem to be preserving the session variables (the sql_mode fell empty, and time_zone stayed on SYSTEM). I ran a few test queries, like SELECT DATE('0000-00-00') and SELECT CURRENT_TIMESTAMP() to see whether this was just an artifact or ProxySQL actually not preserving the variables. These tests confirmed that this wasn't an artifact.

Including the semicolon at the end triggers ProxySQL to still consider it a "multi-select" statement and lock the connection, but it does properly preserve the session variables and function as expected. This does cause a warning to be emitted by ProxySQL, but since there isn't actually another statement after the semicolon, I'm not getting a "bad SQL" error back.

I'll update the PR with the new single-statement connection setup.

Joins the MySQL connection setup statements into a single statement to
prevent issues with ProxySQL, which requires special handling of
connections that might be shared between multiple backends.

Fixes launchbadge#422

Signed-off-by: Marcus Griep <marcus@griep.us>
@neoeinstein neoeinstein force-pushed the 422-split-set-statement-execution branch from e2f18d7 to 274826e Compare June 21, 2020 15:34
@neoeinstein neoeinstein changed the title feat(mysql): add proxysql-compat feature fix(mysql): improve compatibility with ProxySQL Jun 21, 2020
@mehcode mehcode merged commit 816ea65 into launchbadge:master Jun 27, 2020
@mehcode
Copy link
Member

mehcode commented Jun 27, 2020

Thank you for investigating and fixing this. This seems to improve support for older versions of MySQL as well. 👍

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.

[mysql] MySQL with ProxySQL in front reports errors and stalls

2 participants