-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Rework settings handling and implement auto-generation for new ones #14018
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Can the C API for settings get some love too? Inconsistency between duckdb_settings() and duckdb_get_config_flag |
samansmink
reviewed
Sep 20, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @chrisiou, Thanks for the PR, looks great! I've left a few comments.
…eset verification is enabled
0e9ef6e
to
ec8295f
Compare
samansmink
reviewed
Oct 2, 2024
Mytherin
added a commit
that referenced
this pull request
Oct 16, 2024
…14383) Follow-up from #14018, finalizing the implementation This PR enables the generation of settings through a JSON file, avoiding having to touch many places in the code to add a single setting. Examples of such settings: ##### Simple ```json "name": "allow_extensions_metadata_mismatch", "description": "Allow to load extensions with not compatible metadata", "type": "BOOLEAN", "scope": "global" ``` ##### More bells and whistles ```json { "name": "checkpoint_threshold", "description": "The WAL size threshold at which to automatically trigger a checkpoint (e.g. 1GB)", "type": "VARCHAR", "scope": "global", "internal_setting": "checkpoint_wal_size", "custom_implementation": [ "set", "get" ], "aliases": [ "wal_autocheckpoint" ] }, ``` | Setting | Description | |-----------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------| | name | Name of the setting | | description | Description of the setting | | type | SQL type of the setting (e.g. BIGINT) | | return_type | C++ return type of the setting - if not set this is derived from the SQL type (e.g. VARCHAR -> string) | | scope | Global (DBConfig) or Local (ClientConfig) | | internal_setting | C++ variable name of the setting - if not provided equal to name | | on_callbacks | Custom callbacks to generate (set/reset) - generates OnGlobalSet/OnLocalSet/OnGlobalReset/OnLocalReset callbacks that can be used to verify input data | | custom_implementation | Whether or not the setting code is automatically generated - can be either true/false, or a list providing which has a custom implementation (set/reset/get) | | struct | Struct name of the setting, defaults to setting name in CamelCase | | aliases | List of aliases to generate for the setting | Not all settings can be fully auto-generated, therefore they are split between `custom_settings.cpp` and `autogenerated_settings.cpp`. The `custom_implementation` flag controls whether or not code for the setting is automatically generated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a new approach for defining and generating settings and a thread-safe accessor for those settings.
Steps to add a new setting
1. Update the
settings.json
file:(*): Indicates flags that enable custom implementations.
If any of the starred options (*) are enabled, a new code section will be added to
custom_settings.cpp
. See Step 4 for more details. These options won’t overwrite any existing content.2. Add a new variable in the configuration structure
To store the value of the new setting, add a new variable in the appropriate config structure:
global
settings, add it tosrc/include/duckdb/main/config.hpp
.local
settings, add it tosrc/include/duckdb/main/client_config.hpp
.note: If the setting is autogenerated, the
setting.name
will be used as the variable name for the configuration structure. Ensure consistency by using the same name.3. To generate the necessary code, run:
Alternatively, if you only want to run the script that automates the settings auto-generation process, use the
generate_settings.py
script. You can run it:python3 scripts/generate_settings.py
4. (Optional) Implement custom functionality
If the new setting requires custom functionality, look for the newly added corresponding block in
custom_settings.cpp
and modify the new methods. Otherwise, everything should be auto-generated.note: If you want the setting's value to be applied only under certain conditions, use the verification method. If the condition is not met, return false to prevent the value from being set.
5. Write tests and verify that everything works correctly.
About this PR:
Race Condition Fix: Templated Accessor for Settings
Previously, settings were accessed directly from the options, which could lead to race conditions. This PR introduces a templated accessor to improve thread safety. The
GetSetting
accessor now ensures proper locking by obtaining a mutex before returning the setting's value.Example usage:
The
GetSetting
methods inDBConfig
(they are similar in theClientConfig
):What functionality is actually needed for adding a new setting (almost all is now being handled automatically)
settings.hpp
file.DBConfig
orClientContext
.ConfigurationOption internal_options
list found inconfig.cpp
.Explanation of
generate_settings.py
The
generate_settings.py
script automates several steps when adding a new setting:It calls
scripts/settings_scripts/parse_and_sort_settings_in_json.py
, which parses and sorts entries insettings.json
. The settings are stored globally asSettingsList
objects.It invokes
scripts/settings_scripts/update_settings_header_file.py
to automatically declare new settings and their methods insettings.hpp
.It runs
scripts/settings_scripts/update_settings_scopes.py
, which adds new setting scopes and aliases to theinternal_options
configuration list inconfig.cpp
.It executes two scripts:
scripts/settings_scripts/update_autogenerated_functions.py
auto-generates method definitions inautogenerated_settings.cpp
.scripts/settings_scripts/update_custom_signats_functions.py
adds a block tocustom_settings.cpp
, only if custom validation or functionality is required.The script also formats
settings.hpp
,autogenerated_settings.cpp
, andconfig.cpp
files.Disable automatic addition of new coding section in
custom_settings.cpp
The code related to settings in
settings.hpp
,config.cpp
, andautogenerated_settings.cpp
is automatically generated. However, if the addition of a new settings block incustom_settings.cpp
causes issues, it can be disabled. To do so, remove the code that is mentioned in the corresponding comment in thescripts/settings_scripts/update_settings_src_code.py