Skip to content

Fix #5306 - SHOW WARNINGS with inline comments incorrectly sets warning_count #5307

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.

Sign up for GitHub

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

Open
yuji-hatakeyama wants to merge 5 commits into
base: v3.0
Choose a base branch
from
Open

Fix #5306 - SHOW WARNINGS with inline comments incorrectly sets warning_count #5307

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ea01a83
Fix #5306: Add regression test for SHOW WARNINGS with comments
yuji-hatakeyama Jan 19, 2026
0975819
Fix #5306: Use digest_text to suppress warning_count for SHOW WARNINGS
yuji-hatakeyama Jan 19, 2026
a609bc9
Exclude test from groups where digest_text cannot properly detect SHO…
yuji-hatakeyama Jan 19, 2026
0124c55
Remove unused headers from reg_test_5306 test
yuji-hatakeyama Jan 20, 2026
28037bc
Add NULL check for mysql_store_result in reg_test_5306 test
yuji-hatakeyama Jan 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion lib/mysql_connection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1825,7 +1825,13 @@ MDB_ASYNC_ST MySQL_Connection::handler(short event) {
update_warning_count_from_connection();
// we reach here if there was no error
// exclude warning_count from the OK/EOF packet for the ‘SHOW WARNINGS’ statement
MyRS->add_eof(query.length == 13 && strncasecmp(query.ptr, "SHOW WARNINGS", 13) == 0);
bool is_show_warnings = false;
if (myds && myds->sess && myds->sess->CurrentQuery.QueryParserArgs.digest_text) {
const char* dig_text = myds->sess->CurrentQuery.QueryParserArgs.digest_text;
const size_t dig_len = strlen(dig_text);
is_show_warnings = (dig_len == 13 && strncasecmp(dig_text, "SHOW WARNINGS", 13) == 0);
Comment on lines +1831 to +1832
Copy link

@gemini-code-assist gemini-code-assist bot Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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');

Copy link
Author

@yuji-hatakeyama yuji-hatakeyama Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping the current style for consistency with the rest of the codebase

e.g.,

proxysql/lib/mysql_connection.cpp

Line 2795 in 2637d28

(dig_len == 13 && strncasecmp(dig, "SHOW WARNINGS", 13) == 0))) {

}
MyRS->add_eof(is_show_warnings);
NEXT_IMMEDIATE(ASYNC_QUERY_END);
}
}
Expand Down
1 change: 1 addition & 0 deletions test/tap/groups/groups.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
"reg_test_4399-stats_mysql_query_digest-t" : [ "default-g2","mysql-auto_increment_delay_multiplex=0-g2","mysql-multiplexing=false-g2","mysql-query_digests=0-g2","mysql-query_digests_keep_comment=1-g2" ],
"reg_test_4402-mysql_fields-t" : [ "default-g2","mysql-auto_increment_delay_multiplex=0-g2","mysql-multiplexing=false-g2","mysql-query_digests=0-g2","mysql-query_digests_keep_comment=1-g2" ],
"reg_test_4556-ssl_error_queue-t" : [ "default-g2","mysql-auto_increment_delay_multiplex=0-g2","mysql-multiplexing=false-g2","mysql-query_digests=0-g2","mysql-query_digests_keep_comment=1-g2" ],
"reg_test_5306-show_warnings_with_comment-t" : [ "default-g2","mysql-auto_increment_delay_multiplex=0-g2","mysql-multiplexing=false-g2","mysql-query_digests=0-g2","mysql-query_digests_keep_comment=1-g2" ],
"reg_test_compression_split_packets-t" : [ "default-g2","mysql-auto_increment_delay_multiplex=0-g2","mysql-multiplexing=false-g2","mysql-query_digests=0-g2","mysql-query_digests_keep_comment=1-g2" ],
"reg_test_fast_forward_split_packet-t" : [ "default-g2","mysql-auto_increment_delay_multiplex=0-g2","mysql-multiplexing=false-g2","mysql-query_digests=0-g2","mysql-query_digests_keep_comment=1-g2" ],
"reg_test_mariadb_metadata_check-t" : [ "default-g2","mysql-auto_increment_delay_multiplex=0-g2","mysql-multiplexing=false-g2","mysql-query_digests=0-g2","mysql-query_digests_keep_comment=1-g2" ],
Expand Down
89 changes: 89 additions & 0 deletions test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/**
* @file reg_test_5306-show_warnings_with_comment-t.cpp
* @brief This test verifies that SHOW WARNINGS with inline comments does not incorrectly return warning_count in EOF packet.
*/

#include <stdio.h>
#include <unistd.h>
#include <string>
#include <thread>
Copy link

@gemini-code-assist gemini-code-assist bot Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Copy link
Author

@yuji-hatakeyama yuji-hatakeyama Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0124c55 .

#include "mysql.h"
#include "mysqld_error.h"
#include "tap.h"
#include "command_line.h"
#include "utils.h"

int main(int argc, char** argv) {
CommandLine cl;

if (cl.getEnv()) {
diag("Failed to get the required environmental variables.");
return -1;
}

plan(4);

// Initialize Admin connection
MYSQL* proxysql_admin = mysql_init(NULL);
if (!proxysql_admin) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql_admin));
return -1;
}
// Connnect to ProxySQL Admin
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));
return -1;
}

MYSQL_QUERY(proxysql_admin, "SET mysql-handle_warnings=1");
MYSQL_QUERY(proxysql_admin, "LOAD MYSQL VARIABLES TO RUNTIME");

// Initialize ProxySQL connection
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();
}
Comment on lines +39 to +48
Copy link

@coderabbitai coderabbitai bot Jan 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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().

Copy link
Author

@yuji-hatakeyama yuji-hatakeyama Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same pattern in other tests
(e.g.,

proxysql/test/tap/tests/reg_test_4072-show-warnings-t.cpp

Lines 44 to 51 in 2637d28

fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql));
return -1;
}
// Connect to ProxySQL
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();
) .

Since these are test files and resources are freed on process exit anyway. if worth fixing, perhaps better addressed in a separate PR.

Copy link

@coderabbitai coderabbitai bot Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).


// Query that produces a warning (truncation warning)
const char* WARNING_QUERY = "SELECT CAST('abc' AS DOUBLE)";

// Test cases: SHOW WARNINGS without and with comment
const char* show_warnings_queries[] = {
"SHOW WARNINGS",
"SHOW /* comment */ WARNINGS"
};

for (const char* show_query : show_warnings_queries) {
MYSQL_QUERY(proxysql, WARNING_QUERY);
MYSQL_RES* res = mysql_store_result(proxysql);
mysql_free_result(res);

unsigned int warning_count_after_query = mysql_warning_count(proxysql);
diag("After WARNING_QUERY: warning_count=%u", warning_count_after_query);

MYSQL_QUERY(proxysql, show_query);
res = mysql_store_result(proxysql);
unsigned int row_count = mysql_num_rows(res);
mysql_free_result(res);

unsigned int warning_count_after_show = mysql_warning_count(proxysql);
diag("After '%s': warning_count=%u, rows=%u", show_query, warning_count_after_show, row_count);

ok(warning_count_after_query == 1,
"WARNING_QUERY should produce warning. warning_count=%u", warning_count_after_query);

ok(warning_count_after_show == 0,
"'%s' should return warning_count=0 in EOF packet. Actual=%u", show_query, warning_count_after_show);
}

mysql_close(proxysql);
mysql_close(proxysql_admin);

return exit_status();
}