feat(sql-udf): deep calling stack (recursion) prevention for sql udf #14392
Conversation
There was a problem hiding this comment.
We're not sure 16 is safe. It depends on OS configuration, optimization level, target arch, and so on. So that's why I mentioned stacker as a fallback in #14139 (comment).
But the overall idea LGTM.
|
Test cases are updated (i.e., materialized view, valid recursive definition are covered), current approach is changed to global counting calling stack depth to prevent heavy binding along the way. Note that this is not just related to potential recursive call, but also normal deep calling chain, we should be able to detect and timely return error for these cases, otherwise the cluster will panic and quit. Since cc @stdrc @xxchan @BugenZhao @wangrunji0408. P.S. After private discussion, recursive sql udf support is postponed at present, maybe supported in the future. |
| // Update the status for the global counter | ||
| self.udf_context.incr_global_count(); |
There was a problem hiding this comment.
It seems not right that this counter never decreases?
There was a problem hiding this comment.
The counter will be reset to 0 after a success binding to a specific sql udf. (i.e., when cleaning the udf_context)
// Clean the `udf_context` & `udf_recursive_context` after inlining,
// which makes sure the subsequent binding will not be affected
if clean_flag {
self.udf_context.clear();
}
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
As titled, to simulate pg's behavior when dealing with recursive sql udf, a
SQL_UDF_MAX_RECURSIVE_DEPTHis set to 16.The reason for this is that, during local testing, ~60 calls to
bind_functionwill exhaust the stack space for the current running rw thread, which will then be killed by os, and the entire cluster will quit.And note that some test cases are updated, the previous fragile format check for recursive definition is removed. (i.e.,
create function foo(INT) returns varchar language sql as $$select 'foo(INT)'$$;is totally valid now)reference: #14139 (comment). (Please note that the implementation is not exact the same as the proposal in the discussion)
Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.