fix infinite recursion during cte self-reference #1223
Conversation
sql/analyzer/resolve_ctes.go
Outdated
| } | ||
|
|
||
| // look for self references in non-recursive cte | ||
| subquery, ok := cte.(*plan.SubqueryAlias) |
There was a problem hiding this comment.
explicitly cast, we don't want to accidentally ship code that doesn't panic here
sql/analyzer/resolve_ctes.go
Outdated
|
|
||
| // TODO: just compare the tables themselves? | ||
| hasSelfReference := false | ||
| transform.Inspect(subquery.Child, func(node sql.Node) bool { |
There was a problem hiding this comment.
I would prefer we run validation separately from tree mutation. So maybe its own function, validateCtes, and then call it at the top whenever we find a with block?
sql/analyzer/resolve_ctes.go
Outdated
| @@ -24,7 +24,7 @@ import ( | |||
| "github.com/dolthub/go-mysql-server/sql/transform" | |||
| ) | |||
|
|
|||
| const maxCteDepth = 5 | |||
| const maxCteDepth = 10 | |||
There was a problem hiding this comment.
depth takes on a new meaning here. Afaik if someone using the previous version of the code had one CTE top level reference once in a 10th level nested subquery, they will experience a new error? If we increment depth only when recursing in the CTE case would it preserve the previous behavior?
There was a problem hiding this comment.
Might seem niche, but we have several users that dip into ~20 table operations. Hippocratic rule of "do no harm" is relevant.
sql/analyzer/resolve_ctes.go
Outdated
|
|
||
| var newChildren []sql.Node | ||
| for _, child := range n.Children() { | ||
| newChild, _, err := resolveCtesInNode(ctx, a, child, scope, ctes, depth, sel) |
There was a problem hiding this comment.
In general it is important to know that we can use child deltas to avoid building a new node if none of the children changed. We already do not do this everywhere, but if we did we could avoid expensive tree comparisons at the top-level of the analyzer.
Changes:
Fix for: