Add SHOW WARNINGS support in MySQL interface
#58929
Conversation
slvrtrn
added
pr-improvement
|
This is an automated comment for commit 8658cd3 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
| ${MYSQL_CLIENT} --execute "show warnings;" | ||
| ${MYSQL_CLIENT} --execute "SHOW WARNINGS LIMIT 100;" | ||
| ${MYSQL_CLIENT} --execute "show warnings limit 100;" | ||
| ${MYSQL_CLIENT} --execute "SHOW WARNINGS LIMIT 100 OFFSET 100;" |
There was a problem hiding this comment.
Looking at MySQL docs:
SHOW WARNINGS [LIMIT [offset,] row_count]
SHOW WARNINGS LIMIT 100, 50 (MySQL) is equivalent to SHOW WARNINGS LIMIT 100 OFFSET 50 (ClickHouse). Did you ever see syntax [offset,] row_count in logs?
There was a problem hiding this comment.
I only saw plain SHOW WARNINGS so far.
| ${MYSQL_CLIENT} --execute "show warnings limit 100 offset 100;" | ||
| ${MYSQL_CLIENT} --execute "SHOW COUNT(*) WARNINGS;" | ||
| ${MYSQL_CLIENT} --execute "show count(*) warnings;" | ||
| ${MYSQL_CLIENT} --execute "SELECT @@session.warning_count;" |
There was a problem hiding this comment.
@@session.warnings_count is a system variable in MySQL. My feeling is that re-implementing (even just faking) these in ClickHouse opens a can of worms.
If you did not see SELECT @@session.warning_count; in log files, I'd prefer to not provide compatibility with @@session.warning_count.
There was a problem hiding this comment.
It already works even without this PR. I only added it to the test cause it's mentioned in the docs.
| @@ -544,6 +548,18 @@ static bool isFederatedServerSetupSetCommand(const String & query) | |||
| return re2::RE2::FullMatch(query, expr); | |||
| } | |||
|
|
|||
| /// Always return an empty set with appropriate column definitions for SHOW WARNINGS queries | |||
There was a problem hiding this comment.
Thoughts:
- as the implementation is in MySQLHandler.cpp, I assume the statements work only through the MySQL port?
- similarity, I was thinking whether the statements should be documented in docs/en/sql-reference/statements/show.md but maybe that's not necessary when exist only for the MySQL protocol
- we are doing simple string comparisons (l. 91/92) to handle the statements. While matching is case-insensitive (good), statements with more than one whitespace (
SHOW WARNINGS) are to the user's surprise not recognized but I guess this isn't terrible for our use case. Maybe you like to add a comment about that (l. 90 maybe?)
There was a problem hiding this comment.
p.1 - yes, it works only via MySQL port.
p.2 - docs are probably not necessary, indeed.
p.3 is precisely why I am not very happy with the current implementation; it is fine for automatically generated statements (via tools), but it falls apart with any typos, like extra spaces. I'd say when we accumulate all the current fixes and related tests (including the rolled back #58835) AND if we have issues with queries matching (extra spaces, etc), we can think about a more sophisticated approach.
The extra spaces issue, in particular, is true for any query that was historically hardcoded into the MySQLHandler (I just added new ones in a similar fashion).
SHOW WARNINGSis a rather popular query that is sent by a lot of different tools (DBeaver, DataGrip...) and, while it is almost always non-fatal if it fails, it produces unnecessary errors in the log.This PR adds minimal support for it, so the statement does not fail. As discussed with @rschu1ze, we add it for compatibility reasons and with correct column definitions, though we always return an empty set.
See also: https://dev.mysql.com/doc/refman/8.0/en/show-warnings.html
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
MySQL interface gained compatibility with
SHOW WARNINGS/SHOW COUNT(*) WARNINGSqueries, though the returned result is always an empty set.