-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add engine primitive to handle query timeout #16619
Conversation
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
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
|
…meout in cache key Signed-off-by: Manan Gupta <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16619 +/- ##
========================================
Coverage 68.87% 68.87%
========================================
Files 1562 1563 +1
Lines 200624 200729 +105
========================================
+ Hits 138179 138259 +80
- Misses 62445 62470 +25 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Manan Gupta <[email protected]>
Hello! 👋 This Pull Request is now handled by arewefastyet. The current HEAD and future commits will be benchmarked. You can find the performance comparison on the arewefastyet website. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to benchmark the change as it is adding go routines for each run.
This would not show up anything as query timeout needs to be set to use the new engine primitive |
Description
As pointed out in #16624, the query timeout is not handled properly for the entire execution of the query. This PR fixes this issue. The approach is to introduce a new engine primitive that checks for the timeout and then runs the remainder of the execution as part of a go routine. If the execution takes longer than the timeout, the primitive returns an error.
For creating a plan that has the query-timeout specified on plan time, we are using the session setting of query timeout as well as part of the planning. This necessitates the need to add it to the plan cache key as well, since we don't want to use a plan with one query timeout even when the query timeout for the session changes.
Related Issue(s)
Checklist
Deployment Notes