Feature Request: support for MySQL `MAX_EXECUTION_TIME` for query timeout

[timvaillancourt]

Feature Description

👋 Vitess,

Today it appears the query timeout functionality in vttablet query server (ie: --queryserver-config-query-timeout) kills slow running queries using a MySQL KILL <id> operation which I assume is done concurrently in another goroutine

Since (I believe) Oracle/Percona MySQL 5.6 the MAX_EXECUTION_TIME optimizer hint/comment was added to allow the MySQL server to kill a slow-running query at a specified number of milliseconds from within mysqld

This RFC is to propose/discuss a new query timeout behaviour where MAX_EXECUTION_TIME(milliseconds) is used to timeout a query at a certain time, instead of a KILL operation. This approach builds on an idea originally proposed by @demmer 🙇 .

To achieve this I believe this requires:

1. A way to opt-in to this new behaviour, perhaps --queryserver-config-query-timeout-method, options:
   • vttablet (default for now) - vttablet runs KILL in a new goroutine + connection
   • pushdown - vttablet lets MySQL handle query timeouts using MAX_EXECUTION_TIME optimizer hints
2. When enabled, rewriting the Vitess QUERY_TIMEOUT_MS query-directive to a MySQL SELECT /*+ MAX_EXECUTION_TIME(milliseconds) */ optimizer-hint comment
   • Disabling the KILL operation when this feature is used
3. When no QUERY_TIMEOUT_MS provided, add MAX_EXECUTION_TIME comment with --queryserver-config-query-timeout as the timeout
4. Ensuring the error returned from MySQL is compatible with the current one
   • Use query-killed error to update vttablet kill counter/stats
   • Real example: ERROR 3024 (HY000): Query execution was interrupted, maximum statement execution time exceeded

Thanks again @demmer for raising the idea this RFC is based on

Use Case(s)

This approach assumes that:

1. It is more efficient for mysqld to kill a query internally vs a remote/external operation
2. It is more efficient for vttablet to add a MAX_EXECUTION_TIME comment vs perform a KILL operation
3. MAX_EXECUTION_TIME is more reliable
   • Doesn't require an extra MySQL call (could fail)
   • Does not depend on vttablet (eg: a crash/restart)

These assumptions are theoretical and yet-to-be-proven

[deepthi]

This seems somewhat related #11429
Release notes for that PR

VTGate: Support query timeout --query-timeout

--query-timeout allows you to specify a timeout for queries. This timeout is applied to all queries.
It can be overridden by setting the query_timeout session variable.
Setting it as query comment directive with QUERY_TIMEOUT_MS will override other values.

[demmer]

Yes it's somewhat related.

The difference as I see is that PR still enforces this timeout with a golang context, i.e. it's up to the tablet to give up on the query and then kill it.

The idea here was that if mysql has an ability to do this natively, then rather than enforcing said timeout in golang, we could delegate it to mysql's own enforcement as that would be less overhead when the system is potentially already overloaded.

[jeremycole]

The idea here was that if mysql has an ability to do this natively, then rather than enforcing said timeout in golang, we could delegate it to mysql's own enforcement as that would be less overhead when the system is potentially already overloaded.

I am somewhat biased but I agree that max_execution_time should be used. It's a great feature! 😄

[timvaillancourt]

I'll take a shot at implementing this 👍. Some notes/questions from looking at the code and thinking about this more:

• Do we remove the context.Context timeout entirely (block) when MAX_EXECUTION_TIME() is used (SELECT only)?
  • Or another way: are there cases where we can't trust MySQL to kill the SELECT (and we should still have a client side timeout) - my gut says "probably" (stalled, coredumping?, etc) 🤔
  • The MySQL-side kill is best-effort as expected. In my testing on a pretty low-volume server, a kill can take about 50-250ms longer than expected. If a client-side timeout is kept it should maybe 500-1000ms+ greater than the timeout sent to MySQL
• MySQL respects the hint ONLY if the comment is in the 2nd field of the query, eg: SELECT /*+ MAX_EXECUTION_TIME(1234) */ * FROM something;
• The Vitess query-comment parsing logic I found focuses on comments at the "margins" of the query text
  • Support for get/set-ing comments at the 2nd field in SQL text may need to be added to:
    • Check if the hint is already set, if so, skip
    • Set the hint if this new, optional kill-method is enabled

[jfg956]

I will read this a few times before doing an exhaustive comment, but a few things already:

• I would not use the query hint, I would do a round-trip to the server from the vttablet setting the session variable. The reason for that is to still have the original query in the general log and in the p_s query digest table (a query without the hint is a different query from a query with the hint).

Have you consider implementing something like a query hint VT_MAX_EXEC_TIME ? This would be updated from vtgate to vttablet, and then from vttablet to MySQL if some buffering happen. Right now, if I set MAX_EXECUTION_TIME(5s) in my query client side and the query is buffered 3 seconds in the vttablet, it still has 5 seconds in MySQL while maybe it should only have 2.

If you implement VT_MAX_EXEC_TIME by setting the session variable, I would be very interested to extend this work to include a VT_LONG_QUERY_TIME that would set the long_query_time session variable before executing the query. This would only be useful for MySQL 5.7 as 8.0 has a way to do a query hint for this.

... to be completed about only working for SELECT so killing still needed for other query types ..., would still be good to have a VT_MAX_EXEC_TIME for these. Maybe VT_MAX_{SELECT,READ}_TIME and VT_MAX_WRITE_TIME (or something else instread of WRITE for including INSERT, UPDATE and DELETE (and REPLACE and maybe others I am forgetting).

Update 2023-07-07: for DMLs (INSERT, ...), we can optimistically maximize MySQL timeouts by setting innodb_lock_wait_timeout and others, while still having kill in case this fails or something else client side makes this times-out. Once such case could be COMMIT not finishing because of semi-sync, which cannot be killed, and this leads to the idea below.

If we implement vttablet wrapping-up MySQL commands with SET for max-exec-time, long-query-timeout and others, maybe we could also wrap all auto-commits in BEGIN/COMMIT. This way, we can try killing a non-commit statement and handle commit differently as commit cannot be killed in most cases. It could also allow better observability of what of an actual DML or the commit is taking time (right now, an auto-commit UPDATE will also have semi-sync delay in the UPDATE statement instead of the COMMIT, making things harder to observe).

Disabling the KILL operation when this feature is used

You probably should still kill a few ms after the timeout expires, just to be on the safe side.

It is more efficient for mysqld to kill a query internally vs a remote/external operation

Another good thing: you get a counter for query reaching MAX_EXECUTION_TIME, which is interesting observability.

Update 2023-07-07: Also, I think killed queries are not getting in the slow log, maybe queries timing-out ouge get there, which is an observability win.

The Vitess query-comment parsing logic [...]

It would be good to have this logic only happening in the vtgate, and some "meta" / deadline context being sent to the vttablet to avoid double parsing of queries and save some CPU.

Note: in the environment I work, we always add MAX_EXECUTION_TIME to queries, but this does not work well as it is only taken into account by MySQL, and is not updated along the way when some buffering is happening.

[harshit-gangal]

In Vitess we have multiple ways to set timeout and adding another with the logic explained above will make it more complex to understand.

We have

1. mysql_server_query_timeout - which set the query timeout on the context as soon the query is received using MySQL protocol.
2. query-timeout which set the query timeout on the context during execution of the plan.
3. query_timeout_ms comment directive used as an override to pt 2.
4. queryserver-config-query-timeout which is set at vttablet on the context and once expired also kills the running query on the MySQL with a kill statement.

The MAX_EXECUTION_TIME only works for SELECT queries the reason why we did not go with and added query_timeout_ms as a query hint and stayed with the kill approach.

If we go ahead and change things it will only work for SELECT and still have to use a different approach for DMLs, DDLs and other queries.

I think what we can do is instead of kill connection we can execute kill query in that case an open transaction will remain active.

[timvaillancourt]

Have you consider implementing something like a query hint VT_MAX_EXEC_TIME ? This would be updated from vtgate to vttablet, and then from vttablet to MySQL if some buffering happen. Right now, if I set MAX_EXECUTION_TIME(5s) in my query client side and the query is buffered 3 seconds in the vttablet, it still has 5 seconds in MySQL while maybe it should only have 2.

I haven't thought about that yet, no. I was thinking about a smaller change to vttablet only that lets MySQL kill queries instead of a kill statement in a new goroutine. My thinking is this could be an optional feature that adds the QUERY_TIMEOUT_MS (or flag value) as a MAX_EXECUTION_TIME MySQL comment transparently.

You probably should still kill a few ms after the timeout expires, just to be on the safe side.

Yeah, that's a good idea in case something doesn't work out 👍

The MAX_EXECUTION_TIME only works for SELECT queries the reason why we did not go with and added query_timeout_ms as a query hint and stayed with the kill approach.

If we go ahead and change things it will only work for SELECT and still have to use a different approach for DMLs, DDLs and other queries.

Right, my thinking is this would apply to SELECT only based on the QUERY_TIMEOUT_MS (or flag value)