Add settings to disable/enable MySQL/PostgreSQL module - Closes #4951 #4960
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @JavierJF, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request by @JavierJF. This PR introduces new configuration settings and command-line options to allow users to explicitly enable or disable the MySQL and PostgreSQL modules within ProxySQL. This provides more granular control over which database backends and associated functionalities (worker threads, admin interfaces, monitoring) are active, addressing issue #4951. Additionally, it includes minor fixes related to PostgreSQL replication table definitions and MySQL worker thread naming.
Highlights
- New Configuration Settings: Adds new settings
--mysql-workers,--pgsql-workers,--mysql-admin,--pgsql-admin,--mysql-monitor, and--pgsql-monitor(accessible via config file and command line) to control the startup of specific ProxySQL modules for MySQL and PostgreSQL. By default, all are enabled (true). Setting any of these tofalseor0will prevent the corresponding module/feature from starting. - PostgreSQL Replication Table Fix: Removes unsupported
CHECKconstraints related toinnodb_read_onlyandsuper_read_onlyfrom thepgsql_replication_hostgroupstable definition, as these are MySQL-specific concepts. - MySQL Worker Thread Naming Fix: Corrects the thread naming logic for MySQL idle worker threads.
- Module Startup Logic Updates: Modifies the core startup logic for MySQL and PostgreSQL worker threads and admin interfaces to respect the new global configuration variables.
Changelog
Click here to see the changelog
- include/PgSQL_HostGroups_Manager.h
- Removed unsupported
CHECKconstraints (innodb_read_only,super_read_only, combinations) from thepgsql_replication_hostgroupstable definition (line 49).
- Removed unsupported
- include/ProxySQL_Admin_Tables_Definitions.h
- Removed unsupported
CHECKconstraints from thepgsql_replication_hostgroupstable definition (line 279). - Removed unsupported
CHECKconstraints from theruntime_pgsql_replication_hostgroupstable definition (line 290).
- Removed unsupported
- include/proxysql_glovars.hpp
- Added new boolean global variables
mysql_workers,pgsql_workers,mysql_admin,pgsql_adminto theProxySQL_GlobalVariablesstruct (lines 91-94).
- Added new boolean global variables
- lib/MySQL_Thread.cpp
- Modified
set_variablefunction to respect the newmysql_workersglobal variable when setting the number of threads (lines 2063-2064). - Modified
initfunction to respect the newmysql_workersglobal variable when determining the default number of threads (line 2410). - Corrected the thread name setting for MySQL idle threads to use
mysql_threads_idlesinstead ofmysql_threads(line 2465).
- Modified
- lib/PgSQL_Thread.cpp
- Modified
set_variablefunction to respect the newpgsql_workersglobal variable when setting the number of threads (lines 2000-2001). - Modified
initfunction to respect the newpgsql_workersglobal variable when determining the default number of threads (line 2322).
- Modified
- lib/ProxySQL_Admin.cpp
- Removed commented-out code related to client handling in
admin_main_loop(lines 2261-2263, 2300-2302, 2415, 2456, 2474). - Adjusted the loop for starting MySQL admin interfaces to check the
GloVars.global.mysql_adminflag (line 2387). - Adjusted the loop for starting PostgreSQL admin interfaces to check the
GloVars.global.pgsql_adminflag (line 2427).
- Removed commented-out code related to client handling in
- lib/ProxySQL_GloVars.cpp
- Initialized the new global variables (
mysql_workers,pgsql_workers,mysql_admin,pgsql_admin) totrueby default (lines 200-203). - Added command-line options for the new settings (
--mysql-monitor,--pgsql-monitor,--mysql-workers,--pgsql-workers,--mysql-admin,--pgsql-admin) (lines 268-273). - Added logic to process the new command-line options and update the corresponding global variables (lines 499-551).
- Initialized the new global variables (
- src/main.cpp
- Added a template helper function
update_global_variableto simplify reading boolean settings from the config file (lines 675-688). - Used the new helper function to read the new module enable/disable settings from the config file (lines 747-754).
- Added coherence checks to log messages if all admin interfaces, worker threads, or monitoring are disabled, or if PostgreSQL support is fully disabled (lines 891-903).
- Modified the loop for creating MySQL worker threads to check the
GloVars.global.mysql_workersflag (line 1004). - Modified the loop for creating PostgreSQL worker threads to check the
GloVars.global.pgsql_workersflag (line 1028).
- Added a template helper function
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Modules stand tall,
Or not, at your call.
Configured just right,
For day and for night.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Code Review
This pull request introduces valuable flexibility by allowing users to disable/enable MySQL and PostgreSQL modules (workers, admin interfaces, and monitors) via command-line switches and configuration files. The implementation is largely clean and effective, with good refactoring in src/main.cpp to handle global variable updates. The minor fixes for MySQL worker thread naming and PostgreSQL DDL are also welcome improvements.
One critical issue was found that needs addressing before merging. Other than that, the changes enhance the configurability and maintainability of ProxySQL.
Summary of Findings
- Correctness: Typo in PgSQL Thread Configuration: In
lib/PgSQL_Thread.cpp, when determining if the number of PostgreSQL worker threads (intv) is valid, the condition incorrectly checks!GloVars.global.mysql_workersinstead of!GloVars.global.pgsql_workers. This could lead topgsql_workersbeing forced to 0 ifmysql_workersis disabled, regardless of thepgsql_workerssetting. - Maintainability: Refactoring Global Variable Updates: The introduction of the
update_global_variabletemplate function insrc/main.cppis a good refactoring choice, reducing code duplication when processing settings from the configuration file. - Feature Implementation: New Module Control Flags: The new command-line flags (
--mysql-workers,--pgsql-workers, etc.) and corresponding global variables are clearly implemented, defaulting totrueas described and allowing modules to be disabled effectively. - Bug Fix: MySQL Worker Thread Naming: The fix in
lib/MySQL_Thread.cppto usemysql_threads_idles[tn].thread_idfor setting idle thread names appears to correct a previous issue. - Database Schema: PostgreSQL Replication Hostgroups Simplification: The DDL changes in
include/PgSQL_HostGroups_Manager.handinclude/ProxySQL_Admin_Tables_Definitions.hsimplify thecheck_typeconstraint forpgsql_replication_hostgroups, which is a good cleanup if the removed options were unsupported. - Maintainability: Code Cleanup: Several instances of commented-out code were removed in
lib/ProxySQL_Admin.cpp, improving code readability. - Maintainability: Command-line Option Processing (Not Commented due to Review Settings): In
lib/ProxySQL_GloVars.cpp, the processing of new command-line options involves several similarif (opt->isSet(...)) { ... }blocks. While clear, this could potentially be refactored into a helper function if more options are added in the future, similar to theupdate_global_variabletemplate used for config file parsing. This was not commented on directly due to the severity filter settings.
Merge Readiness
This pull request introduces significant and useful new functionality. However, there is a high severity issue in lib/PgSQL_Thread.cpp related to the configuration of PostgreSQL worker threads that must be addressed before merging. Once this issue is resolved, the PR should be in good shape. As an AI, I am not authorized to approve pull requests; please ensure further review and approval by authorized maintainers.
…#4951 Accessible as config file and command line switches: + --(mysql|pgsql)-workers=(true|false) + --(mysql|pgsql)-admin=(true|false) + --(mysql|pgsql)-monitor=(true|false) The default value for all is 'true'.
Since editors sometimes don't completely disable non-reachable code, like macro-protected, it's cleaner too keep parenthesis open/close outside this. Otherwise this can confuse editors scope matching.
JavierJF
dismissed
gemini-code-assist[bot]’s stale review
Not picking up addressed changes.
|
retest this please |
There was a problem hiding this comment.
Pull Request Overview
This PR adds command-line and configuration settings to enable/disable MySQL and PostgreSQL modules while also applying minor fixes. Key changes include the introduction of a template helper function (update_global_variable) for setting globals, updates to command-line options and defaults for MySQL/PgSQL worker, admin, and monitor modules, and modifications to SQL table definitions to remove unsupported check constraints.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main.cpp | Introduces the update_global_variable template, uses it to update new globals, and adjusts thread creation. |
| lib/ProxySQL_GloVars.cpp | Adds default values and new command-line options for MySQL/PgSQL modules; updates process_opts_post logic. |
| lib/ProxySQL_Admin.cpp | Applies conditional loops based on new global admin flags and removes legacy commented code. |
| lib/PgSQL_Thread.cpp, lib/MySQL_Thread.cpp | Adjust thread count logic to consider the new worker flags during configuration time. |
| include/proxysql_glovars.hpp | Declares new global flags for module enablement. |
| include/ProxySQL_Admin_Tables_Definitions.h | Updates the pgsql_replication_hostgroups table definition to only accept 'read_only' for check_type. |
| include/PgSQL_HostGroups_Manager.h | Mirrors the changes of replication hostgroups definition for consistency. |
Comments suppressed due to low confidence (1)
src/main.cpp:753
- [nitpick] The configuration key 'mysql-monitor' does not clearly match the variable name 'my_monitor'; consider renaming either the key or the variable for improved consistency.
update_global_variable("mysql-monitor", GloVars.global.my_monitor);
| void update_global_variable(const string& name, T& var) { | ||
| const Setting& root { GloVars.confFile->cfg.getRoot() }; | ||
|
|
||
| if (root.exists(name)==true) { |
There was a problem hiding this comment.
[nitpick] Consider simplifying the expression to 'if (root.exists(name))' to improve readability.
| if (root.exists(name)==true) { | |
| if (root.exists(name)) { |
Copilot uses AI. Check for mistakes.
| @@ -276,7 +276,7 @@ | |||
| #define ADMIN_SQLITE_TABLE_PGSQL_FIREWALL_WHITELIST_SQLI_FINGERPRINTS "CREATE TABLE pgsql_firewall_whitelist_sqli_fingerprints (active INT CHECK (active IN (0,1)) NOT NULL DEFAULT 1 , fingerprint VARCHAR NOT NULL , PRIMARY KEY (fingerprint) )" | |||
| #define ADMIN_SQLITE_TABLE_PGSQL_QUERY_RULES_FAST_ROUTING "CREATE TABLE pgsql_query_rules_fast_routing (username VARCHAR NOT NULL , database VARCHAR NOT NULL , flagIN INT NOT NULL DEFAULT 0 , destination_hostgroup INT CHECK (destination_hostgroup >= 0) NOT NULL , comment VARCHAR NOT NULL , PRIMARY KEY (username, database, flagIN) )" | |||
| #define ADMIN_SQLITE_TABLE_PGSQL_HOSTGROUP_ATTRIBUTES "CREATE TABLE pgsql_hostgroup_attributes (hostgroup_id INT NOT NULL PRIMARY KEY , max_num_online_servers INT CHECK (max_num_online_servers>=0 AND max_num_online_servers <= 1000000) NOT NULL DEFAULT 1000000 , autocommit INT CHECK (autocommit IN (-1, 0, 1)) NOT NULL DEFAULT -1 , free_connections_pct INT CHECK (free_connections_pct >= 0 AND free_connections_pct <= 100) NOT NULL DEFAULT 10 , init_connect VARCHAR NOT NULL DEFAULT '' , multiplex INT CHECK (multiplex IN (0, 1)) NOT NULL DEFAULT 1 , connection_warming INT CHECK (connection_warming IN (0, 1)) NOT NULL DEFAULT 0 , throttle_connections_per_sec INT CHECK (throttle_connections_per_sec >= 1 AND throttle_connections_per_sec <= 1000000) NOT NULL DEFAULT 1000000 , ignore_session_variables VARCHAR CHECK (JSON_VALID(ignore_session_variables) OR ignore_session_variables = '') NOT NULL DEFAULT '' , hostgroup_settings VARCHAR CHECK (JSON_VALID(hostgroup_settings) OR hostgroup_settings = '') NOT NULL DEFAULT '' , servers_defaults VARCHAR CHECK (JSON_VALID(servers_defaults) OR servers_defaults = '') NOT NULL DEFAULT '' , comment VARCHAR NOT NULL DEFAULT '')" | |||
| #define ADMIN_SQLITE_TABLE_PGSQL_REPLICATION_HOSTGROUPS "CREATE TABLE pgsql_replication_hostgroups (writer_hostgroup INT CHECK (writer_hostgroup>=0) NOT NULL PRIMARY KEY , reader_hostgroup INT NOT NULL CHECK (reader_hostgroup<>writer_hostgroup AND reader_hostgroup>=0) , check_type VARCHAR CHECK (LOWER(check_type) IN ('read_only','innodb_read_only','super_read_only','read_only|innodb_read_only','read_only&innodb_read_only')) NOT NULL DEFAULT 'read_only' , comment VARCHAR NOT NULL DEFAULT '', UNIQUE (reader_hostgroup))" | |||
| #define ADMIN_SQLITE_TABLE_PGSQL_REPLICATION_HOSTGROUPS "CREATE TABLE pgsql_replication_hostgroups (writer_hostgroup INT CHECK (writer_hostgroup>=0) NOT NULL PRIMARY KEY , reader_hostgroup INT NOT NULL CHECK (reader_hostgroup<>writer_hostgroup AND reader_hostgroup>=0) , check_type VARCHAR CHECK (LOWER(check_type) IN ('read_only')) NOT NULL DEFAULT 'read_only' , comment VARCHAR NOT NULL DEFAULT '', UNIQUE (reader_hostgroup))" | |||
There was a problem hiding this comment.
Since 3.0.1 we can't simply change table definition. We need an upgrade procedure.
There was a problem hiding this comment.
Actually, this may need a PR on its own.
There was a problem hiding this comment.
I don't know if that is really worth it, the check type is never used in the PostgreSQL implementation, since only read_only (the default) is supported, which value is never really used either. A migration procedure for a unused settings seems unnecessary... But I can split it into another PR.
There was a problem hiding this comment.
It is necessary because during bootstrap ProxySQL will delete any table that do not have the expected table definition. So even removing unused constraint should cause ProxySQL to drop and recreate the table.
There was a problem hiding this comment.
Ahhh right! now I remember. The table is being rebuild automatically, so the entries already stored will be missing without the upgrade due to the rebuilding. Should I pack those changes in another PR?
There was a problem hiding this comment.
Yes, a dedicated PR is better, as it has nothing to do with start-up options.
Thanks!
| bool mysql_workers; | ||
| bool pgsql_workers; | ||
| bool mysql_admin; | ||
| bool pgsql_admin; |
There was a problem hiding this comment.
nitpicking: we should follow a pattern, either first all mysql variables then all posgres variables (or viceversa), or follow some order: *_monitor , *-workers , *_admin .
|
When documenting these variables it is worth to mention that the Hostgroups Manager and Authentication Module are always enabled, no matter if Admin/Monitor/Workers are disabled. |
Main addition
This PR adds the new settings:
Accessible as config file and command line switches, the default value for all is 'true'.
Minor fixes
pgsql_replication_hostgroupstable definition