Qualify macros only when called, and throw error on recursive macros

[lnkuiper]

Fixes #8141

[Mytherin]

Thanks for the PR! Looks good - one more comment:

[Mytherin]

I think that this will not work with TABLE macros, correct? i.e. this will still fail:

create macro m1(a) as a+1;
create macro m2(a) as table select m1(a);
create or replace macro m1(a) as (from m2(42));
select m1(42);

I think it might make sense to have an expression recursion limit in the binder similar to what we have in the transformer (look for StackCheck). Macros can also be used to circumvent the expression depth limit otherwise which can lead to a stack overflow.

[lnkuiper]

I've implemented StackCheck in ExpressionBinder, seems to work well. I've set the maximum depth to 128. With higher depths, we get stack overflows instead. I hope 128 is OK.

Also, I found a bug unrelated (I think) to this PR:

$ duckdb
-- Loading resources from /Users/laurens/.duckdbrc
v0.8.1-dev1402 aada97ec5f
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
D drop macro sum;
ABORT THROWN BY INTERNAL EXCEPTION: Database "system" not found
^C[1]    53269 abort (core dumped)  duckdb

[Alex-Monahan]

I think Python has a 10,000 level default on the stack and it can be edited with a parameter, in case that is helpful!

[Mytherin]

I've implemented StackCheck in ExpressionBinder, seems to work well. I've set the maximum depth to 128. With higher depths, we get stack overflows instead. I hope 128 is OK.

Also, I found a bug unrelated (I think) to this PR:

$ duckdb
-- Loading resources from /Users/laurens/.duckdbrc
v0.8.1-dev1402 aada97ec5f
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
D drop macro sum;
ABORT THROWN BY INTERNAL EXCEPTION: Database "system" not found
^C[1]    53269 abort (core dumped)  duckdb

Could you open an issue with that bug?

[lnkuiper]

Could you open an issue with that bug?

Done #8287

I've also investigated why I could only set the limit to 128, and it's because there's a lot of recursive calls in the Binder, especially when binding table macro's. This is one recursive binding iteration:

ExpressionBinder::BindExpression
BaseSelectBinder::BindExpression
ExpressionBinder::BindMacro
ExpressionBinder::BindExpression
ExpressionBinder::BindExpression
BaseSelectBinder::BindExpression
ExpressionBinder::Bind
ExpressionBinder::Bind
Binder::BindSelectNode
Binder::BindNode
Binder::BindNode
Binder::Bind
Binder::Bind
Binder::BindNode
Binder::BindNode
ExpressionBinder::BindExpression

In this cycle, the stack counter of StackCheck is incremented once (in the main switch in expression_binder.cpp), despite there being 16 function calls.

[Mytherin]

I wonder if there is any place where a large amount of data is placed onto the stack (e.g. a large array)? 128 does seem very low for a limit. As Alex mentioned Python defaults to 10K, SQLite defaults to 1K.

We might also want to add more recursion checking in e.g. Binder::BindSelectNode as well.

[lnkuiper]

I don't think any large stack allocations are being done. For every pass through the main switch in ExpressionBinder, we have 16 function calls until we increment StackCheck again, but only for this specific test case with the recursive table macro. Small stack allocations are being done in some of the 16 function calls.

I've tried again and increased the limit to 425 (that's as far as I went with my binary search). Considering we have 16 function calls per increment, that's a stack depth 425 * 16 = 6800. If we want, we could consider increasing the stack size at compile-time. I could also add a special case to bind_macro_expression.cpp to increment the stack counter by more?

[Mytherin]

I think perhaps we need to add more instances of the StackChecker along the path then. Shouldn't the stack depth be incremented multiple times as ExpressionBinder::BindExpression is called several times in the path?

We should likely also add a stack increment to Binder::BindNode.

[Mytherin]

Thanks!