Fix #5272: Add mysql-select_version_forwarding variable for SELECT VERSION()#5277
Fix #5272: Add mysql-select_version_forwarding variable for SELECT VERSION()#5277renecannao merged 3 commits intov3.0from
Conversation
…RSION() Since ProxySQL 3.0.4, SELECT VERSION() queries were intercepted and returned ProxySQL's mysql-server_version variable instead of proxying to backends. This broke SQLAlchemy for MariaDB which expects "MariaDB" in the version string. This commit adds a new variable `mysql-select_version_forwarding` with 4 modes: - 0 = never: Always return ProxySQL's version (3.0.4+ behavior) - 1 = always: Always proxy to backend (3.0.3 behavior) - 2 = smart (fallback to 0): Try backend connection, else ProxySQL version - 3 = smart (fallback to 1): Try backend connection, else proxy (default) The implementation includes: - New global variable mysql_thread___select_version_forwarding - New function get_backend_version_for_hostgroup() to peek at backend connection versions without removing them from the pool - Modified SELECT VERSION() handler to support all 4 modes - ProxySQL backend detection to avoid recursion Mode 3 (default) ensures SQLAlchemy always gets the real MariaDB version string while maintaining fast response when connections are available.
Summary of ChangesHello @renecannao, 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! This pull request addresses a critical compatibility issue where ProxySQL's interception of Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new configuration variable, mysql-select_version_forwarding, to control how ProxySQL responds to SELECT VERSION() queries. The changes implement four distinct modes: always returning ProxySQL's version, always forwarding the query to a backend, or two 'smart' modes that attempt to retrieve a backend's version from an available connection before falling back to either ProxySQL's version or forwarding the query. A new helper function, get_backend_version_for_hostgroup, was added to facilitate retrieving backend server versions. Review comments suggest improving code readability and maintainability by replacing the integer modes with an enum and refactoring the duplicated logic present in the 'smart' modes.
lib/MySQL_Session.cpp
Outdated
| if ((pkt->size==SELECT_MYSQL_VERSION_LEN+5 && *((char *)(pkt->ptr)+4)==(char)0x03 && strncasecmp((char *)SELECT_MYSQL_VERSION,(char *)pkt->ptr+5,pkt->size-5)==0) || | ||
| (pkt->size==SELECT_MYSQL_VERSION_FUNC_LEN+5 && *((char *)(pkt->ptr)+4)==(char)0x03 && strncasecmp((char *)SELECT_MYSQL_VERSION_FUNC,(char *)pkt->ptr+5,pkt->size-5)==0)) { | ||
| char *version_to_return = NULL; | ||
| int mode = mysql_thread___select_version_forwarding; // 0=never, 1=always, 2=smart(fallback to 0), 3=smart(fallback to 1) |
There was a problem hiding this comment.
The modes 0, 1, 2, 3 for mysql-select_version_forwarding are used as magic numbers. To improve readability and maintainability, consider defining an enum for these modes. This would make the code self-documenting and less prone to errors. For example:
enum SelectVersionForwardingMode {
NEVER = 0,
ALWAYS = 1,
SMART_FALLBACK_INTERNAL = 2,
SMART_FALLBACK_PROXY = 3
};This enum could be defined in a shared header like include/MySQL_Thread.h to be accessible where needed.
lib/MySQL_Session.cpp
Outdated
| else if (mode == 2) { | ||
| // smart (fallback to 0): try to get version from backend connection, else use ProxySQL version | ||
| int target_hg = (current_hostgroup >= 0) ? current_hostgroup : default_hostgroup; | ||
|
|
||
| if (target_hg >= 0) { | ||
| version_to_return = get_backend_version_for_hostgroup(target_hg); | ||
|
|
||
| // Check if backend is ProxySQL (to avoid recursion) | ||
| if (version_to_return && strstr(version_to_return, "ProxySQL")) { | ||
| version_to_return = NULL; | ||
| } | ||
| } | ||
|
|
||
| // Fallback to ProxySQL version if no backend version found | ||
| if (!version_to_return) { | ||
| version_to_return = mysql_thread___server_version; | ||
| } | ||
| } | ||
| else if (mode == 3) { | ||
| // smart (fallback to 1): try to get version from backend connection, else proxy to backend | ||
| int target_hg = (current_hostgroup >= 0) ? current_hostgroup : default_hostgroup; | ||
|
|
||
| if (target_hg >= 0) { | ||
| version_to_return = get_backend_version_for_hostgroup(target_hg); | ||
|
|
||
| // Check if backend is ProxySQL (to avoid recursion) | ||
| if (version_to_return && strstr(version_to_return, "ProxySQL")) { | ||
| version_to_return = NULL; | ||
| } | ||
| } | ||
|
|
||
| // Fallback: if no backend version found, proxy to backend (mode 1 behavior) | ||
| if (!version_to_return) { | ||
| return false; | ||
| } | ||
| } | ||
| else { | ||
| // mode 0 (never): use ProxySQL's version | ||
| version_to_return = mysql_thread___server_version; | ||
| } |
There was a problem hiding this comment.
The logic for modes 2 and 3 is very similar, leading to some code duplication. This can be refactored to make the code more concise and easier to maintain by extracting the common logic for finding a backend version.
else if (mode == 2 || mode == 3) {
// smart modes
int target_hg = (current_hostgroup >= 0) ? current_hostgroup : default_hostgroup;
if (target_hg >= 0) {
version_to_return = get_backend_version_for_hostgroup(target_hg);
// Check if backend is ProxySQL (to avoid recursion)
if (version_to_return && strstr(version_to_return, "ProxySQL")) {
version_to_return = NULL;
}
}
if (!version_to_return) {
if (mode == 2) { // fallback to 0
version_to_return = mysql_thread___server_version;
} else { // mode == 3, fallback to 1
return false;
}
}
}
else { // mode == 0
// mode 0 (never): use ProxySQL's version
version_to_return = mysql_thread___server_version;
}…ling - Add SelectVersionForwardingMode enum to replace magic numbers (0,1,2,3) - Refactor modes 2 and 3 to eliminate code duplication - Improve code readability and maintainability Addresses feedback from gemini-code-assist on PR #5277
The test was failing because the default mode 3 (smart with fallback to proxying) tries to proxy the query when no backends exist, causing the query to fail. Changes: - Split test into two scenarios: 1. Mode 3 (explicitly set): Expected to FAIL with no backends 2. Mode 2: Expected to SUCCEED with no backends (fallback to internal) - Added extensive documentation explaining: * All 4 modes of mysql-select_version_forwarding * Why mode 3 fails with no backends (correct behavior) * Why mode 2 succeeds with no backends (fallback to internal version) * Test execution flow and why both scenarios matter - Refactored test into test_mode() helper function for clarity
|
|
retest this please |
1 similar comment
|
retest this please |
This reverts commit 186ea8b. ProxySQL version 3.0.5 has been released with a fix for this issue ([1]). [1] sysown/proxysql#5277
This reverts commit 186ea8b. ProxySQL version 3.0.5 has been released with a fix for this issue ([1]). [1] sysown/proxysql#5277 Signed-off-by: Jan Horstmann <horstmann@osism.tech>
This reverts commit 186ea8b. ProxySQL version 3.0.5 has been released with a fix for this issue ([1]). [1] sysown/proxysql#5277 Signed-off-by: Jan Horstmann <horstmann@osism.tech>
Problem
Since ProxySQL 3.0.4,
SELECT VERSION()queries are intercepted and return ProxySQL'smysql-server_versionvariable instead of proxying to backend servers. This breaks SQLAlchemy and other MariaDB clients that expect "MariaDB" in the version string.Example error:
This happens because SQLAlchemy doesn't detect MariaDB (the version is "8.0.11" instead of something like "10.11.15-MariaDB") and tries to use MySQL-specific syntax.
Solution
This PR adds a new global variable
mysql-select_version_forwardingwith 4 modes:Implementation Details
New Variable
mysql-select_version_forwardingNew Function:
get_backend_version_for_hostgroup()Peeks at backend connection versions without removing them from the connection pool:
ConnectionsFree->conns_length()server_versionfrom first available connectionwrlock()/wrunlock()ProxySQL Backend Detection
To avoid recursion when ProxySQL is behind ProxySQL:
Usage Example
Why Mode 3 is Default
Mode 3 ensures SQLAlchemy always gets the real MariaDB version:
Files Changed
include/proxysql_structs.h- Variable declarationsinclude/MySQL_Thread.h- Variable in MySQL_Threads_Handlerinclude/MySQL_Session.h- Function declarationlib/MySQL_Thread.cpp- Variable initialization and registrationlib/MySQL_Session.cpp- Function implementation and SELECT VERSION() handlerRelated Issues
Fixes #5272