Use Kill Query for Non-Transaction Query Execution and Update Query Timeout / Cancelled Error Message #15694
Conversation
harshit-gangal
added
Type: Enhancement
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
harshit-gangal
force-pushed
the
dbconn-kill
branch
3 times, most recently
from
9bee3bd to
7843728
Compare
harshit-gangal
removed
NeedsWebsiteDocsUpdate
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15694 +/- ##
==========================================
+ Coverage 68.40% 68.42% +0.01%
==========================================
Files 1556 1556
Lines 195121 195490 +369
==========================================
+ Hits 133479 133764 +285
- Misses 61642 61726 +84 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
harshit-gangal
force-pushed
the
dbconn-kill
branch
from
855f9fc to
d1d8d2f
Compare
Signed-off-by: Harshit Gangal <[email protected]>
harshit-gangal
force-pushed
the
dbconn-kill
branch
3 times, most recently
from
15b76f1 to
a43cb4c
Compare
…al query Signed-off-by: Harshit Gangal <[email protected]>
harshit-gangal
force-pushed
the
dbconn-kill
branch
from
a43cb4c to
ea97265
Compare
Signed-off-by: Harshit Gangal <[email protected]>
harshit-gangal
requested review from
shlomi-noach,
rohit-nayak-ps,
mattlord and
deepthi
as code owners
harshit-gangal
changed the title
Signed-off-by: Harshit Gangal <[email protected]>
| // we can't safely kill a query in a transaction, we need to kill the connection | ||
| _ = dbc.Kill(errMsg, time.Since(now)) | ||
| } else { | ||
| _ = dbc.KillQuery(errMsg, time.Since(now)) |
There was a problem hiding this comment.
This means that we attempt to kill the connection/query. The comment says that we do, but w/o checking the error we don't know if it was successful do we? IMO we should return the error here and check it at the call sites:
return dbc.Kill(errMsg, time.Since(now))
} else {
return dbc.KillQuery(errMsg, time.Since(now))
It looks like dbc.KillWithContext() can fail for a number of reasons and we shouldn't assume the kill succeeded, should we?
There was a problem hiding this comment.
If for any reason kill fails, we log the error message. Kill itself is not directly in the query execution path. But, it kills any other running query, which will receive the error message if the kill succeeds.
| sql := fmt.Sprintf("kill query %d", dbc.conn.ID()) | ||
| go func() { | ||
| _, err := killConn.Conn.ExecuteFetch(sql, -1, false) | ||
| ch <- err |
There was a problem hiding this comment.
Why not close the channel here as we did elsewhere?
There was a problem hiding this comment.
I added it in exec and streamexec, there is no reason we cannot add it here.
Added now.
| }() | ||
| testContextError(t, ctx, exec, | ||
| "(errno 1317) (sqlstate 70100): Query execution was interrupted", | ||
| 150*time.Millisecond) |
There was a problem hiding this comment.
I think that this test is likely to be flaky in the CI, given these very small time windows. I'd recommend extending all of them at least a bit.
There was a problem hiding this comment.
I have run these tests multiple times and have not observed any flakiness. I will increase them if they come out as flaky.
Signed-off-by: Harshit Gangal <[email protected]>
|
LGTM |
Description
This PR changes the error message returned on the context error.
On context timeout:
ERROR 3024 (HY000): Query execution was interrupted, maximum statement execution time exceededOn context cancelled:
ERROR 1317 (70100): Query execution was interruptedThis PR also executed
kill queryinstead ofkill connectionon context error to reduce the connection churn. This is only performed on non-transactional queries.Related Issue(s)
Checklist