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
sql: avoid stack overflow on deeply nested ASTs #9117
Conversation
I still need to check that this actually fixes the issue, so just a draft for now. |
Testing locally the query in 9103 still produces a crash. Not convinced it's the same stack crash (we're investigating what might be another). |
@mjibson did you test in release mode? In debug mode,
|
@philip-stoev perhaps you could take a stab at tweaking the tests or the recursion limit constant here? I'm not going to have a chance to look at this for a bit. |
f5a0097
to
1499132
Compare
Nope, was in debug. |
In the SQL planner, use the same technique as the SQL parser for automatically growing the stack when presented with a deeply nested AST. The stack-checking code is generalized and moved to a new `ore::stack` module. Fix MaterializeInc#9103.
1499132
to
394e19d
Compare
Ok, great, this should be ready to go now. @philip-stoev the limits test now passes, and if you like you could introduce an even larger number of additions and verify that you get a smooth error message rather than a crash. I'm also pretty confident that the infrastructure in |
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.
The only unfortunate thing about this is that using checked recursion probably impedes TCO even in cases where the compiler could have made it work.
Probably not an issue in any of the cases we’re using it in, though.
394e19d
to
c8c5069
Compare
With 1000 items I get an actual result out of this query. With 1100 items I get a stack overflow elsewhere. I am unable to get the "recursion limit exceeded" error.
I am going to open a separate ticket for this (as I have for several other stack overflows elsewhere). |
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.
As the particular crash the issue was about is now fixed and the test passes, I think this PR is good to go. I am going to open a follow-up ticket about the sql::plan::transform_ast::Desugarer::visit_expr_mut_internal
crash.
For the optimizer, I had filed #9000 to use stacker. I added a comment in that ticket to use the newly added stack management utilities. |
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.
LGTM, I will then it to fix similar issues in the optimizer (see #9000).
/// | ||
/// If the recursion limit for the recursion guard returned by | ||
/// [`CheckedRecursion::recursion_guard`] has been reached, returns a | ||
/// `RecursionLimitError`. Otherwise, it will call `f`, possibly growing the |
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.
nit: strictly speaking, returns a RecursionLimitError
converted to E
.
Thanks for the reviews, everyone! Let the games of whack a mole begin. |
In the SQL planner, use the same technique as the SQL parser for
automatically growing the stack when presented with a deeply nested AST.
The stack-checking code is generalized and moved to a new
ore::stack
module.
Fix #9103.
Motivation
This PR fixes a recognized bug.
Checklist
user-facing behavior changes.