fix: drop a deep BinaryExpr chain iteratively to avoid a stack overflow

[mdashti]

Which issue does this close?

None filed.

Rationale for this change

This PR makes a deep BinaryExpr chain drop iteratively instead of recursively.

A left-associative chain like a OR b OR ... parses with shallow recursion (the left side accumulates in a loop), so recursion_limit never bounds its length. sql_expr_to_logical_expr builds it iteratively (the stack machine from #1444), so planning is fine. But the resulting Expr is as deep as the chain, and its recursive Drop through the boxed children overflows the stack. The tree-walk passes are #[recursive]-protected, so the Drop is the one recursion the protection can't reach.

impl Drop for Expr isn't viable: it forbids moving fields out of Expr, which map_children and the rewriters do by value (E0509).

What changes are included in this PR?

Wrap the recursive BinaryExpr.left/right edges in a BoxedExpr newtype whose Drop detaches each node's children into a heap work-list before the node drops. Expr and BinaryExpr keep no Drop, so the by-value destructuring elsewhere is untouched, and BinaryExpr::new keeps its Box<Expr> signature. The unbounded-depth edges are only these chains; other nesting is capped by recursion_limit.

The rest of the diff is mechanical: a few consumers move BinaryExpr operands by value, so each *left becomes *left.into_inner().

Are these changes tested?

Yes. deep_binary_expr_chain_drops_without_overflowing builds a 200000-deep OR chain and drops it on a normal stack. The existing test_stack_overflow cases cover the SQL planning path, and this fix is what lets them pass without a large-stack workaround.

Are there any user-facing changes?

No.

[adriangb]

i'm curious if this is a test-only concern or also a real production issue. should we implement an iterative Drop?

[adriangb]

doesn't this defeat the purpose of the test? can we keep the smaller stack size for the planning (which is what we are trying to ensure is iterative) but move the value into a large stack thread to run drop()?

[mdashti]

Thanks. You're right. It's a real production issue, so I went with the iterative Drop. The BinaryExpr children hold a BoxedExpr whose Drop tears a deep chain down off the call stack (39e458c5c), so the original test_stack_overflow cases pass with no stack trick. impl Drop for Expr wasn't viable (E0509 from the by-value destructuring in map_children), so the newtype wraps just the recursive edges. Repurposed the PR for it.

[mdashti]

@adriangb Thanks for the comment.

[mdashti]

Thanks. You're right. It's a real production issue, so I went with the iterative Drop. The BinaryExpr children hold a BoxedExpr whose Drop tears a deep chain down off the call stack (39e458c5c), so the original test_stack_overflow cases pass with no stack trick. impl Drop for Expr wasn't viable (E0509 from the by-value destructuring in map_children), so the newtype wraps just the recursive edges. Repurposed the PR for it.

[mdashti]

The deep-unparse regression test now runs in its own test binary, so the suite is stable. The underlying cross-thread race on recursive's process-global red zone (what the in-process test churn exposed) is tracked separately in #23246 and left out of scope here.

[adriangb]

This seems right to me, I could not figure out any way to make this simpler or less churn. @xudong963 since you work on the recursion guards originally I wonder if you can spot any alternative to making this change?