-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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) #23267
*: add infoschema client errors (#22382) #23267
Conversation
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
/run-all-tests |
@morgo please accept the invitation then you can push to the cherry-pick pull requests. |
Hold this PR since we do bugfix backports to 4.0 only. |
This will require a parser change as well to support |
This is a critical issue since it helps expose incorrect usage of client multi-statement. But it's unclear to me how we are expected to proceed. I've unassigned myself and assigned @bb7133 and @alex-quan-001 |
/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: 866315b
|
cherry-pick #22382 to release-4.0
You can switch your code base to this Pull Request by using git-extras:
# In tidb repo: git pr https://github.com/pingcap/tidb/pull/23267
After apply modifications, you can push your change to this PR via:
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