feat: add thread_stack_size params to customize the thread stack si…#3885
Uh oh!
There was an error while loading. Please reload this page.
Conversation
| // Soft limit on the number of rows that `ANALYZE` touches per index. | ||
| pragmas.insert("analysis_limit".into(),None); | ||
| let default_thread_stack_size = 512*1024;// 512KB |
There was a problem hiding this comment.
I'm not comfortable making the default stack size this small. Because we invoke user-supplied callbacks from the worker thread, it's impossible to say what a safe minimum stack size is besides the current default.
Additionally, there's no leeway for platform-specific requirements. 64-bit platforms are going to need more stack size than 32-bit platforms because the size of pointers and usize/isize values is doubled.
This should be Option<usize> and default to not specifying a stack size and just letting the std choose.
There was a problem hiding this comment.
Agree with you, I'll post a new PR!
abonander commented Aug 21, 2025
Closing due to inactivity and unaddressed feedback. Feel free to open a new PR after addressing the comment. |
Changes `thread_stack_size` from `usize` to `Option<usize>` to address concerns about safety and platform compatibility. Key improvements: - Default to `None`, using Rust std's default stack size (typically 2MB) - Only apply custom stack size when explicitly configured - Safer for user callbacks with unpredictable stack requirements - Platform-agnostic (handles 32-bit vs 64-bit differences automatically) - Marked as an advanced option in documentation with appropriate warnings This addresses the feedback from PR launchbadge#3885 about hardcoded stack sizes being unsafe due to: 1. Unpredictable stack needs of user-supplied callbacks 2. Platform-specific requirements (32-bit vs 64-bit) 3. Need for conservative defaults Related: launchbadge#3885
2 participants
The thread stack size could be critical on mobile devices. Adding the param
thread_stack_sizecould be more flexible, especially for building a mobile application.Does your PR solve an issue?
Delete this text and add "fixes #(issue number)".
Do not just list issue numbers here as they will not be automatically closed on merging this pull request unless prefixed with "fixes" or "closes".
Is this a breaking change?
Delete this text and answer yes/no and explain.
If yes, this pull request will need to wait for the next major release (
0.{x + 1}.0)Behavior changes can be breaking if significant enough.
Consider Hyrum's Law: