-
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
server, sessionctx: improved mysql compatibility with support for init_connect #23713
Conversation
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.
Rest LGTM
server/conn.go
Outdated
@@ -766,6 +826,14 @@ func (cc *clientConn) Run(ctx context.Context) { | |||
cc.pkt.setReadTimeout(time.Duration(waitTimeout) * time.Second) | |||
start := time.Now() | |||
data, err := cc.readPacket() | |||
if initConnectFailed { |
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.
Why the error is not returned immediately? Because of some dependencies on cc
variable?
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.
I am trying to match the semantic of when MySQL disconnects the client - but actually I just checked and it's not quite correct. See TiDB:
mysql> select 1;
ERROR 2013 (HY000): Lost connection to MySQL server during query
vs MySQL:
mysql [localhost:8023] {u1} ((none)) > select 1;
ERROR 2006 (HY000): MySQL server has gone away
No connection. Trying to reconnect...
Connection id: 48
Current database: *** NONE ***
ERROR 1184 (08S01): Aborted connection 48 to db: 'unconnected' user: 'u1' host: 'localhost' (init_connect command failed)
I might have misunderstood it, and it might occur in the initial handshake.
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.
I have changed it to the handshake stage. It is not quite identical to MySQL because MySQL seems to allow the client to initially connect based on capabilities, and I'm not sure we have all the logic to do that yet. So now it is:
$ mysql -e "create user u1;set global init_connect='fail';"; mysql -uu1
ERROR 1184 (08S01): Aborted connection 5 to db: 'unconnected' user: 'u1' host: '127.0.0.1' (init_connect command failed)
/lgtm |
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
[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: dbc8ce0
|
/run-unit-test |
/run-unit-test |
/run-cherry-picker |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
cherry pick to release-4.0 in PR #26031 |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
cherry pick to release-5.0 in PR #26072 |
@lzmhhh123 be careful cherry picking to 4.0 but not the other branches in between (in this case 5.0). It leads to a bad user experience because the feature appears and then dissapears. |
What problem does this PR solve?
Issue Number: close #18894
Problem Summary:
MySQL supports the ability to execute a set of statements on first connection. This PR adds the same feature to TiDB.
It can be useful for some applications, since SQL modes can be set, user variables, etc. It is also used extensively in the mysql-test testsuite.
What is changed and how it works?
Proposal: xxx
What's Changed:
How it Works:
Related changes
pingcap/docs
/pingcap/docs-cn
:Check List
Tests
Side effects
Release note
init_connect
and associated functionality.