vtgate: ignore MySQL client probe query select $$ in querylog#20073
vtgate: ignore MySQL client probe query select $$ in querylog#20073ChaitanyaD48 wants to merge 2 commits into
select $$ in querylog#20073Conversation
…elect 31239 Signed-off-by: ChaitanyaD48 <chaitanya.d48@gmail.com>
…eamlog Signed-off-by: ChaitanyaD48 <chaitanya.d48@gmail.com>
vitess-bot
Bot
added
NeedsWebsiteDocsUpdate
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
There was a problem hiding this comment.
Pull request overview
Filters out the MySQL 8.4+ client “probe” query (select $$) from Vitess query logging to avoid unavoidable parse-error noise on new client connections.
Changes:
- Suppress query-log emission when the SQL is exactly
select $$(case-insensitive, surrounding whitespace ignored). - Add unit tests covering the suppression behavior and ensuring similar queries still log normally.
- Minor formatting-only adjustments in
streamlogcounter declarations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| go/streamlog/streamlog.go | Add early suppression in QueryLogConfig.ShouldEmitLog for the select $$ probe query. |
| go/streamlog/streamlog_test.go | Add test cases verifying select $$ is filtered while non-probe variants still emit. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20073 +/- ##
===========================================
+ Coverage 69.67% 83.96% +14.29%
===========================================
Files 1614 2 -1612
Lines 216793 131 -216662
===========================================
- Hits 151044 110 -150934
+ Misses 65749 21 -65728
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // MySQL 8.4+ clients send "select $$" at connection time to probe dollar-quote | ||
| // syntax support. This is hardcoded client behavior that cannot be disabled; | ||
| // suppress it to avoid log noise. | ||
| if strings.EqualFold(strings.TrimSpace(sql), "select $$") { | ||
| return false, "" | ||
| } |
There was a problem hiding this comment.
IMO... clients can send whatever queries they want. It's not up to the Vitess developers to decide what should and should not be logged. It's a log of queries coming from clients.
And IMO every Vitess user should not pay the cost of an EqualFold on every query just to elide it. The use case in the issue was:
For integration load testing I have scripts that spin up a lot of parallel connections to the database, and every time a mysql client initiates a new connection to the server I get a log message in the vtgate log about the invalid query. As it's a hard-coded query that is meaningless (as far as I can tell) and completely out of my control, that is just unnecessary noise in the query log.
IMO we should not modify how Vitess behaves for all users simply so that it fits better into that very narrow use case / scenario, especially when there seems to be a pretty clear performance cost.
Am I missing or misunderstanding something?
There was a problem hiding this comment.
What might make sense though... is providing a Viper config option that allowed users to specify a set of SQL query shapes to ignore in the logs. What do you think about that? Then a user could specify this specific query and any others coming from their clients that they want to elide from the vtgate logs.
There was a problem hiding this comment.
@mattlord Thanks for the review! Its a Good Idea to have a user configurable viper config option to ignore a set of SQL Query shapes in Logs.
I have some questions around this -
Question 1: How should we "match" a query?
Should we match by normalized shape or exact string? In case of Reading "query shape" as the output of sqlparser.RedactSQLQuery, select 31239 and select 99999 would both collapse to a single entry like select :vtg1 but, select $$ itself doesn't parse, so RedactSQLQuery fails on it. We'd need a fallback for the un-parseable cases. The options I see:
- Shape-match when parseable, raw-string fallback otherwise.
- Just raw-string match (case/whitespace-insensitive), skipping normalization entirely.
Which approach do you think makes more sense?
And Addressing your performance Concern. I feel this new user-configurable viper config option must sit at the executor level in vtgate, not inside streamlog's ShouldEmitLog. Planning to go with a len() == 0 fast path something like -
func shouldIgnoreInQueryLog(sql string) bool {
if len(ignoredQueries) == 0 {
return false // ← fast path: users who don't opt-in land here
}
// expensive part: match sql string, do map lookup, etc.
normalized := strings.ToLower(strings.TrimSpace(sql))
_, found := ignoredQueriesMap[normalized]
return found
}for users who don't opt in pay essentially nothing (one int compare), and users who do pay a single map lookup per query. That should keep the cost off everyone except the operators who actually asked for it.
What do you think?
There was a problem hiding this comment.
I think we should do it by shape or the normalized form where the variable parts are replaced by a placeholder.
For the performance part, that sounds good. Thanks! ❤️
Description
MySQL 8.4+ clients (
mysqlCLI, Percona fork) unconditionally sendselect $$immediately after the handshake to probe whether the server supports$$as a quoting style. This is hardcoded in the client ([upstream commit (https://github.com/mysql/mysql-server/commit/9c7bc2973110861b731311f8b42af152f62d65af)) and cannot be disabled or suppressed by the user.Vitess's SQL parser does not recognize
$$as valid syntax, so every new connection from a MySQL 8.4+ client produces a parse-error entry in vtgate's query log.Before
After
Related Issue(s)
Fixes #19610
Checklist