server: max_connections admission is coupled with session creation
- Dominant language
- Go
- Stars
- 40.5k
- Forks
- 6.2k
- PR merge metrics
- PR metrics pending
Description
## Bug Report
### 1. Minimal reproduce step (Required)
There are two related problems because global connection admission is checked from `clientConn.openSession()`, although a session can be created multiple times during one physical connection lifecycle.
#### Scenario A: `COM_CHANGE_USER` unnecessarily checks the global connection limit
1. Configure `max_connections` to a non-zero value `N`.
2. Establish `N` client connections.
3. On one of the existing connections, send a valid `COM_CHANGE_USER` command.
The command follows this path:
```text
handleChangeUser
-> openSession
-> checkConnectionCount
-> ConnectionCount
-> Server.rwlock.RLock
```
`COM_CHANGE_USER` reuses the same physical connection and connection ID. It does not add an entry to `Server.clients`, but it still checks `max_connections` and accesses the global connection-map lock.
#### Scenario B: concurrent connection admission can exceed `max_connections`
1. Configure a small non-zero `max_connections` value.
2. Start more than that number of connection handshakes concurrently.
3. Synchronize the attempts so that they call `checkConnectionCount()` before any of them calls `registerConn()`.
The limit check and registration use separate critical sections:
```text
openSession:
RLock
check len(Server.clients)
RUnlock
after authentication:
Lock
Server.clients[connectionID] = conn
Unlock
```
Multiple connection attempts can therefore observe the same count, pass the check, and register later.
### 2. What did you expect to see? (Required)
- Global connection admission should happen once per physical connection lifecycle.
- Checking and reserving capacity should be atomic, so successfully admitted connections cannot exceed a non-zero `max_connections` value.
- `COM_CHANGE_USER` should not consume another global connection slot or access the global connection-map lock. A successful user change should keep the same connection entry. A failed user change should restore the previous authenticated session and also keep the same connection entry.
- A connection slot should be released exactly once when the physical connection is closed or its initial handshake fails.
### 3. What did you see instead (Required)
- `openSession()` checks `max_connections` every time it creates a TiDB session, including during `COM_CHANGE_USER`.
- When the current connection count has reached the limit, a valid `COM_CHANGE_USER` on an existing connection may be rejected even though it does not create another physical connection.
- Under a proxy workload with frequent `COM_CHANGE_USER`, every command calls `checkConnectionCount()` and acquires the read side of the global connection-map `RWMutex`. Concurrent `registerConn()` and `clientConn.Close()` calls require its write side, which can cause severe reader/writer contention.
- The check and registration are not atomic, so concurrent initial handshakes can pass the check and subsequently register more connections than the configured limit.
### 4. What is your TiDB version? (Required)
The code path exists on the current master branch and has also been confirmed in TiDB v8.5.3.
### Additional context
The session creation and physical connection admission responsibilities should be separated. One possible direction is to reserve a connection slot atomically during initial connection admission, convert that reservation into a registered connection after successful authentication, and release it exactly once on failure or close. Recreating a session for `COM_CHANGE_USER` should reuse the existing reservation.
This also allows connection-count enforcement to be decoupled from `Server.clients`, reducing pressure on the global connection-map `RWMutex` without weakening the connection limit.
Contributor guide
Research direction
Start by tracing handleChangeUser through clientConn.openSession() and checkConnectionCount(), then compare the admission path with registerConn() and clientConn.Close(). Verify how Server.clients and the connection-count lock are used during initial handshakes, authentication failure, COM_CHANGE_USER, and close. Done means admission is atomic, session recreation does not recheck capacity, and each reserved slot is released exactly once.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, mysql
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100