Fix #5306 - SHOW WARNINGS with inline comments incorrectly sets warning_count #5307
Conversation
Summary of ChangesHello @yuji-hatakeyama, 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 bug in ProxySQL where 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
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces raw-query comparison with digest_text-based detection for SHOW WARNINGS EOF handling in the async result path; adds a TAP regression test verifying SHOW WARNINGS with inline comments yields correct EOF warning_count. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug where SHOW WARNINGS with inline comments would not be detected properly, leading to an incorrect warning_count. The approach of using digest_text is sound and consistent with other parts of the codebase. The added regression test is thorough and covers the necessary cases.
I've added a couple of suggestions: one to improve the performance of the string comparison in mysql_connection.cpp and another to clean up unused headers in the new test file. Overall, this is a good fix.
| const size_t dig_len = strlen(dig_text); | ||
| is_show_warnings = (dig_len == 13 && strncasecmp(dig_text, "SHOW WARNINGS", 13) == 0); |
There was a problem hiding this comment.
To improve efficiency, you can avoid calling strlen, which scans the entire string. A more performant approach for an exact match check is to use strncasecmp and then verify the string ends with a null terminator right after the matched part.
is_show_warnings = (strncasecmp(dig_text, "SHOW WARNINGS", 13) == 0 && dig_text[13] == '\0');There was a problem hiding this comment.
Keeping the current style for consistency with the rest of the codebase
e.g.,
proxysql/lib/mysql_connection.cpp
Line 2795 in 2637d28
| #include <unistd.h> | ||
| #include <string> | ||
| #include <thread> |
There was a problem hiding this comment.
These headers (unistd.h, string, thread) don't appear to be used in this file. It's good practice to remove unused includes to improve compilation times and reduce code clutter.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp`:
- Around line 27-31: The check for proxysql_admin after calling mysql_init()
calls mysql_error(proxysql_admin) while proxysql_admin may be NULL; change the
error path in the mysql_init() failure branch (the proxysql_admin variable) to
avoid calling mysql_error with a NULL handle and instead log a static
descriptive message (e.g. "mysql_init() failed") or include errno-based text;
update the fprintf call in that block to use the static message rather than
mysql_error(proxysql_admin).
- Around line 42-51: The early-return error paths leak MySQL handles: ensure any
MYSQL* that was successfully initialized/connected is closed before returning.
Specifically, if mysql_real_connect(proxysql, ...) fails call
mysql_close(proxysql) (and similarly call mysql_close(proxysql_admin) on paths
where proxysql_admin was created but you return early); update the error
branches that use mysql_error(proxysql) to close the corresponding MYSQL*
handles before returning or calling exit_status().
| MYSQL* proxysql = mysql_init(NULL); | ||
| if (!proxysql) { | ||
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql)); | ||
| return -1; | ||
| } | ||
|
|
||
| if (!mysql_real_connect(proxysql, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { | ||
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql)); | ||
| return exit_status(); | ||
| } |
There was a problem hiding this comment.
Minor: Resource leak on early return.
If the ProxySQL connection fails, proxysql_admin is not closed before returning. Similar pattern appears for the proxysql_admin initialization failure at line 29 (though less critical since nothing was allocated yet for other resources).
Proposed fix
MYSQL* proxysql = mysql_init(NULL);
if (!proxysql) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql));
+ mysql_close(proxysql_admin);
return -1;
}
if (!mysql_real_connect(proxysql, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql));
+ mysql_close(proxysql);
+ mysql_close(proxysql_admin);
return exit_status();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MYSQL* proxysql = mysql_init(NULL); | |
| if (!proxysql) { | |
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql)); | |
| return -1; | |
| } | |
| if (!mysql_real_connect(proxysql, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { | |
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql)); | |
| return exit_status(); | |
| } | |
| MYSQL* proxysql = mysql_init(NULL); | |
| if (!proxysql) { | |
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql)); | |
| mysql_close(proxysql_admin); | |
| return -1; | |
| } | |
| if (!mysql_real_connect(proxysql, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { | |
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql)); | |
| mysql_close(proxysql); | |
| mysql_close(proxysql_admin); | |
| return exit_status(); | |
| } |
🤖 Prompt for AI Agents
In `@test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp` around lines
42 - 51, The early-return error paths leak MySQL handles: ensure any MYSQL* that
was successfully initialized/connected is closed before returning. Specifically,
if mysql_real_connect(proxysql, ...) fails call mysql_close(proxysql) (and
similarly call mysql_close(proxysql_admin) on paths where proxysql_admin was
created but you return early); update the error branches that use
mysql_error(proxysql) to close the corresponding MYSQL* handles before returning
or calling exit_status().
There was a problem hiding this comment.
Same pattern in other tests
(e.g.,
proxysql/test/tap/tests/reg_test_4072-show-warnings-t.cpp
Lines 44 to 51 in 2637d28
Since these are test files and resources are freed on process exit anyway. if worth fixing, perhaps better addressed in a separate PR.
There was a problem hiding this comment.
@yuji-hatakeyama, you're absolutely right—this is a common pattern in ProxySQL test files, and since these processes are short-lived, the OS will clean up the resources on exit anyway. I agree it's out of scope for this PR and would be better addressed separately if at all. Thanks for the clarification!
✏️ Learnings added
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:27.573Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp`:
- Around line 67-70: The test calls mysql_store_result(proxysql) into res and
then unconditionally calls mysql_num_rows(res); add a NULL check for res after
mysql_store_result to avoid undefined behavior: if res is NULL, handle the error
path (e.g., set row_count = 0 or fail the test/assert accordingly) before
calling mysql_num_rows and ensure mysql_free_result is only called when res is
non-NULL. Update the block around MYSQL_QUERY(proxysql, show_query),
mysql_store_result(proxysql), mysql_num_rows(res), and mysql_free_result(res) to
perform this check using the existing variables res, proxysql, show_query and
preserve intent of the test.
♻️ Duplicate comments (3)
test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp (3)
25-27: Minor:mysql_error()on potentially NULL handle.If
mysql_init()returnsNULL, callingmysql_error(proxysql_admin)is undefined behavior. Use a static error message instead.
39-48: Minor: Resource leaks andmysql_error()on NULL handle.The issues in this section (calling
mysql_error(proxysql)whenproxysqlmay be NULL, and not closingproxysql_adminon failure paths) were flagged in a previous review.
30-33: Minor: Resource leak on admin connection failure.If
mysql_real_connectfails,proxysql_adminwas successfully initialized but is not closed before returning.Proposed fix
if (!mysql_real_connect(proxysql_admin, cl.host, cl.admin_username, cl.admin_password, NULL, cl.admin_port, NULL, 0)) { fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql_admin)); + mysql_close(proxysql_admin); return -1; }
|
|
All bot feedback has been addressed. Ready for review when you have a chance. |
1 participant
Summary
Fixes #5306
When
SHOW WARNINGSqueries contain inline comments (e.g.,SHOW /* comment */ WARNINGS),ProxySQL incorrectly returns
warning_countThis PR fixes the issue by using
digest_text(which has comments stripped) instead of raw query text for SHOW WARNINGS detection,following the existing pattern used in
update_warning_count_from_connection().Changes
digest_textto detect SHOW WARNINGS foradd_eof()callTest
The regression test verifies that both
SHOW WARNINGSandSHOW /* comment */ WARNINGSreturnwarning_count=0in the EOF packet after a query that produces warnings.The test is excluded from
mysql-query_digests=0andmysql-query_digests_keep_comment=1groups,because
digest_textis unavailable or contains comments in these configurations.a609bc9
This is consistent with other
digest_text-based detection logic in the codebase (e.g.,update_warning_count_from_connection()).Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.