WIP: Add new Bison based SQL parser for SET statements - #5088
Conversation
|
Can one of the admins verify this patch? |
8802a8b to
a5fba94
Compare
|
- Updated command with `-DWITHOUT_SERVER=ON` preventing unnecessary compilations. - Compatibility seems broken with CMake >= 4.0. For now, 'CMake' version 3.x should be used for building the tests locally.
- Updated command with `-DWITHOUT_SERVER=ON` preventing unnecessary compilations.
Error was "Use of undeclared identifier 'uint16_t'".
- Added initial SQL parser and abstractions. - Added utilities for integration with current SET processing. - Added new value (3) for 'mysql-set_parser_algorithm'. This mode replaces the previous regex based SET parser (MySQL_Set_Stmt_Parser) in favor of the new SQL Parser. - Added example of expression verification for set statements handling 'sql_mode'. This new verification improves previous logic, and fixes previous logical failures of the parsing. There are extra TODO comments left with potential improvements for these kind of verifications in the future.
This commit performs some header cleanup and isolates 'Session_Regex'
('match_regexes') into isolated files. This separation of utilities
allows unit testing for all those function/classes which are not
necessary bounded to deeply interconnected classes logic (like
'MySQL_Session', 'MySQL_Variables', etc...).
Attempting to use these classes in unit tests will inevitable result in
linking issues due to their interconnection and current relationships
with globals. Meanwhile, the utility files can be isolated and linked
with any test file.
Following these principles, 'match_regexes' are now global objects
(singletons), which are isolated and complete by construction. Same goes
for 'ignore_vars' and 'variables_regexp' used for 'SET' statements
matching.
Some additional unnecessary headers cleanup is also performed in the
commit.
Right now used in TAP tests.
Adds a new TAP tests that checks the parity between the previous REGEX based SET parser and the new SQL SET parser.
- Added multi-statement support, with improved semicolon handling. - Multiple memory leaks fixed. The leaks derived from incomplete/invalid rules in combination with the current C++ Union Bison API. - Fixed support for 'SET CHARSET' syntax. - Multiple format fixes.
e7452d3 to
648104a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/tap/tests/tokenizer_payloads/regular_tokenizer_digests.hjson (1)
128-139:⚠️ Potential issue | 🟡 MinorComment contradicts expected digest values.
Line 128 states "Just one space preserved before and after (';')" but the updated expected digests on lines 136-139 show no space before the semicolon (
FOR UPDATE; SELECT ?). Either update the comment to reflect the new behavior or verify the expected digest values are correct.lib/MySQL_Protocol.cpp (1)
2549-2554:⚠️ Potential issue | 🟡 MinorFix typo in handshake debug message.
Minor log typo (“Hanshake”) makes grep/alerts harder.🛠️ Suggested fix
- "Hanshake in progress session_id=%u user=\"%s\" password=\"%s\" client_pass=\"%s\" scramble=\"%s\"" + "Handshake in progress session_id=%u user=\"%s\" password=\"%s\" client_pass=\"%s\" scramble=\"%s\""include/query_processor.h (1)
181-228:⚠️ Potential issue | 🟠 MajorReset
query_detailsininit()to avoid stale parser output.
Query_Processor_Output::init()clears the other fields but leavesquery_detailsuntouched, so reused instances can carry parser results from a previous query. That can misroute or mis-handle later statements.🔧 Proposed fix
void init() { ptr=NULL; size=0; @@ comment=NULL; // `#643` firewall_whitelist_mode = WUS_NOT_FOUND; create_new_conn=0; + query_details = {}; }include/MySQL_Variables.h (1)
41-41:⚠️ Potential issue | 🟡 Minor
inlineon declaration without definition in header.
server_get_hashis declaredinlinebut the definition isn't visible in this header. Theinlinekeyword on a declaration (without definition) in a header is unusual - either provide the inline definition here or remove theinlinekeyword and define it in the .cpp file.🐛 Remove inline from declaration
- inline uint32_t server_get_hash(MySQL_Session* session, int idx) const; + uint32_t server_get_hash(MySQL_Session* session, int idx) const;
🤖 Fix all issues with AI agents
In `@common_mk/openssl_flags.mk`:
- Around line 24-25: The find commands used to set LIB_SSL_PATH and
LIB_CRYPTO_PATH use the invalid GNU find option `--maxdepth`; update both
commands to use the correct single-dash `-maxdepth` option so they run correctly
against SSL_LDIR (i.e., modify the assignments to LIB_SSL_PATH and
LIB_CRYPTO_PATH to call find $(SSL_LDIR) -maxdepth 1 -name "libssl.so" ... and
find $(SSL_LDIR) -maxdepth 1 -name "libcrypto.so" ... respectively), preserving
the rest of the pipeline (2>/dev/null | head -n 1).
In `@include/proxysql_utils.h`:
- Around line 313-399: The rvalue operator()(template..., T<A>&&) in struct
_b_h_filter and the two free-function overloads filter(R(*f)(const A&), const
T<A>&) and filter(R(*f)(A&&), T<A>&&) are incorrect: replace the invalid
std::transform/std::copy_if usage with logic that applies f to each element to
produce an R, then only pushes the result into the output container if that R
evaluates true; concretely, in _b_h_filter::operator()(..., T<A>&& c) and in the
free-function rvalue overload (filter(R(*f)(A&&), T<A>&&)), iterate over the
moved elements (use std::make_move_iterator or a range-based for over c with
std::move) call auto tmp = f(std::move(elem)); if (tmp)
r.push_back(std::move(tmp)); and in the const-ref overload (filter(R(*f)(const
A&), const T<A>&)), iterate by const reference, call auto tmp = f(elem); if
(tmp) r.push_back(std::move(tmp)); ensuring reserve(c.size()) is kept and
behavior is consistent across all overloads.
In `@lib/c_tokenizer.cpp`:
- Around line 986-1007: ver_num_len can grow past the cur_proc_cmnt buffer even
though characters are only copied up to FIRST_COMMENT_MAX_LENGTH-1; update the
logic in the block that reads digits (and before writing the null terminator) so
you only increment c_t_st->ver_num_len while it is strictly less than
FIRST_COMMENT_MAX_LENGTH-1 (or cap it when writing the terminator), e.g. only
copy and increment when c_t_st->ver_num_len < FIRST_COMMENT_MAX_LENGTH-1 and
when finalizing set cur_proc_cmnt[min(c_t_st->ver_num_len,
FIRST_COMMENT_MAX_LENGTH-1)] = '\0' so cur_proc_cmnt and later uses
(cur_cmd_ver) cannot be indexed out of bounds; reference c_t_st->ver_num_len,
cur_proc_cmnt, FIRST_COMMENT_MAX_LENGTH and the digit-read branch that checks
is_digit_char(*shared_st->q).
- Around line 257-312: copy_mysql_ver_num reads past src for short inputs and
mis-parses components by reusing strcspn on the wrong pointer; update
copy_mysql_ver_num to defensively parse three numeric components by scanning
from the current pointer s to the next '.' or end (use s_end to bound scans) and
only dereference s after confirming s <= s_end, copy at most two digit
characters per component (pad with '0' when absent), advance s to just after the
next '.' (or to s_end+1) for the next component, and ensure the function
documents/assumes dst has room for 6 digits plus a terminating NUL; refer to
symbols copy_mysql_ver_num, dst, src, s, s_end, and dot_p when making these
changes.
In `@lib/Makefile`:
- Around line 117-118: The Bison-generated header is only a side-effect of the
$(MYSQL_BISON_C) rule so MySQL_Lexer.yy.c (which depends on MySQL_Parser.tab.h)
can fail to build; update the Makefile so the Bison rule is a multi-target rule
that explicitly declares both $(MYSQL_BISON_C) and $(MYSQL_BISON_H) as targets
(dependent on $(MYSQL_PARSER).y and the same headers), leaving the bison command
unchanged, so MySQL_Parser.tab.h is produced as a first-class target and Make
can satisfy the MySQL_Lexer.yy.c dependency.
In `@lib/mysql_connection.cpp`:
- Around line 3038-3055: The call to ProcessQueryAndSetStatusFlags_UserVariables
currently passes myds->sess->qpo unguarded and can dereference null; update the
block around mul and the subsequent calls to check that myds, myds->sess and
myds->sess->qpo are non-null before calling
ProcessQueryAndSetStatusFlags_UserVariables(myds->sess->qpo, query_digest_text),
and if any is null call the digest-only fallback (i.e., keep calling
ProcessQueryAndSetStatusFlags_Warnings(query_digest_text) or the existing
digest-based overload) instead; reference the symbols myds, sess, qpo,
ProcessQueryAndSetStatusFlags_UserVariables to locate and change the logic
accordingly.
- Around line 2753-2767: The code in
MySQL_Connection::IsKeepMultiplexEnabledVariables uses rfind(...) which can read
past vn's buffer and is case-sensitive; fix by performing explicit
length-checked, case-insensitive comparisons against vn: for the '@' case ensure
v.first.size() > 1 and v.first.size() - 1 >= vn.size(), compute the start offset
= 1 + (v.first.size() - 1 - vn.size()) and call a case-insensitive comparison
(e.g. strncasecmp) between v.first.c_str() + start_offset and vn.data() for
vn.size() bytes; for the non-'@' case compare v.first and vn with a
case-insensitive equality (e.g. strcasecmp or strncasecmp with vn.size()); use
get_keep_mult_vars(mysql_thread___keep_multiplexing_variables), keep_mult_vars,
s->vars_assigns and v.first/vn names to locate the exact spot and avoid any
pointer/length arithmetic that could read beyond vn.
- Around line 2727-2732: The trim function's substr length is incorrect: in
string_view trim(const string_view& s) you should compute the length as (f_pos -
s_pos + 1) instead of using f_pos + 1, and handle the all-whitespace case by
returning an empty string_view when s_pos or f_pos is npos; update trim to check
for s.find_first_not_of(...) == npos (or f_pos == npos) and return {} in that
case, otherwise return s.substr(s_pos, f_pos - s_pos + 1) so leading and
trailing whitespace are correctly removed.
In `@lib/MySQL_Lexer.l`:
- Around line 62-72: The bug is that when ver_len == 6 the strncpy uses yytext
instead of yytext + 3, so change the copy to use yytext + 3 as the source and
ensure the null terminator is set for the ver buffer (size 7); update the
strncpy call in the ver_len == 6 branch to copy 6 bytes from (yytext + 3) into
ver and then set ver[6] = '\0' (references: variables ver, ver_len and yytext in
the MySQL_Lexer.l snippet).
In `@lib/MySQL_Parser.y`:
- Around line 207-213: The reduction for the grammar rule "input_stmt_list
TOKEN_SEMICOLON input_stmt" is incorrectly adding the prior list node ($1) into
the AST root causing null/duplicate children; update the action in that rule
(the block referencing parser_context->ast_root_->add_child($1) and
add_child($3)) to stop adding $1 and only append the new statement ($3) to
parser_context->ast_root_ (keep setting $$ as nullptr as before and leave the
add_child($3) call intact).
In `@lib/MySQL_Session_Utils.cpp`:
- Around line 4-12: The global std::array mysql_match_regexes currently calls
get_mysql_variables_regexp() at static init time, creating a static
initialization order risk; change mysql_match_regexes into a function-local
static (e.g. a getter like get_mysql_match_regexes()) that constructs and
returns the std::array<Session_Regex,4> inside the function so
get_mysql_variables_regexp() is invoked after its TU is initialized; update call
sites to use the new getter and keep the same Session_Regex contents and
ordering when building the array.
In `@lib/MySQL_SET_Parser_Utils.cpp`:
- Around line 274-279: p_match_regex_3 currently calls get_node(node, ...)
before verifying node is non-null which can dereference a null pointer; modify
p_match_regex_3 to first check if node == nullptr and return false early, then
call get_node to populate set_charset and perform the existing check (use
get_node, set_charset.first == 0) so no dereference happens on a null node.
In `@lib/MySQL_Thread.cpp`:
- Around line 1840-1853: The SRV_VER_REGEX used by validate_mysql_version is too
restrictive and rejects real-world MariaDB/Percona version strings; update the
regex (used in validate_mysql_version) to allow uppercase letters and multiple
hyphen-separated suffix segments (e.g., change the suffix character class to
[A-Za-z0-9] and allow (?:-(?:[A-Za-z0-9]+(?:-[A-Za-z0-9]+)*) ) or otherwise
permit one or more hyphen-separated tokens) so values like "10.6.17-MariaDB" and
"8.0.36-24-Percona" validate correctly when assigning variables.server_version
for mysql-server_version handling in the code that calls validate_mysql_version.
In `@lib/PgSQL_Session_Utils.cpp`:
- Around line 4-10: The global pgsql_match_regexes array directly calls
get_pgsql_variables_regexp() during static init and can run before
pgsql_variables_regexp is constructed; fix by replacing the global
std::array<Session_Regex,4> pgsql_match_regexes with a function-local static
getter (e.g. a function get_pgsql_match_regexes() that contains a static
std::array<Session_Regex,4> initialized using get_pgsql_variables_regexp()) so
initialization is lazy and safe; update all usages to call
get_pgsql_match_regexes() instead of the global, and ensure related
initialization functions like build_pgsql_variables_regex() and globals such as
pgsql_tracked_variables remain unchanged.
In `@lib/PgSQL_Variables_Utils.cpp`:
- Around line 16-45: The regex builder build_pgsql_variables_regex appends
ignore_vars directly to res without adding a '|' when res already contains
tracked names, causing malformed alternation; update the loop that appends each
iv from ignore_vars to check if res is non-empty and, if so, prepend or append a
'|' before adding the first iv (or use a conditional that adds "|" when
!res.empty()), then continue appending subsequent iv entries with the existing
separator logic so pgsql_tracked_variables and ignore_vars are properly
separated in the resulting regex.
In `@lib/proxysql_utils.cpp`:
- Around line 467-471: The trim(string&& s) function can call s.find_last_not_of
and then erase(npos+1) which throws on all-whitespace input; update trim to
guard against npos by checking the results of find_first_not_of and
find_last_not_of (or at least test last_not == string::npos) before calling
erase: if the string is all whitespace return an empty string immediately,
otherwise perform the two erase calls using the valid indices; reference trim,
find_first_not_of, find_last_not_of, erase and string::npos in your change.
- Around line 800-804: The call to RE2::FullMatch in validate_mysql_version
should use the fully qualified re2 namespace; update the call in
validate_mysql_version(const char* v) to invoke re2::RE2::FullMatch instead of
RE2::FullMatch and keep the existing re2::StringPiece(v) and *SRV_VER_REGEX
usage so it compiles consistently with the other re2 symbols like const
re2::LazyRE2 SRV_VER_REGEX.
- Around line 798-799: The SRV_VER_REGEX is too restrictive and rejects
real-world server strings; update validation so it mirrors
mysql_get_server_version() behavior: accept three dot-separated numeric
components followed by any suffix (allowing uppercase letters, multiple hyphens,
colons, plus signs, etc.) instead of only a single lowercase hyphenated token.
Concretely, replace or relax SRV_VER_REGEX to match
"^\\d{1,2}\\.\\d{1,2}\\.\\d{1,2}" followed by an optional suffix of non-space
characters (or remove the regex check and call mysql_get_server_version()
parsing logic) so config assignments no longer get silently rejected. Ensure
changes reference SRV_VER_REGEX and mysql_get_server_version() so the validator
and parser align.
In `@test/tap/tests/Makefile`:
- Around line 238-245: The Makefile uses a misspelled macro
-DEXCLUDE_TRACKING_VARAIABLES in the test targets test_set_parser_parity-t and
test_set_parser_sql_mode-t which prevents the intended conditional compilation;
update the flag to the correct -DEXCLUDE_TRACKING_VARIABLES in those target
lines (the ones invoking $(CXX) for test_set_parser_parity-t and
test_set_parser_sql_mode-t that include MySQL_Set_Stmt_Parser.cpp /
proxysql_global.cpp) so the exclusion macro is set properly during compilation.
In `@test/tap/tests/test_set_parser_parity.cpp`:
- Around line 302-325: The CSV path construction for SET_TESTING_CSV_PATH is
incorrect because concatenating get_env("TAP_WORKDIR") + "./set_testing-240.csv"
can produce "<dir>./set_testing-240.csv" when TAP_WORKDIR lacks a trailing
slash; update the initialization so it safely joins the workdir and filename
(handle empty/null workdir) using a proper path join strategy (e.g.,
std::filesystem::path or explicitly append a '/' when missing) so
SET_TESTING_CSV_PATH always becomes "<workdir>/set_testing-240.csv" and
subsequent open() calls in get_valid_queries() succeed.
🟡 Minor comments (6)
src/proxysql_global.cpp-57-73 (1)
57-73:⚠️ Potential issue | 🟡 MinorCross-TU static initialization order is undefined for
mysql_tracked_vars.
mysql_tracked_varsdepends onmysql_ignore_varsfrom another translation unit (lib/MySQL_Variables_Utils.cpp). While both use simple aggregate initialization from literals—reducing practical risk—the initialization order remains undefined per the C++ standard. To eliminate this dependency, consider makingmysql_ignore_varsa function-local static withinget_mysql_ignore_vars(), or lazily initializemysql_tracked_varson first use.include/proxysql_structs.h-4-9 (1)
4-9:⚠️ Potential issue | 🟡 MinorGuard
<cstdint>for C compatibility.
<cstdint>is C++-only and should be wrapped with#ifdef __cplusplusto prevent compilation issues if C source files ever include this header.🛠️ Suggested fix
-#include <cstdint> `#include` <strings.h> `#include` <stdint.h> +#ifdef __cplusplus +#include <cstdint> +#endifinclude/MySQL_Session_Utils.h-4-8 (1)
4-8:⚠️ Potential issue | 🟡 MinorAdd
#ifdef __cplusplusguards to protect against C inclusionThe header contains C++-only features (
std::array,Session_Regex) but lacks#ifdef __cplusplusguards. While no C files currently include this header, it's vulnerable to future accidental C inclusion, which would cause build failures. Wrap the C++-specific content with#ifdef __cplusplusguards to prevent misuse:`#ifndef` MYSQL_SESSION_UTILS_H `#define` MYSQL_SESSION_UTILS_H `#ifdef` __cplusplus `#include` "Base_Session_Utils.h" `#include` <array> extern std::array<Session_Regex,4> mysql_match_regexes; `#endif` // __cplusplus `#endif` // MYSQL_SESSION_UTILS_Hinclude/MySQL_SET_Parser_Utils.h-174-179 (1)
174-179:⚠️ Potential issue | 🟡 MinorReturn type mismatch:
is_space_charreturnscharbut documentation says it returnsbool.The function is documented as returning
True if...but the signature returnschar. This should returnboolfor semantic clarity.🐛 Fix return type
-char is_space_char(char c); +bool is_space_char(char c);include/MySQL_AST.h-195-283 (1)
195-283:⚠️ Potential issue | 🟡 MinorMissing
NODE_LEFT_SHIFT_OPERATORandNODE_RIGHT_SHIFT_OPERATORinto_string().These node types are defined in the enum (lines 53-54) but not handled in the
to_string()function, causing them to return"UNHANDLED_TYPE(...)".🐛 Add missing cases
else if (t == NodeType::NODE_OPERATOR) { return "OPERATOR"; } + else if (t == NodeType::NODE_LEFT_SHIFT_OPERATOR) { return "LEFT_SHIFT_OPERATOR"; } + else if (t == NodeType::NODE_RIGHT_SHIFT_OPERATOR) { return "RIGHT_SHIFT_OPERATOR"; } else if (t == NodeType::NODE_QUALIFIED_IDENTIFIER) { return "QUALIFIED_IDENTIFIER"; }test/tap/tests/test_set_parser_sql_mode.cpp-329-334 (1)
329-334:⚠️ Potential issue | 🟡 MinorRemove duplicate insertion of
valid_sql_mode_subexprs.
valid_sql_mode_subexprsis appended twice, doubling test count and work without adding coverage.🔧 Suggested fix
std::copy(set_queries.begin(), set_queries.end(), std::back_inserter(valid_stmts)); std::copy(setparser_queries.begin(), setparser_queries.end(), std::back_inserter(valid_stmts)); std::copy(valid_sql_mode_subexprs.begin(), valid_sql_mode_subexprs.end(), std::back_inserter(valid_stmts)); - std::copy(valid_sql_mode_subexprs.begin(), valid_sql_mode_subexprs.end(), std::back_inserter(valid_stmts)); std::copy(invalid_sql_mode_subexprs.begin(), invalid_sql_mode_subexprs.end(), std::back_inserter(invalid_stmts));
🧹 Nitpick comments (19)
test/tap/tests/tokenizer_payloads/regular_tokenizer_digests.hjson (1)
155-166: Consider adding s2, s3, s4 expected digests for completeness.This test block only defines
s1. Other blocks in this file typically define multiple digest variants. If all algorithms should produce an empty string for these inputs, consider explicitly addings2,s3, ands4for consistency and to ensure all tokenizer variants are tested.include/btree.h (1)
107-114: Remove duplicate#include <cstdint>directives.This file now has three includes of
<cstdint>(lines 107, 109, and 114). The newly added one at line 109 is redundant. While harmless due to standard header guards, it adds unnecessary clutter.🧹 Proposed cleanup
`#include` <sys/types.h> `#include` <cstdint> `#include` <algorithm> -#include <cstdint> `#include` <functional> `#include` <iostream> `#include` <iterator> `#include` <limits> -#include <cstdint> `#include` <type_traits>include/c_tokenizer.h (3)
13-16: Clarify the version format and buffer size.The macro
MYSQL_SERVER_VER_MAX_LENGTH = 6appears to accommodate only the numeric portion (MMnnrr, e.g., "080033"). The doc comment oncopy_mysql_ver_nummentions format'MMnnrr-suffix', suggesting a suffix exists. Ensure callers allocateMYSQL_SERVER_VER_MAX_LENGTH + 1bytes (for null terminator) and that the suffix is intentionally excluded from this length.Consider adding a brief comment clarifying the format:
`#ifndef` MYSQL_SERVER_VER_MAX_LENGTH +// Length of numeric version string "MMnnrr" (e.g., "080033" for 8.0.33), excluding null terminator `#define` MYSQL_SERVER_VER_MAX_LENGTH 6 `#endif` /* MYSQL_SERVER_VER_MAX_LENGTH */
42-42: Document memory ownership forserver_version.The new
char* server_versionfield is a raw pointer. The struct's ownership semantics should be documented: who allocates this memory, who is responsible for freeing it, and what is the expected lifetime? Without this, there's risk of memory leaks or use-after-free bugs.📝 Suggested documentation
typedef struct _options { bool lowercase; bool replace_null; bool replace_number; bool keep_comment; int grouping_limit; int groups_grouping_limit; int max_query_length; + /** + * `@brief` MySQL server version string (format: "MMnnrr", e.g., "080033"). + * `@note` Caller-owned; must remain valid for the lifetime of this struct. + * May be NULL if version tracking is not needed. + */ char* server_version; } options;
69-73: Document the required destination buffer size.The
copy_mysql_ver_numfunction should specify the minimum buffer size requirement for parameterdto prevent buffer overflows.📝 Enhanced documentation
/** * `@brief` Copies a MySQL version number of format 'MMnnrr-suffix' from 's' to 'd'. + * `@param` d Destination buffer, must be at least MYSQL_SERVER_VER_MAX_LENGTH + 1 bytes. + * `@param` s Source version string (may include suffix after '-', which is ignored). * `@returns` The parameter 'd'. */ char* copy_mysql_ver_num(char* d, const char* s);test/deps/mariadb-connector-c/ma_global.h.patch (1)
7-9: Inconsistent__STDC_VERSION__check across patches.This patch uses
202311L(the final C23 standard value), whiledeps/mariadb-client-library/ma_global.h.patchuses202300L(a draft value). Both achieve the goal, but for consistency and correctness, consider aligning both patches to use202311Lsince that's the official C23 value.Suggested alignment in deps/mariadb-client-library/ma_global.h.patch
-#if !defined(bool) && !defined(bool_defined) && (!defined(HAVE_BOOL) || !defined(__cplusplus)) && (__STDC_VERSION__ < 202300L) +#if !defined(bool) && !defined(bool_defined) && (!defined(HAVE_BOOL) || !defined(__cplusplus)) && (__STDC_VERSION__ < 202311L)include/PgSQL_Session_Utils.h (1)
8-8: Consider documenting the array size constant.The magic number
4represents the number of regex patterns but lacks documentation. A brief comment explaining what these 4 patterns match would improve maintainability.Example documentation
+// Array of 4 regex patterns for matching: [describe the 4 pattern purposes] extern std::array<Session_Regex,4> pgsql_match_regexes;test/deps/Makefile (1)
53-56: Duplicate-DWITHOUT_SERVER=ONflag.The option
-DWITHOUT_SERVER=ONappears twice in the cmake command (lines 53 and 54). While harmless, this duplication should be cleaned up.Remove duplicate flag
cd mysql-connector-c-8.4.0/mysql-connector-c && cmake . -DWITHOUT_SERVER=ON -DFORCE_INSOURCE_BUILD=1 \ - -DCMAKE_BUILD_TYPE=RelWithDebInfo -DWITHOUT_SERVER=ON -DDOWNLOAD_BOOST=1 \ + -DCMAKE_BUILD_TYPE=RelWithDebInfo -DDOWNLOAD_BOOST=1 \ -DWITH_BOOST=./mysql-server/downloads/ -DWITH_UNIT_TESTS=OFF \ -DCMAKE_CXX_FLAGS_RELWITHDEBINFO="-O0 -ggdb -DNDEBUG -fPIC" -DCMAKE_POLICY_VERSION_MINIMUM=3.5lib/Makefile (1)
126-127: Track grammar header changes when compilingMySQL_Parser.oo.
IfMySQL_Parser.cppincludes the generated header, incremental builds won’t rebuild after grammar edits. Add the header to the prerequisites.🛠️ Suggested fix
-$(ODIR)/$(MYSQL_PARSER).oo: $(MYSQL_PARSER).cpp $(ODIR)/MySQL_Lexer.yy.oo +$(ODIR)/$(MYSQL_PARSER).oo: $(MYSQL_PARSER).cpp $(MYSQL_BISON_H) $(ODIR)/MySQL_Lexer.yy.oo $(CXX) -fPIC -c -o $@ $< $(MYCXXFLAGS) $(CXXFLAGS)include/Base_Session_Utils.h (1)
12-21: Add explicit copy/move deletion to prevent future accidental copies ofSession_Regex.
The class owns raw pointers with a custom destructor. While current code safely constructs array elements in-place without copying, explicitly deleting copy/move operations prevents future maintainers from accidentally triggering double-free bugs.Suggested header change
class Session_Regex { private: void* opt; void* re; char* s; public: Session_Regex(const char* p); ~Session_Regex(); + Session_Regex(const Session_Regex&) = delete; + Session_Regex& operator=(const Session_Regex&) = delete; + Session_Regex(Session_Regex&&) = delete; + Session_Regex& operator=(Session_Regex&&) = delete; bool match(const char* m); };include/mysql_connection.h (1)
260-284: Add coverage for the newquery_details_tmultiplexing path.The doc notes this overload is untested. Please add tests for SET queries with/without keep-multiplexing variables.
lib/MySQL_Lexer.l (1)
43-48: Unused scanner stateHINTdeclared but never entered.The
HINTstate is declared but there's no rule in the lexer that transitions to it viaBEGIN(HINT). This appears to be dead code or an incomplete feature.Consider removing unused state or adding transition rules
%x COMMENT %x COMMAND -%x HINT %x SQSTRING %x DQSTRING %x BTIDENTinclude/MySQL_Parser.h (2)
78-80: Consider makingast_root_private.
ast_root_is declared public but there's alreadyinternal_set_ast()for modifying it. Theparse()method returns the AST viastd::move(ast_root_), so external access toast_root_after parsing would return nullptr. Making it private would better encapsulate the internal state.♻️ Move ast_root_ to private section
- /** - * `@brief` The root node of the Abstract Syntax Tree (AST). - */ - std::unique_ptr<AstNode> ast_root_; - private: + /** + * `@brief` The root node of the Abstract Syntax Tree (AST). + */ + std::unique_ptr<AstNode> ast_root_; /** * `@brief` Vector with the error messages encountered during parsing. */
17-19: Document lifetime requirements forsrv_verpointer.
ParserOpts::srv_veris a non-owningconst char*. The caller must ensure the pointed-to string remains valid for the duration of theparse()call. Consider documenting this requirement or usingstd::string_viewfor clearer semantics.include/MySQL_AST.h (2)
136-138: Consider usingstd::vector<std::unique_ptr<AstNode>>for safer memory management.The
childrenvector holds raw owning pointers which requires manual deletion in the destructor. Usingstd::unique_ptrwould provide automatic cleanup and clearer ownership semantics, especially important if exceptions occur during AST construction.♻️ Use unique_ptr for children
- std::vector<AstNode*> children {}; + std::vector<std::unique_ptr<AstNode>> children {};This would require updating
add_childto acceptstd::unique_ptr<AstNode>and the destructor would become trivial (default).
286-298: Consider parameterizingprint_astwithstd::ostream&for flexibility.The function uses
std::coutdirectly, which limits its use in contexts where output should go elsewhere (e.g., logging systems, string streams for testing).♻️ Add ostream parameter
-inline void print_ast(const AstNode* n, std::string&& prefix = {}, bool is_last = true) { +inline void print_ast(const AstNode* n, std::ostream& os = std::cout, std::string prefix = {}, bool is_last = true) { if (!n) { return; } - std::cout << prefix; - std::cout << (is_last ? "`-- " : "|-- "); - std::cout << "[" << to_string(n->type) << "] " << + os << prefix; + os << (is_last ? "`-- " : "|-- "); + os << "[" << to_string(n->type) << "] " << (n->value.empty() ? "" : "('" + n->value + "')") << "\n";lib/MySQL_Parser.cpp (2)
14-18: Type mismatch inYY_EXTRA_TYPEtypedef.Line 17 declares
YY_EXTRA_TYPEasMySQLParser::ParserOpts*, but line 20 showsmysql_yylex_init_extratakesMySQLParser::Parser*. This typedef is unused and misleading.♻️ Remove unused typedef or correct the type
// Forward declaration for the opaque Flex buffer type struct yy_buffer_state; typedef struct yy_buffer_state *YY_BUFFER_STATE; -typedef MySQLParser::ParserOpts* YY_EXTRA_TYPE;
52-58: Location information fromyylocis ignored in error reporting.The overload accepting
MYSQL_YYLTYPE* yylocignores the location data and calls the sameinternal_add_error(msg)as the simpler overload. Consider usinginternal_add_error_at()to include line/column information in error messages.♻️ Use location-aware error reporting
void mysql_yyerror(MYSQL_YYLTYPE* yyloc, yyscan_t, MySQLParser::Parser* parser_context, const char* msg) { if (!parser_context) { assert(0 && "Invalid param: Context must be 'this' from 'mysqlparser::parser'."); } else { - parser_context->internal_add_error(msg); + if (yyloc) { + parser_context->internal_add_error_at(msg, yyloc->first_line, yyloc->first_column); + } else { + parser_context->internal_add_error(msg); + } } }include/MySQL_Session.h (1)
574-584: Consider adding TODO to track deprecation of originalunable_to_parse_set_statement.The documentation mentions this should replace the original function after the legacy alternative is deprecated. Adding a TODO or tracking issue would help ensure this refactor is completed.
| if (c_t_st->proc_ver_mark && is_digit_char(*shared_st->q)) { | ||
| if (c_t_st->ver_num_len < FIRST_COMMENT_MAX_LENGTH - 1) { | ||
| cur_proc_cmnt[c_t_st->ver_num_len] = *shared_st->q; | ||
| } | ||
|
|
||
| c_t_st->ver_num_len++; | ||
| } else { | ||
| // consume final space-char delim; avoids space duplication on multi-stmt digests | ||
| if ( | ||
| shared_st->q_cur_pos <= (shared_st->q_len-2) && | ||
| c_t_st->proc_ver_mark && is_space_char(*shared_st->q) | ||
| ) { | ||
| shared_st->q += 1; | ||
| shared_st->q_cur_pos += 1; | ||
| } | ||
|
|
||
| c_t_st->proc_ver_mark = false; | ||
|
|
||
| // process version number; /*!MMmmrr */ | ||
| if (c_t_st->ver_num_len) { | ||
| cur_proc_cmnt[c_t_st->ver_num_len] = 0; | ||
| char cur_cmd_ver[MYSQL_SERVER_VER_MAX_LENGTH + 1] { 0 }; |
There was a problem hiding this comment.
Prevent out-of-bounds when cmd version marks are longer than the buffer
ver_num_len keeps incrementing even after the buffer cap, but later it’s used as an index for the null terminator. A long digit sequence will write past cur_proc_cmnt. Increment only when in range.
🔒 Suggested fix
- if (c_t_st->proc_ver_mark && is_digit_char(*shared_st->q)) {
- if (c_t_st->ver_num_len < FIRST_COMMENT_MAX_LENGTH - 1) {
- cur_proc_cmnt[c_t_st->ver_num_len] = *shared_st->q;
- }
-
- c_t_st->ver_num_len++;
- } else {
+ if (c_t_st->proc_ver_mark && is_digit_char(*shared_st->q)) {
+ if (c_t_st->ver_num_len < FIRST_COMMENT_MAX_LENGTH - 1) {
+ cur_proc_cmnt[c_t_st->ver_num_len++] = *shared_st->q;
+ }
+ } else {🤖 Prompt for AI Agents
In `@lib/c_tokenizer.cpp` around lines 986 - 1007, ver_num_len can grow past the
cur_proc_cmnt buffer even though characters are only copied up to
FIRST_COMMENT_MAX_LENGTH-1; update the logic in the block that reads digits (and
before writing the null terminator) so you only increment c_t_st->ver_num_len
while it is strictly less than FIRST_COMMENT_MAX_LENGTH-1 (or cap it when
writing the terminator), e.g. only copy and increment when c_t_st->ver_num_len <
FIRST_COMMENT_MAX_LENGTH-1 and when finalizing set
cur_proc_cmnt[min(c_t_st->ver_num_len, FIRST_COMMENT_MAX_LENGTH-1)] = '\0' so
cur_proc_cmnt and later uses (cur_cmd_ver) cannot be indexed out of bounds;
reference c_t_st->ver_num_len, cur_proc_cmnt, FIRST_COMMENT_MAX_LENGTH and the
digit-read branch that checks is_digit_char(*shared_st->q).
| $(MYSQL_BISON_C): $(MYSQL_PARSER).y $(IDIR)/MySQL_AST.h $(IDIR)/$(MYSQL_PARSER).h | ||
| bison -Wcounterexamples -d -v --report=all -o $(MYSQL_BISON_C) --defines=$(MYSQL_BISON_H) $(MYSQL_PARSER).y |
There was a problem hiding this comment.
Declare the generated Bison header as a build target.
MySQL_Lexer.yy.c depends on MySQL_Parser.tab.h, but the header is only a side‑effect of the .c rule. On a clean tree, make can fail with “No rule to make target …”. Make the Bison rule multi‑target so the header is explicit.
🛠️ Suggested fix
-$(MYSQL_BISON_C): $(MYSQL_PARSER).y $(IDIR)/MySQL_AST.h $(IDIR)/$(MYSQL_PARSER).h
+$(MYSQL_BISON_C) $(MYSQL_BISON_H): $(MYSQL_PARSER).y $(IDIR)/MySQL_AST.h $(IDIR)/$(MYSQL_PARSER).h
bison -Wcounterexamples -d -v --report=all -o $(MYSQL_BISON_C) --defines=$(MYSQL_BISON_H) $(MYSQL_PARSER).y📝 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_BISON_C): $(MYSQL_PARSER).y $(IDIR)/MySQL_AST.h $(IDIR)/$(MYSQL_PARSER).h | |
| bison -Wcounterexamples -d -v --report=all -o $(MYSQL_BISON_C) --defines=$(MYSQL_BISON_H) $(MYSQL_PARSER).y | |
| $(MYSQL_BISON_C) $(MYSQL_BISON_H): $(MYSQL_PARSER).y $(IDIR)/MySQL_AST.h $(IDIR)/$(MYSQL_PARSER).h | |
| bison -Wcounterexamples -d -v --report=all -o $(MYSQL_BISON_C) --defines=$(MYSQL_BISON_H) $(MYSQL_PARSER).y |
🤖 Prompt for AI Agents
In `@lib/Makefile` around lines 117 - 118, The Bison-generated header is only a
side-effect of the $(MYSQL_BISON_C) rule so MySQL_Lexer.yy.c (which depends on
MySQL_Parser.tab.h) can fail to build; update the Makefile so the Bison rule is
a multi-target rule that explicitly declares both $(MYSQL_BISON_C) and
$(MYSQL_BISON_H) as targets (dependent on $(MYSQL_PARSER).y and the same
headers), leaving the bison command unchanged, so MySQL_Parser.tab.h is produced
as a first-class target and Make can satisfy the MySQL_Lexer.yy.c dependency.
| string trim(string&& s) { | ||
| s.erase(0, s.find_first_not_of(" \n\r\t")); | ||
| s.erase(s.find_last_not_of(" \n\r\t") + 1); | ||
|
|
||
| return s; |
There was a problem hiding this comment.
trim() throws on all‑whitespace input.
When the string becomes empty, find_last_not_of returns npos, so erase(npos + 1) throws. Guard for npos.
🛠️ Suggested fix
string trim(string&& s) {
- s.erase(0, s.find_first_not_of(" \n\r\t"));
- s.erase(s.find_last_not_of(" \n\r\t") + 1);
+ const auto first = s.find_first_not_of(" \n\r\t");
+ if (first == string::npos) {
+ return {};
+ }
+ const auto last = s.find_last_not_of(" \n\r\t");
+ s.erase(last + 1);
+ s.erase(0, first);
return s;
}📝 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.
| string trim(string&& s) { | |
| s.erase(0, s.find_first_not_of(" \n\r\t")); | |
| s.erase(s.find_last_not_of(" \n\r\t") + 1); | |
| return s; | |
| string trim(string&& s) { | |
| const auto first = s.find_first_not_of(" \n\r\t"); | |
| if (first == string::npos) { | |
| return {}; | |
| } | |
| const auto last = s.find_last_not_of(" \n\r\t"); | |
| s.erase(last + 1); | |
| s.erase(0, first); | |
| return s; | |
| } |
🤖 Prompt for AI Agents
In `@lib/proxysql_utils.cpp` around lines 467 - 471, The trim(string&& s) function
can call s.find_last_not_of and then erase(npos+1) which throws on
all-whitespace input; update trim to guard against npos by checking the results
of find_first_not_of and find_last_not_of (or at least test last_not ==
string::npos) before calling erase: if the string is all whitespace return an
empty string immediately, otherwise perform the two erase calls using the valid
indices; reference trim, find_first_not_of, find_last_not_of, erase and
string::npos in your change.
| const string SET_TESTING_CSV_PATH { get_env("TAP_WORKDIR") + "./set_testing-240.csv" }; | ||
|
|
||
| rc_t<vector<string>> get_valid_queries() { | ||
| vector<string> test_qs {}; | ||
|
|
||
| std::copy(exhaustive_queries.begin(), exhaustive_queries.end(), std::back_inserter(test_qs)); | ||
| std::copy(set_queries.begin(), set_queries.end(), std::back_inserter(test_qs)); | ||
| std::copy(setparser_queries.begin(), setparser_queries.end(), std::back_inserter(test_qs)); | ||
| std::copy(valid_sql_mode_subexpr.begin(), valid_sql_mode_subexpr.end(), std::back_inserter(test_qs)); | ||
|
|
||
| char* DISABLE_SET_TESTING_CSV { getenv("DISABLE_SET_TESTING_CSV_PATH") }; | ||
|
|
||
| if (!DISABLE_SET_TESTING_CSV) { | ||
| std::fstream logfile_fs {}; | ||
|
|
||
| printf("Openning log file path:'%s'\n", SET_TESTING_CSV_PATH.c_str()); | ||
| // no scope found, defaults to session | ||
| logfile_fs.open(SET_TESTING_CSV_PATH.c_str(), std::fstream::in | std::fstream::out); | ||
|
|
||
| if (!logfile_fs.is_open() || !logfile_fs.good()) { | ||
| fprintf(stderr, "Failed to open '%s' file path=\"%s\" error=%d\n", | ||
| basename(SET_TESTING_CSV_PATH.c_str()), SET_TESTING_CSV_PATH.c_str(), errno | ||
| ); | ||
| return { EXIT_FAILURE, {} }; |
There was a problem hiding this comment.
Fix TAP_WORKDIR path join to avoid invalid CSV paths.
If TAP_WORKDIR doesn’t end with a slash, the current concatenation yields <dir>./set_testing-240.csv, which fails to open and aborts the test. Use a safe join that handles empty and non-terminated paths.
🔧 Suggested fix
-const string SET_TESTING_CSV_PATH { get_env("TAP_WORKDIR") + "./set_testing-240.csv" };
+const string SET_TESTING_CSV_PATH {
+ [] {
+ const string wd = get_env("TAP_WORKDIR");
+ if (wd.empty()) {
+ return string{"./set_testing-240.csv"};
+ }
+ if (wd.back() == '/' || wd.back() == '\\') {
+ return wd + "set_testing-240.csv";
+ }
+ return wd + "/set_testing-240.csv";
+ }()
+};📝 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.
| const string SET_TESTING_CSV_PATH { get_env("TAP_WORKDIR") + "./set_testing-240.csv" }; | |
| rc_t<vector<string>> get_valid_queries() { | |
| vector<string> test_qs {}; | |
| std::copy(exhaustive_queries.begin(), exhaustive_queries.end(), std::back_inserter(test_qs)); | |
| std::copy(set_queries.begin(), set_queries.end(), std::back_inserter(test_qs)); | |
| std::copy(setparser_queries.begin(), setparser_queries.end(), std::back_inserter(test_qs)); | |
| std::copy(valid_sql_mode_subexpr.begin(), valid_sql_mode_subexpr.end(), std::back_inserter(test_qs)); | |
| char* DISABLE_SET_TESTING_CSV { getenv("DISABLE_SET_TESTING_CSV_PATH") }; | |
| if (!DISABLE_SET_TESTING_CSV) { | |
| std::fstream logfile_fs {}; | |
| printf("Openning log file path:'%s'\n", SET_TESTING_CSV_PATH.c_str()); | |
| // no scope found, defaults to session | |
| logfile_fs.open(SET_TESTING_CSV_PATH.c_str(), std::fstream::in | std::fstream::out); | |
| if (!logfile_fs.is_open() || !logfile_fs.good()) { | |
| fprintf(stderr, "Failed to open '%s' file path=\"%s\" error=%d\n", | |
| basename(SET_TESTING_CSV_PATH.c_str()), SET_TESTING_CSV_PATH.c_str(), errno | |
| ); | |
| return { EXIT_FAILURE, {} }; | |
| const string SET_TESTING_CSV_PATH { | |
| [] { | |
| const string wd = get_env("TAP_WORKDIR"); | |
| if (wd.empty()) { | |
| return string{"./set_testing-240.csv"}; | |
| } | |
| if (wd.back() == '/' || wd.back() == '\\') { | |
| return wd + "set_testing-240.csv"; | |
| } | |
| return wd + "/set_testing-240.csv"; | |
| }() | |
| }; | |
| rc_t<vector<string>> get_valid_queries() { | |
| vector<string> test_qs {}; | |
| std::copy(exhaustive_queries.begin(), exhaustive_queries.end(), std::back_inserter(test_qs)); | |
| std::copy(set_queries.begin(), set_queries.end(), std::back_inserter(test_qs)); | |
| std::copy(setparser_queries.begin(), setparser_queries.end(), std::back_inserter(test_qs)); | |
| std::copy(valid_sql_mode_subexpr.begin(), valid_sql_mode_subexpr.end(), std::back_inserter(test_qs)); | |
| char* DISABLE_SET_TESTING_CSV { getenv("DISABLE_SET_TESTING_CSV_PATH") }; | |
| if (!DISABLE_SET_TESTING_CSV) { | |
| std::fstream logfile_fs {}; | |
| printf("Openning log file path:'%s'\n", SET_TESTING_CSV_PATH.c_str()); | |
| // no scope found, defaults to session | |
| logfile_fs.open(SET_TESTING_CSV_PATH.c_str(), std::fstream::in | std::fstream::out); | |
| if (!logfile_fs.is_open() || !logfile_fs.good()) { | |
| fprintf(stderr, "Failed to open '%s' file path=\"%s\" error=%d\n", | |
| basename(SET_TESTING_CSV_PATH.c_str()), SET_TESTING_CSV_PATH.c_str(), errno | |
| ); | |
| return { EXIT_FAILURE, {} }; |
🤖 Prompt for AI Agents
In `@test/tap/tests/test_set_parser_parity.cpp` around lines 302 - 325, The CSV
path construction for SET_TESTING_CSV_PATH is incorrect because concatenating
get_env("TAP_WORKDIR") + "./set_testing-240.csv" can produce
"<dir>./set_testing-240.csv" when TAP_WORKDIR lacks a trailing slash; update the
initialization so it safely joins the workdir and filename (handle empty/null
workdir) using a proper path join strategy (e.g., std::filesystem::path or
explicitly append a '/' when missing) so SET_TESTING_CSV_PATH always becomes
"<workdir>/set_testing-240.csv" and subsequent open() calls in
get_valid_queries() succeed.
- Improved abstractions used for processing parser output.
- Reworked MySQL_Session '*___handle_SET_command' and improved error
reporting.
- Added support for SET multi-statements verification.
+ TODO: Requires more testing.
- Added support to the tokenizer for processing CMD based on the
currently configured 'mysql-version':
+ Added validation for configured 'mysql-version'.
- Introduced handling of 'MySQL_Connection' flags directly via QPO
output.
- Fixed leaks in 'set_testing-240'. - Added 'server_version' support for 'test_mysql_query_digests_stages-t.cpp'. - Added specific TAP test for SQL_MODE validation. - Logging and logic fixes for SET statement tests after recent changes. - Changes for expected tokenizer digests.
Removed unnecessary include.
Test failed to compile due to missing type definition:
```
In file included from /usr/include/c++/15.2.1/memory:80,
from unit_test.h:13,
from unit-strip_schema_from_query-t.cpp:3:
/usr/include/c++/15.2.1/bits/unique_ptr.h: In instantiation of ‘void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = SQLite3_result]’:
/usr/include/c++/15.2.1/bits/unique_ptr.h:398:17: required from ‘std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = SQLite3_result; _Dp = std::default_delete<SQLite3_result>]’
398 | get_deleter()(std::move(__ptr));
| ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
/home/javjarfer/Projects/proxysql/include/MySQL_LDAP_Authentication.hpp:24:68: required from here
24 | virtual std::unique_ptr<SQLite3_result> dump_all_users() { return 0; };
| ^
/usr/include/c++/15.2.1/bits/unique_ptr.h:90:23: error: invalid application of ‘sizeof’ to incomplete type ‘SQLite3_result’
90 | static_assert(sizeof(_Tp)>0,
| ^~~~~~~~~~~
```
648104a to
337d55e
Compare
Search should be restrited to SSL_DIR directory.
Ensure initialization of static objects accross multiple TU.
Simplifies rules complexity for parsing multi-statements and improves the resulting AST.
|
Update pushed on branch
Validation done locally:
This keeps the current behavior where unsupported/unknown |
|
|
Closing this PR as obsolete now that ParserSQL is the active parser path in v3.0.9 and this branch still carries the old Bison/parser implementation. I ported the useful black-box TAP coverage into the current ParserSQL test style here: #5893 The replacement PR intentionally keeps only cases that ParserSQL currently handles correctly through the public adapter. The old cases that depend on unsupported or partially parsed syntax (for example PERSIST/PERSIST_ONLY, SET LOCAL, subquery expressions, bit operators, MEMBER/SOUNDS LIKE, and INTERVAL unit chains) are not asserted there because doing so would lock in incomplete behavior rather than useful regression coverage. |
Description
This PR introduces a new parser that currently targets SET statements. This new parser is thought as a replacement of the previous REGEX based parser. The parser is available through a new value (
3) for config variableset_parser_algorithm.Details
The introduction of this parser opens new possibilities for value validation for SET statements. An example of this would be value validation for
sql_mode, which is already improved in this PR current form. With more accurate validation and error reporting.Testing
Two types of testing are performed, one checking that the current capabilities for variable tracking hasn't change. This is verified via
set_testing-240-t.cppusing the new parsing algorithm. The second kind of testing is evaluating the differences between the two parsers and ensuring that in case these exists, are limitations of the previous REGEX based parser, seetest_set_parser_parity.]Extra changes
CMake 4.0and standard changes inGCC 15. This compilation issues have been addressed in individual commits. No dependency upgrades were required for now.CMakefor connector5.7and are maybe not worth addressing. Using aCMake 3version for compilation might be worth considering for addressing this dependency compilation issues in a simple way.CMakefiles (likeOpenSSLlibrary search). The implementation still poor, and should be improved/simplified.40a3ed4dcfd7320b0d450f3f17485b55969bf2ca. There are a lot of potential improvements for the strategy (like namespace separation for utilities) but I think it's worth keeping something like this in mind with future testing.Extra Reworks - QPO - MySQL Connections - Query Digests
Query Digests / Multiplexing / Query Interception
In 'handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_qpo' the use of query-digest and
raw-query is still mixed. There are still some intercepted queries that use the 'raw-query':
All these queries should be moved to
handler_special_queries, since they do not rely on QPO output ordigests.
PROPOSAL:
All queries that are intercepted in a 'best-effort' manner should be contained in a single point. Their
implementation should be simplified and contained. There is another decision to make regarding this, even if
this best-effort query interception can be not-linked to the 'Query_Processor', we should determine which are
the expected consequences of disabling query digests.
Maybe not everything related to query-interception should be disabled as a consequence of disabling
query-digests. But for instance,
LAST_INSERT_IDqueries are now intercepted, and there are replied withthings like
number_active_transactionsand withlast_insert_idfor the session. But there is an importantmissing thing. The query analysis required that might lead to setting some of these status, like
number_active_transactionsrequiresquery_digeststo be enabled.This complicates the overall picture for query handling, specially for queries which are not isolated, or
depend of internal status set by previous queries that require processing. These queries should, for sure, not
be intercepted in case query-digests are not available. Leaving the interception of queries when query-digest
is disabled to a single class of queries, queries that can aims to retrieve server information, that is
currently contained in the session, and that doesn't requires prior query interception, e.g, queries about
server version, user info, etc...
So the proposal would be, disabling
query-digestsresults in automatic multiplexing disable, and only thepreviously quoted queries are intercepted and replied. Which should be identified and parsed via the following
suggested regexes.
SIMPLE PARSING REGEXES (LazyRE2) / digests & no-digests:
The parsing of these queries should rely in simple regexes that should account for the query itself, spacing,
and ignoring comments, cmds, and hints. RE2 has proven to be extremely fast for this simple cases, specially
when regexes are already constructed (which is moderately expensive). So, all of these regexes should be lazy
regexes (re2::LazyRE2). This parsing should be robust enough to allow simple changes in the query performed
via connectors or users, and prevent silly breaks, while remaining simple.
This is also specially important when taking into account
query_digests_keep_comment. All the previousexact matches may break if this option is changed for digests. This should not be the case.
Intercepted Queries / Comments (Regular,CMD,Hints)
In
handler_special_queriesthere are many queries that are right now intercepted, but their interception isthe most basic form, pure
strncmpmatching with length matching. This supposes an issue when dealing withcomments (or cmd or hints) in queries, which could brake this simple parsing.
PROPOSAL:
All these query matching should be parsed using simple regexes previously suggested. Other examples of
optimizations that suffer from the same issue:
handler_SetAutocommithandler_CommitRollbackIntercepted Queries / Digests / Comments
Method
GPFC_QueryUSEattempts to use both digests orquery-digests, and handles the query differently,using the classic weak match (
strncasecmp), this should be refactored in the regexes for digests or nothingfor digests disabled thanks to the implicit disables multiplex proposed.
All methods inside
ProcessQueryAndSetStatusFlagsuse digests (another reason to disable multiplexingentirely if digests are disabled), and right now all use the weak
strncasecmp. That can be replaced by thepreviously suggested regexes at least.
Once the new parser is able to parse every query, will offer a superior id than regexes. So, the regexes shall
be the fallback, and when enabled, the parser should be the way to identify all these queries.
unable_to_parse_set_statement
This shouldn't work the same now, we should identify the query, and proceed based on the type:
should be enough for them. Digests don't preserve commands by default, we MUST consider this change, see
TODO in
c_tokenizer.cpp.Multiplexing vs Lock-Hostgroup
Disabling multiplexing is a connection based status, meanwhile, 'lock hostgroup' is a session based strategy
for locking the session, preventing it from reaching other hostgroups.
Lock on hostgroup could be thought as a super-set of multiplex-disabled, as it imposes an extra limitation on
top of multiplexing being disabled for the connection. This is the intended design, lock-on-hostgroup should
imply multiplex being disabled, otherwise the point for connection reuse (or failure if trying to reach other
hostgroup) would be lost.
Current behavior is dependent on config and query type, let's break down using query type as classes:
Locking on hostgroup is only performed in the following three cases by the 'QueryProcessor':
NOTE: Should 'qpo->multiplex' be able to 'lift' the restriction of hostgroup locking? Since,'lock_on_hostgroup' is a super-set of 'multiplex', preventing multiplex disabling should also lift lock on
hostgroup. So, value
qpo->multiplex==2should be able to lift the hostgroup locking, since it was designed tospecially prevent locking even if user defined variables were detected. What about
qpo->multiplex==1?We should keep this value separated, this value can force multiplex to be enabled for the query, yet, if the
query processor detects a user-defined variable, will still force a
lock_on_hostgroup, since multiplex willbe disabled via
STATUS_MYSQL_CONNECTION_USER_VARIABLE.This isn't the case right now, since
0also prevents both,lock_on_hostgroupand multiplex disabled (forset_query_lock_on_hostgroup=1). This should be fixed atQPOlevel formultiplex=0.In 'MySQL_Connection', if
set_query_lock_on_hostgroup==0(old behavior):STATUS_MYSQL_CONNECTION_NO_MULTIPLEXdirectly set viamultiplexvalue.STATUS_MYSQL_CONNECTION_USER_VARIABLEcan be prevented viamultiplex==2, preventing multiplex disabled.If
set_query_lock_on_hostgroup==1:STATUS_MYSQL_CONNECTION_NO_MULTIPLEXdirectly set viamultiplexvalue.STATUS_MYSQL_CONNECTION_USER_VARIABLEdependent oflock_on_hostgroup, set iflockwas exercised.NOT SET statements:
STATUS_MYSQL_CONNECTION_NO_MULTIPLEXdirectly set viamultiplexvalue.STATUS_MYSQL_CONNECTION_USER_VARIABLEcan be prevented viamultiplex==2, preventing multiplex disabled.Conclusions - QPO Changes
Using the previous context, it's possible to simplify the flow in the following way. No matter the value of
set_query_lock_on_hostgroup:STATUS_MYSQL_CONNECTION_NO_MULTIPLEXdirectly set viamultiplexvalue:0-> disabled,1or2-> enabled.STATUS_MYSQL_CONNECTION_USER_VARIABLEdependent onmultiplex == 2. Iftrue, nolock_on_hostgroupshould take place, as multiplex is
enabledand this variable should never be set. With the exception ofSETstatement parsing errors or (currently)SETmulti-statements, not-understanding the query atQPOlevel should always result in a lock via
STATUS_MYSQL_CONNECTION_USER_VARIABLE.This should all be done at
QPOlevel, sincedigestsshould not be analyzed inMySQL_Connection,specially after the new
SQL parseris in place.Reworks Example - IsKeepMultiplexEnabledVariables
As other logic present in
MySQL_Connectionfor flags processingIsKeepMultiplexEnabledVariablesis veryweak. The logic doesn't perform any syntax related check, and could be easily fooled/broken:
/* foo */ SELECT @bar: As previously mentionedquery_digests_keep_commentenabled would break thisSELECTbased query detection.SELECTwhich contains akeep_multiplexing_variable, just by text match, can hide variables between the variable appearance andthe next
,. E.g:SELECT @@version INTO @foo.E.g:
SELECT @@version, (SELECT 1)(here(SELECTwould be detected as a variable).If the old behavior for
set_query_lock_on_hostgroupdoesn't meanbackwards compatible, but it's analternative option that shall be maintained, the implementation should also rely on the
QPOoutput. Thismeans updating the impl for
IsKeepMultiplexEnabledVariables, which shall rely on the parser, or in case ofthe parser being disabled an alternative impl using regexes for simple variable detection.
PROPOSAL:
When parser isn't available, a simpler, regex based approach can achieve far more robust results:
specified by [MySQL documentation][https://dev.mysql.com/doc/refman/8.4/en/user-variables.html] (
@alphanumeric characters,
.,_, and$finished with an space). This should be a trivial regex, sincethanks to digests we are not dealing with values (
?). System variables should be matched using the sameprinciple, since their matching mark is identical to
user definedones, but with@@.Summary by CodeRabbit
New Features
Refactor