Skip to content
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

support COM_RESET_CONNECTION to reset session #1273

Closed
liubog2008 opened this issue Oct 15, 2021 · 14 comments
Closed

support COM_RESET_CONNECTION to reset session #1273

liubog2008 opened this issue Oct 15, 2021 · 14 comments

Comments

@liubog2008
Copy link

liubog2008 commented Oct 15, 2021

Issue description

Now ResetSession only mark a flag. It's more reasonable to send COM_RESET_CONNECTION to really reset session.

https://dev.mysql.com/doc/internals/en/com-reset-connection.html

Example code

If possible, please enter some example code here to reproduce the issue.

Error log

If you have an error log, please paste it here.

Configuration

Driver version (or git SHA):

Go version: run go version in your console

Server version: E.g. MySQL 5.6, MariaDB 10.0.20

Server OS: E.g. Debian 8.1 (Jessie), Windows 10

@methane
Copy link
Member

methane commented Oct 15, 2021

It will kill performance. Why do you need it?

@liubog2008
Copy link
Author

Emmm, that's a big story.

I hope to gracefully shutdown a database(not only for mysql but for some mysql protocol compatible server, such as TiDB), but I find it that only client side knows when to close TCP connections.

Now

1. server is shutting down.
2. server closes all connections that are idle.
3. client gets `reset` error when sending commands to server in these TCP connections. (Maybe ok for functions of DB.*, but failed for Conn.*)
4. client throws error to frontend because it can't be handled.

I hope

1. server is shutting down.
2. server is waiting for all connections being closed.
3. client sends COM_RESET_CONNECTION after all actions are done.
3. server returns error for COM_RESET_CONNECTION to notify client to change a TCP connection.
4. client discards the TCP connection between the server which is shutting down.
5. server is ready to close.

While server is marked as shutting down, response of ResetSession can be used to notify client to change a new TCP connection to avoid reset error.

The performance of one instance is not an important problem(can be resolved by scaling). And this feature can be optional for performance sensitive scenario.

And some mysql pools such as https://github.com/mysql/mysql-connector-python has implemented this feature.(a question about this feature: https://dba.stackexchange.com/questions/290727/when-and-why-should-i-reset-session-in-a-connection-pool)

Sorry for my poor English, I'm not sure the reason is clearly explained.

@methane
Copy link
Member

methane commented Oct 18, 2021

You should try db.SetConnMaxLifetime() with short timeout. That is enough for graceful shutdown.

@liubog2008
Copy link
Author

liubog2008 commented Oct 18, 2021

If MaxLifetime is long, server should wait too long time, and if MaxLifetime is short, TCP connection may be swapped too frequent and the average of latency will increase.

@methane
Copy link
Member

methane commented Oct 18, 2021

if MaxLifetime is short, TCP connection may be swapped too frequent and the average of latency will increase.

Have you really tried it? 3sec? 5sec? how much overhead?

MySQL connection is relatively cheap. I think it is much cheaper than sending COM_RESET_CONNECTION for every query.

@liubog2008
Copy link
Author

I think it is much cheaper than sending COM_RESET_CONNECTION for every query

Not for every query, but when putting conn back to pool. And I think COM_RESET_CONNECTION is more lightweight than open a new MySQL connection.

COM_RESET_CONNECTION < COM_CHANGE_USER < reconnect

COM_RESET_CONNECTION: only reset session state
COM_CHANGE_USER: re-authentication but in the same TCP connection
reconnect: re-handshake for TCP connection

@liubog2008
Copy link
Author

liubog2008 commented Oct 18, 2021

Have you really tried it? 3sec? 5sec? how much overhead?

Connection pool is designed to avoid the handshake time of mysql connection. If MaxLifetime is too short, connections will be closed too frequent and pool will have no effect(Just like closing connection immediately).

@methane
Copy link
Member

methane commented Oct 18, 2021

Not for every query, but when putting conn back to pool.

db.Exec() and db.Query() is well-used pattern for Go. In this case, every query putting conn back to pool.

And I think COM_RESET_CONNECTION is more lightweight than open a new MySQL connection.

Of course, but it is much more frequent.

@methane
Copy link
Member

methane commented Oct 18, 2021

Have you really tried it? 3sec? 5sec? how much overhead?

Connection pool is designed to avoid the handshake time of mysql connection. If MaxLifetime is too short, connections will be closed too frequent and pool will have no effect(Just like closing connection immediately).

If your app sends 500query/sec, 1500 queries can be sent in 3sec lifetime.
It is very far from "pool will have no effect(Just like closing connection immediately)." Reconnection overhead become 1/1500.

@methane
Copy link
Member

methane commented Oct 18, 2021

And if it is unacceptable for you, you can use both of ConnMaxIdleTime and ConnMaxLifetime.
For example, Lifetime can be 3min and IdleTime can be 5sec.

@liubog2008
Copy link
Author

If your app sends 500query/sec, 1500 queries can be sent in 3sec lifetime.
It is very far from "pool will have no effect(Just like closing connection immediately)." Reconnection overhead become 1/1500.

Good point, I never really assume such short of lifetime(normally maxlifetime is X mins). And shutdown time above 30s is hardly acceptable(default in kubernetes is 30s).

I'll reconsider it and may test it by the real world data.

@liubog2008
Copy link
Author

I change my mind, an arbitrary lifetime is acceptable and enough. What I really need is changing the timeout of graceful shutdown.

@Hansanshi
Copy link

It will kill performance. Why do you need it?

@methane

Provide a hook or a option for user to send COM_RESET_CONNECTION when reset session?

COM_RESET_CONNECTION can avoid session variable leaked. It do may be needed in some scenes.

Someone need while many don't need, let user decide it.

@shogo82148
Copy link
Contributor

I have seen code that assumes session variables will leak. It just happens to work, but it is incorrect. I think it would be useful to avoid such code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants