-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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.
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
*: add infoschema client errors #22382
Conversation
76699ae
to
0a1f635
Compare
Co-authored-by: djshow832 <zhangming@pingcap.com>
Co-authored-by: djshow832 <zhangming@pingcap.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/lgtm |
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by writing |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: acf1f51
|
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
cherry pick to release-4.0 in PR #23267 |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
cherry pick to release-5.0-rc in PR #23268 |
/run-build-image |
Do you have related documentation? @morgo |
@SunRunAway I do now: pingcap/docs#5062 - can you please help review? :-) |
What problem does this PR solve?
Issue Number: close #14433
Revives PR: #20785
Problem Summary:
In the PR #22351 , it is proposed that multiStmt be permitted as a warning, and later changed to a default. This provides an upgrade path for users.
The problem is, errors sent to the client were previously not captured by the server. So it is difficult to tell if a user is depending on the buggy behavior of multiStmt, and if the defaults change will cause them problems.
Thus, the proposal is to cherry-pick to 4.0 and 5.0 to provide a viable way to get past the multiStmt vulnerability.
What is changed and how it works?
What's Changed:
This PR introduces a method to observe errors and warnings sent to clients. For example, using multiStmt as an example:
(The warning is the default, when set to
OFF
, it generated an error).In total, three new information schema tables have been introduced:
The design is modeled loosely based on what MySQL 8.0 has in performance_schema. But there are the following differences to be aware of:
TRUNCATE TABLE
command. But since these are in infoschema in TiDB, a commandFLUSH CLIENT_ERRORS_SUMMARY
is added.errno
package - some are known not to be generated. Also, it creates a very large table if there are a lot of users or hosts.ERROR_MESSAGE (in sprintf format), not the
ERROR_NAME. This is a current limitation based on what is stored in the
errno` package. I think it's fine.ERROR_RAISED
/ERROR_HANDLED
counts, and the columns are just renamed toERROR_COUNT
. TiDB does not have stored procs, and thus no error handling.How it Works:
The errors and warnings could be generated anywhere in code. I capture them not at generate time, but as they are sent to the client. This means that internal sql execution that triggers warnings etc. is not logged.
There is no persistence of the statistics, and no cluster-wide view.
Related changes
Check List
Tests
Side effects
Release note