executor: introduce max_user_connections (#59197) | tidb-test=pr/2720#67337
Conversation
ti-chi-bot
Bot
added
release-note
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-user connection limits: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server (auth flow)
participant Privilege as Privilege Manager
participant DB as mysql.user (storage)
Client->>Server: Connect (login)
Server->>Server: determine user@host
Server->>Privilege: GetUserResources(user, host)
Privilege->>DB: SELECT Max_user_connections FROM mysql.user
DB-->>Privilege: Max_user_connections
Privilege-->>Server: user_limit
Server->>Server: load globalLimit (vardef) and calculateConnectionLimit
Server->>Server: check activeConnections vs effectiveLimit
alt Limit exceeded
Server-->>Client: ErrTooManyUserConnections (server:1203)
else Allowed
Server->>Server: increaseUserConnectionsCount()
Server-->>Client: Accept connection
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
pkg/parser/parser.go (1)
23291-23293: Consider clarifying the warning message grammar.The warning text is grammatically awkward—using "but" to mean "except" is non-idiomatic, and "they" has an ambiguous antecedent. It could be misread as implying
MAX_USER_CONNECTIONSis also ignored.✏️ Suggested wording
if needWarning { - yylex.AppendError(yylex.Errorf("TiDB does not support WITH ConnectionOptions but MAX_USER_CONNECTIONS now, they would be parsed but ignored.")) + yylex.AppendError(yylex.Errorf("TiDB only supports MAX_USER_CONNECTIONS in WITH ConnectionOptions; other options will be parsed but ignored.")) parser.lastErrorAsWarn() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/parser.go` around lines 23291 - 23293, The warning text emitted via yylex.AppendError(yylex.Errorf(...)) when needWarning is true is grammatically unclear and ambiguous; update the message passed to yylex.Errorf to use clear wording (for example: "TiDB does not support WITH ConnectionOptions; ConnectionOptions will be parsed but ignored. MAX_USER_CONNECTIONS is unaffected.") so it clearly states that only ConnectionOptions are ignored. Modify the string in the call that currently sits alongside parser.lastErrorAsWarn() and keep the same error/warning flow (yylex.AppendError and parser.lastErrorAsWarn) and the needWarning conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/executor/simple.go`:
- Around line 829-830: The per-user MAX_USER_CONNECTIONS assignment in the
ast.MaxUserConnections case silently truncates values to math.MaxInt16; change
the clamp to use the same upper bound as the system variable/column contract
(the sysvar max_user_connections limit used elsewhere) when setting
info.maxUserConnections so values like 50000 are preserved (e.g., use the sysvar
constant or central limit value instead of math.MaxInt16), and add a
unit/integration test that CREATE/ALTER USER ... WITH MAX_USER_CONNECTIONS 50000
persists and is enforced at that higher limit.
In `@pkg/executor/test/simpletest/simple_test.go`:
- Around line 254-255: The test currently queries the entire mysql.user table
which is non-deterministic; change the tk.MustQuery call to only select the two
accounts under test and force a stable order — e.g. use "select user,
max_user_connections from mysql.user where user in ('root','test') order by
user" — then keep the Check assertion as testkit.Rows("root 0", "test 0") so the
comparison is deterministic regardless of other users or scan order.
In `@pkg/parser/parser.y`:
- Around line 13825-13827: The warning message emitted via
yylex.AppendError(yylex.Errorf(...)) and demoted with parser.lastErrorAsWarn()
is misleading because it says all ConnectionOptions "would be parsed but
ignored" even when MAX_USER_CONNECTIONS is supported; update the warning text to
only state that unsupported ConnectionOptions will be parsed but ignored (or
explicitly mention that MAX_USER_CONNECTIONS is supported) so the message scopes
to unsupported options only and does not imply MAX_USER_CONNECTIONS is ignored.
In `@pkg/server/conn.go`:
- Around line 811-814: The probe call cc.checkUserConnectionCount(host) in
openSessionAndDoAuth should be removed and replaced by performing the actual
locked reservation of a user connection (the same increment/lock used in
user_connections.go to claim a slot) during the initial-connect auth path—do the
reservation before sending the successful handshake/OK and release it on auth
failure; explicitly skip performing this locked reservation when the request is
a COM_CHANGE_USER (i.e., when openSessionAndDoAuth is invoked from
handleChangeUser), so connection ownership isn't transferred on change-user
while still enforcing limits on fresh handshakes.
In `@pkg/server/err/error.go`:
- Around line 35-36: Fix the exported comment for ErrTooManyUserConnections by
correcting the misspelled word “mang” to “many” so the comment reads
“ErrTooManyUserConnections is returned when too many user connections are
established.” Update the comment directly above the ErrTooManyUserConnections
declaration to ensure API docs and code navigation show the corrected text.
In `@pkg/server/server.go`:
- Around line 726-733: The current increment/decrement pair is racy for sessions
that call COM_CHANGE_USER because increaseUserConnectionsCount() uses the
current session user but decreaseUserConnectionCount() reads
ctx.GetSessionVars().User at close, possibly a different user; fix by capturing
the identity that was actually counted when the increment succeeds and using
that exact identity on close: modify conn.increaseUserConnectionsCount() (or its
caller around conn.increaseUserConnectionsCount()) to store the counted
username/identifier in a new conn field (e.g., conn.countedUser or
conn.countedUserKey) and update conn.decreaseUserConnectionCount() to use that
stored field instead of reading ctx.GetSessionVars().User; ensure the stored
field is set only on successful increment and cleared after decrement to avoid
double-decrement.
In `@pkg/server/user_connections.go`:
- Around line 37-41: increaseUserConnectionsCount currently computes and
increments a user key but decreaseUserConnectionCount recomputes the key from
SessionVars().User at close time (which breaks when COM_CHANGE_USER occurs);
modify clientConn to store the actual counted identity/key on the connection
object when increaseUserConnectionsCount() succeeds (e.g., a new field like
countedUserKey or countedIdentity on clientConn) and have
decreaseUserConnectionCount() always read and decrement that stored key (and
clear it after decrement) instead of rebuilding from SessionVars().User; update
both increaseUserConnectionsCount and decreaseUserConnectionCount to use this
stored key so the same identity that was incremented is always decremented.
---
Nitpick comments:
In `@pkg/parser/parser.go`:
- Around line 23291-23293: The warning text emitted via
yylex.AppendError(yylex.Errorf(...)) when needWarning is true is grammatically
unclear and ambiguous; update the message passed to yylex.Errorf to use clear
wording (for example: "TiDB does not support WITH ConnectionOptions;
ConnectionOptions will be parsed but ignored. MAX_USER_CONNECTIONS is
unaffected.") so it clearly states that only ConnectionOptions are ignored.
Modify the string in the call that currently sits alongside
parser.lastErrorAsWarn() and keep the same error/warning flow (yylex.AppendError
and parser.lastErrorAsWarn) and the needWarning conditional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c820046d-aeb5-4194-ae42-0a80b4550214
📒 Files selected for processing (22)
br/pkg/restore/snap_client/systable_restore_test.goerrors.tomlpkg/errno/errname.gopkg/executor/show.gopkg/executor/simple.gopkg/executor/test/simpletest/simple_test.gopkg/parser/parser.gopkg/parser/parser.ypkg/privilege/privilege.gopkg/privilege/privileges/cache.gopkg/privilege/privileges/privileges.gopkg/server/BUILD.bazelpkg/server/conn.gopkg/server/err/error.gopkg/server/server.gopkg/server/user_connections.gopkg/server/user_connections_test.gopkg/session/bootstrap.gopkg/session/bootstrap_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/noop.gopkg/sessionctx/variable/sysvar.go
💤 Files with no reviewable changes (1)
- pkg/sessionctx/variable/noop.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5-20260323-v8.5.5 #67337 +/- ##
================================================================
Coverage ? 55.5244%
================================================================
Files ? 1817
Lines ? 651533
Branches ? 0
================================================================
Hits ? 361760
Misses ? 262908
Partials ? 26865
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
I found one concrete CI issue from the Jenkins build log and pushed a fix for it. Latest commit: What was fixed:
Why:
Local verification:
I will continue watching this PR and fix the next actionable failures/comments. |
|
I found one concrete CI issue from the Jenkins build log and pushed a fix for it.\n\nLatest commit: 2ceaef1\n\nWhat was fixed:\n- add missing Bazel strict dependency to \n\nWhy:\n- now imports , but the Bazel target deps were not updated, which caused the strict-deps failure in build/check jobs.\n\nLocal verification:\n- panic: The hack package only supports go1.25, please confirm the correctness of the ABI before upgrading goroutine 1 [running]: |
|
Addressed the current actionable review items in a follow-up commit:
Local verification:
Please take another look. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/executor/simple.go (1)
100-109: Minor typo in comment."infomations" should be "information".
✏️ Suggested fix
-// resourceOptionsInfo represents the resource infomations to limit user. +// resourceOptionsInfo represents the resource information to limit user.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 100 - 109, The top comment above the resourceOptionsInfo struct has a typo: replace "infomations" with "information" in the doc comment describing resourceOptionsInfo (the comment that lists MAX_QUERIES_PER_HOUR, MAX_UPDATES_PER_HOUR, MAX_CONNECTIONS_PER_HOUR and MAX_USER_CONNECTIONS) so the comment reads "resourceOptionsInfo represents the resource information to limit user." and keep the rest of the sentence unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/executor/simple.go`:
- Around line 100-109: The top comment above the resourceOptionsInfo struct has
a typo: replace "infomations" with "information" in the doc comment describing
resourceOptionsInfo (the comment that lists MAX_QUERIES_PER_HOUR,
MAX_UPDATES_PER_HOUR, MAX_CONNECTIONS_PER_HOUR and MAX_USER_CONNECTIONS) so the
comment reads "resourceOptionsInfo represents the resource information to limit
user." and keep the rest of the sentence unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cae8c825-5fb6-408f-aad4-1a9c9d1b7047
📒 Files selected for processing (11)
pkg/executor/simple.gopkg/executor/test/simpletest/simple_test.gopkg/parser/parser.gopkg/parser/parser.ypkg/server/conn.gopkg/server/conn_test.gopkg/server/err/error.gopkg/server/user_connections.gopkg/server/user_connections_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/sysvar.go
✅ Files skipped from review due to trivial changes (1)
- pkg/server/user_connections_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/server/err/error.go
- pkg/parser/parser.go
- pkg/executor/test/simpletest/simple_test.go
- pkg/server/user_connections.go
- pkg/server/conn.go
|
I found and fixed a deterministic Bazel packaging issue from the latest Jenkins failures. Latest commit: What was fixed:
Why:
|
|
Found another deterministic Bazel sync issue from the latest reruns and pushed a follow-up fix. Latest commit: What was fixed:
Why:
Local verification:
|
|
Found a deterministic unit-test baseline mismatch from the latest reruns and pushed a small follow-up fix. Latest commit: What was fixed:
Why:
Local verification:
I will keep watching the reruns. |
|
Found another deterministic information_schema baseline mismatch from the latest unit-test reruns and pushed a follow-up fix. Latest commit: What was fixed:
Why:
Local verification:
|
|
I found and fixed a deterministic BR system-table restore issue from the latest Jenkins failures. Latest commit: What was fixed:
Why:
Local verification:
I am still watching the remaining reruns. |
bb7133
changed the title
|
/retest |
|
/test mysql-test |
|
@EmmaDuDu: No presubmit jobs available for pingcap/tidb@release-8.5-20260323-v8.5.5 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test mysql-test |
|
@EmmaDuDu: No presubmit jobs available for pingcap/tidb@release-8.5-20260323-v8.5.5 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test mysql-test |
|
@EmmaDuDu: No presubmit jobs available for pingcap/tidb@release-8.5-20260323-v8.5.5 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test mysql-test |
|
@EmmaDuDu: No presubmit jobs available for pingcap/tidb@release-8.5-20260323-v8.5.5 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
bb7133
changed the title
|
/retest |
|
/test mysql-test tidb-test=pr/2720 |
|
@EmmaDuDu: No presubmit jobs available for pingcap/tidb@release-8.5-20260323-v8.5.5 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test mysql-test tidb-test=pr/2720 |
|
@EmmaDuDu: No presubmit jobs available for pingcap/tidb@release-8.5-20260323-v8.5.5 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@EmmaDuDu: No presubmit jobs available for pingcap/tidb@release-8.5-20260323-v8.5.5 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Commit 7a9e1d1 moved user connection counting into conn.go's openSessionAndDoAuth() (when reserveUserConnection=true), so that a rejection during the handshake sends a proper MySQL error packet back to the client rather than a raw TCP reset. However, the increaseUserConnectionsCount() call in server.go:onConn was not removed, causing each connection to count twice against the per-user limit. With max_user_connections=2 this meant: - conn.go increments to 1 (new entry) - server.go increments to 2 (already at limit) - 2nd connection is rejected immediately Remove only the redundant increment from server.go while keeping the defer conn.decreaseUserConnectionCount() in place, which is still needed to decrement the count when a successful connection eventually closes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/retest |
|
/test unit-test |
|
@EmmaDuDu: No presubmit jobs available for pingcap/tidb@release-8.5-20260323-v8.5.5 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
1 similar comment
|
/retest |
| PRIMARY KEY (Host, User));` | ||
| Max_user_connections INT UNSIGNED NOT NULL DEFAULT 0, | ||
| PRIMARY KEY (Host, User), | ||
| KEY i_user (User));` |
There was a problem hiding this comment.
Unintened change?
KEY i_user is added by 1M user project, where we need to query by user name without host, so the primary key is not enough and this index is added.
Here we change the bootstrap.go, but upgrade.go doesn't have it, this would cause some problem. @bb7133
bb7133
force-pushed
the
cherry-pick-59197-to-release-8.5-20260323-v8.5.5
branch
from
6f492f3 to
60b547f
Compare
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, tangenta, tiancaiamao, yudongusa, YuJuncen The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
merged commit 6c7aaa0
into
pingcap:release-8.5-20260323-v8.5.5
|
@bb7133: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
10 participants
This is an automated cherry-pick of #59197
What problem does this PR solve?
Issue Number: close #59203
Problem Summary:
TiDB supports
max_user_connectionsto limit the number of user connections.What changed and how does it work?
release-8.5-20260323-v8.5.5max_user_connectionssysvarConflict resolution for release-8.5
To make the cherry-pick work on
release-8.5-20260323-v8.5.5, I made the following conflict-resolution / compatibility adjustments:pkg/server/user_connections.goto the release-8.5MatchIdentitysignatureversion244only, instead of referencing non-existent intermediate upgrade functionsmax_user_connectionsas a real sysvar and removed the duplicate noop registration that shadowed it, soSET GLOBALandSHOW VARIABLESbehave correctly on this branchsysvar.go/vardef/tidb_vars.goreferences to the release-8.5 code layoutThese are porting-only changes to resolve branch drift; they are not intended to change the original feature behavior beyond making the cherry-pick work correctly on the target release branch.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Bug Fixes
Migration
Tests