perf: optimize ParserSQL digest and classification overhead - #6137
perf: optimize ParserSQL digest and classification overhead#6137Snehil-Shah wants to merge 1 commit into
Conversation
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (2)Header include guards use the `#ifndef __CLASS_*_H` convention.📄 CodeRabbit inference engine (CLAUDE.md) Files:
Class names must use `PascalCase` with protocol prefixes such as `MySQL_`, `PgSQL_`, and `ProxySQL_`.📄 CodeRabbit inference engine (CLAUDE.md) Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughParserSQL now records statement types during digest parsing, exposes protocol-specific conversion functions, and reuses stored classifications in MySQL and PostgreSQL command detection. Unclassified statements continue through text-based parsing. ChangesParserSQL command classification
Merge Risk: ⚪ Minimal · up to This change reuses ParserSQL results and reduces digest allocations while preserving normal MySQL and PostgreSQL classification behavior. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/Query_Processor_ParserSQL.cpp">
<violation number="1" location="lib/Query_Processor_ParserSQL.cpp:445">
P1: When callers use either ParserSQL digest adapter directly, short normalized output now makes `digest_text` point to inline storage, but existing callers free that field and abort. Keep `digest_text` heap-allocated for this public adapter contract, or update the API and every caller to use the ownership-aware cleanup path in both MySQL and PostgreSQL branches.</violation>
</file>
<file name="include/proxysql_structs.h">
<violation number="1" location="include/proxysql_structs.h:902">
P2: When a zero-initialized `SQP_par_t` reaches command lookup with ParserSQL enabled, `parsersql_stmt_type` is `0`, so the lookup treats it as a cached type and never parses `digest_text`. Initialize this field to `-1` for every manually constructed parser state, or make the lookup distinguish an absent cache from a valid type.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (normalized_len < QUERY_DIGEST_BUF) { | ||
| memcpy(qp->buf, normalized, normalized_len); | ||
| qp->buf[normalized_len] = '\0'; | ||
| qp->digest_text = qp->buf; |
There was a problem hiding this comment.
P1: When callers use either ParserSQL digest adapter directly, short normalized output now makes digest_text point to inline storage, but existing callers free that field and abort. Keep digest_text heap-allocated for this public adapter contract, or update the API and every caller to use the ownership-aware cleanup path in both MySQL and PostgreSQL branches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/Query_Processor_ParserSQL.cpp, line 445:
<comment>When callers use either ParserSQL digest adapter directly, short normalized output now makes `digest_text` point to inline storage, but existing callers free that field and abort. Keep `digest_text` heap-allocated for this public adapter contract, or update the API and every caller to use the ownership-aware cleanup path in both MySQL and PostgreSQL branches.</comment>
<file context>
@@ -413,26 +413,39 @@ void parsersql_digest_init_mysql(SQP_par_t* qp, const char* query, int query_len
+ if (normalized_len < QUERY_DIGEST_BUF) {
+ memcpy(qp->buf, normalized, normalized_len);
+ qp->buf[normalized_len] = '\0';
+ qp->digest_text = qp->buf;
+ } else {
+ qp->digest_text = strndup(normalized, normalized_len);
</file context>
| char *digest_text; | ||
| char *first_comment; | ||
| char *query_prefix; | ||
| int parsersql_stmt_type; |
There was a problem hiding this comment.
P2: When a zero-initialized SQP_par_t reaches command lookup with ParserSQL enabled, parsersql_stmt_type is 0, so the lookup treats it as a cached type and never parses digest_text. Initialize this field to -1 for every manually constructed parser state, or make the lookup distinguish an absent cache from a valid type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At include/proxysql_structs.h, line 902:
<comment>When a zero-initialized `SQP_par_t` reaches command lookup with ParserSQL enabled, `parsersql_stmt_type` is `0`, so the lookup treats it as a cached type and never parses `digest_text`. Initialize this field to `-1` for every manually constructed parser state, or make the lookup distinguish an absent cache from a valid type.</comment>
<file context>
@@ -899,6 +899,7 @@ struct __SQP_query_parser_t {
char *digest_text;
char *first_comment;
char *query_prefix;
+ int parsersql_stmt_type;
};
</file context>
|
1 participant
This implements two performance improvements in the digest computation and command-type classification path with the ParserSQL parser:
std::stringinstance, and by reusing the struct's existing buffer for the digest text (instead of reallocating).NOTE: This does NOT affect the legacy parser path in any way.
Measurements
All numbers in the below tables are in ns. They are calculated by arming
clock_gettime(CLOCK_THREAD_CPUTIME_ID, ...)around the code under bench.The numbers are recorded for the following four configurations:
Machine specs: Ubuntu 24.04.4 (colima), aarch64 (ARM64), 4 vCPUs, 8 GiB RAM using jemalloc allocator correctly.
Function-level benchmark
Benchmarked the following two functions in the hot-path:
Before:
After:
taking median of 35 iterations with 200,000 calls per iteration (both funcs serially) on one single core.
Full proxy synchronous path
CPU time from query packet received to just before the query is handed to the backend, excluding all network/backend IO.
Setup: Running a full ProxySQL server with single MySQL and a single PgSQL worker thread (on a real MySQL backend), with timers attached in code (just calculating the proxy synchronous path), and ran sysbench for 20 seconds and saved all measurements. Took the median of 10 such 20-second iterations.
Summary by CodeRabbit