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

Support for SET statements #2855

Closed
sougou opened this issue May 14, 2017 · 5 comments
Closed

Support for SET statements #2855

sougou opened this issue May 14, 2017 · 5 comments

Comments

@sougou
Copy link
Contributor

sougou commented May 14, 2017

#2724 mentioned that there's a need to support for arbitrary SET statements. Looking at the list here, many of those variables are pretty intrusive and likely to break Vitess. Just reserving a connection where you're allowed to set anything will not be sufficient.

So, I think we should instead support variables on a case-by-case basis. For some of them, special code may have to be written, like we had to do for autocommit.

Let's see if we can get the list built and addressed.

@bbeaudreault
Copy link
Contributor

@sjmudd has a need for SET max_execution_time=120000. I told him this would probably be easy to add. Thoughts?

Additionally, some SET commands may be nice to just optionally no-op.

@sjmudd
Copy link
Contributor

sjmudd commented May 23, 2017

I guess coming from a setup where we knew we had a persistent connection then it makes sense to use some of these settings. That said by setting them one connection may behave differently to another and that would not be what's expected. So sticky sessions would be quite nice to avoid this problem and possibly "reset state" when the app disconnects.

The other option might be (depending on the variables concerned) to accept the statement and ignore it, or give a warning. This allows for testing without breaking the app, but does not have the intended affect so might not be ideal but aprovides a stepping stone to play with vitess while the app is being adjusted.

@sjmudd
Copy link
Contributor

sjmudd commented May 23, 2017

But using something like the query hints in 5.7 seems better. https://dev.mysql.com/doc/refman/5.7/en/optimizer-hints.html#optimizer-hints-execution-time
as this works at the query level. Potentially vitess could also understand the hints and even apply them even if the underlying mysql (mariadb ?) doesn't understand them. Maybe too much work though ?

@sougou
Copy link
Contributor Author

sougou commented May 24, 2017

There is functionality overlap between this variable and the vttablet query killer. The difference is that the query killer works for all queries including DMLs. So, this gives us a few options:

  1. Re-interpret max_execution_time as applicable to all queries. If so, the implementation will just set the timeout on the query killer.
  2. Perform more complex logic: Apply the timeout only if it's a SELECT query.
  3. Keep the two functionalities orthogonal, and pass this timeout to the connection that would execute the statement, and reset it before returning to the pool.
  4. Go the other way around: If it's a SELECT query, send the current timeout as max_execution_time to MySQL.

Option 1 would be the most elegant, but it's not 100% backward compatible.

2 & 3 differ from the fact that vttablet issues a KILL, which also kills the connection; It results in a reconnect later. But it's reliable and proven to work well in production.

To consider option 4, we have to see if max_execution_time is better than a KILL:

  • How reliable is it?
  • Does it kill the connection also?
  • What error do you get on killed queries?
  • Do all versions of mysql support it?

@sougou
Copy link
Contributor Author

sougou commented Sep 3, 2017

The initial use cases that this bug needed have been addressed. So, I'll close this. Going forward, we can create specific ones as needed.

@sougou sougou closed this as completed Sep 3, 2017
frouioui pushed a commit to planetscale/vitess that referenced this issue Nov 21, 2023
…s to PRS (vitessio#2862)

* backport of 2855

* feat: fix e2e tests

Signed-off-by: Manan Gupta <[email protected]>

* feat: fix e2e tests again

Signed-off-by: Manan Gupta <[email protected]>

---------

Signed-off-by: Manan Gupta <[email protected]>
Co-authored-by: Manan Gupta <[email protected]>
frouioui pushed a commit to planetscale/vitess that referenced this issue Mar 26, 2024
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