executor: introduce max_user_connections#68965
Merged
Merged
Conversation
ti-chi-bot
Bot
added
do-not-merge/cherry-pick-not-approved
do-not-merge/release-note-label-needed
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (23)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughAdds global and per-user max_user_connections: sysvar and runtime value, bootstrap column + version bump to v231, parser and executor CREATE/ALTER/SHOW changes, privilege cache/API, server-side per-user counting/enforcement with a new error, and tests. ChangesMAX_USER_CONNECTIONS resource limit implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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: 1
🧹 Nitpick comments (2)
pkg/executor/test/simpletest/simple_test.go (2)
271-273: ⚡ Quick winPrefer error-code assertion over full parser message matching.
Line 273 asserts the complete parser message (including column offset/format), which is brittle. Assert parse error code instead.
Suggested change
- _, err := tk.Exec(alterUserSQL) - require.Error(t, err) - require.Equal(t, err.Error(), "[parser:1064]You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use line 1 column 58 near \"-2;\" ") + tk.MustGetErrCode(alterUserSQL, mysql.ErrParse)As per coding guidelines, test changes should stay minimal and deterministic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/test/simpletest/simple_test.go` around lines 271 - 273, The test currently matches the entire parser message from tk.Exec(alterUserSQL) which is brittle; replace the exact equality check (require.Equal(t, err.Error(), ...)) with an assertion that verifies the parse error code only (e.g., use require.Contains(t, err.Error(), "[parser:1064]") or an equivalent error-code check) while keeping require.Error(t, err) and using the same tk.Exec and alterUserSQL variables so the test remains minimal and deterministic.
279-286: ⚡ Quick winAdd a SHOW CREATE USER assertion for the new resource clause.
This test verifies persistence in
mysql.user, but it doesn’t yet validate the SQL-facing output contract (WITH MAX_USER_CONNECTIONS) added in executor show logic.Suggested addition
alterUserSQL = `ALTER USER 'test2'@'localhost' WITH MAX_USER_CONNECTIONS 50000;` tk.MustExec(alterUserSQL) result = tk.MustQuery(`select user, max_user_connections from mysql.user WHERE User="test2"`) result.Check(testkit.Rows("test2 50000")) + showRows := tk.MustQuery(`SHOW CREATE USER 'test2'@'localhost'`).Rows() + require.Contains(t, showRows[0][0].(string), "WITH MAX_USER_CONNECTIONS 50000")Based on learnings, for SQL behavior changes in
pkg/executor/**, perform targeted unit test plus relevant integration test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/test/simpletest/simple_test.go` around lines 279 - 286, Add assertions that validate the SQL-facing output by running SHOW CREATE USER after creating and altering the user and checking that the returned CREATE USER statement contains the WITH MAX_USER_CONNECTIONS clause. Specifically, after tk.MustExec(createUserSQL) and after tk.MustExec(alterUserSQL) call tk.MustQuery("SHOW CREATE USER 'test2'@'localhost'") (or the testkit equivalent), capture the result and use result.Check(...) to assert the returned DDL string includes "WITH MAX_USER_CONNECTIONS 50000" so the executor's show logic is verified alongside the mysql.user persistence checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/privilege/privileges/privileges.go`:
- Around line 317-345: The new getters miss honoring the skip-grant fast-path;
modify GetEncodedPassword and GetUserResources to check
mysqlPriv.SkipWithGrant() after obtaining record :=
mysqlPriv.connectionVerification(user, host) and, when SkipWithGrant() is true,
return the record values directly (for GetEncodedPassword return
record.AuthenticationString or "" if record==nil; for GetUserResources return
record.MaxUserConnections, nil or 0,nil if record==nil) instead of performing
the normal isValidHash/error logic; otherwise keep the existing isValidHash
checks and error returns.
---
Nitpick comments:
In `@pkg/executor/test/simpletest/simple_test.go`:
- Around line 271-273: The test currently matches the entire parser message from
tk.Exec(alterUserSQL) which is brittle; replace the exact equality check
(require.Equal(t, err.Error(), ...)) with an assertion that verifies the parse
error code only (e.g., use require.Contains(t, err.Error(), "[parser:1064]") or
an equivalent error-code check) while keeping require.Error(t, err) and using
the same tk.Exec and alterUserSQL variables so the test remains minimal and
deterministic.
- Around line 279-286: Add assertions that validate the SQL-facing output by
running SHOW CREATE USER after creating and altering the user and checking that
the returned CREATE USER statement contains the WITH MAX_USER_CONNECTIONS
clause. Specifically, after tk.MustExec(createUserSQL) and after
tk.MustExec(alterUserSQL) call tk.MustQuery("SHOW CREATE USER
'test2'@'localhost'") (or the testkit equivalent), capture the result and use
result.Check(...) to assert the returned DDL string includes "WITH
MAX_USER_CONNECTIONS 50000" so the executor's show logic is verified alongside
the mysql.user persistence checks.
🪄 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: 75b2bdd9-6c8d-418b-8658-7276c16bd6d1
📒 Files selected for processing (23)
br/pkg/restore/snap_client/systable_restore_test.goerrors.tomlpkg/errno/errname.gopkg/executor/infoschema_reader_test.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/variable/noop.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tidb_vars.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 #68965 +/- ##
================================================
Coverage ? 56.4254%
================================================
Files ? 1848
Lines ? 672980
Branches ? 0
================================================
Hits ? 379732
Misses ? 265479
Partials ? 27769
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
bb7133
force-pushed
the
bb7133/cp-6c7aaa0-release-8.5
branch
3 times, most recently
from
949a4de to
e37cb81
Compare
bb7133
added
the
release-note
ti-chi-bot
Bot
removed
the
release-note
bb7133
added
the
release-note
ti-chi-bot
Bot
added
release-note
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/executor/test/simpletest/simple_test.go (1)
297-298: 💤 Low valuePrefer
require.NoErrorfor error assertions.Using
require.Nil(t, err)works butrequire.NoError(t, err)provides better diagnostic output when the assertion fails. The rest of this file usesrequire.NoError(e.g., line 292).♻️ Suggested fix
tk.MustExec(`GRANT CREATE USER ON *.* TO 'test1'@'localhost'`) _, err = tkTest1.Exec(`ALTER USER 'test1'@'localhost' WITH MAX_USER_CONNECTIONS 2`) -require.Nil(t, err) +require.NoError(t, err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/test/simpletest/simple_test.go` around lines 297 - 298, Replace the require.Nil assertion with require.NoError for the tkTest1.Exec call: the call to tkTest1.Exec(`ALTER USER 'test1'@'localhost' WITH MAX_USER_CONNECTIONS 2`) currently checks err with require.Nil(t, err) — change this to require.NoError(t, err) to match the rest of the file and provide better diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/executor/test/simpletest/simple_test.go`:
- Around line 297-298: Replace the require.Nil assertion with require.NoError
for the tkTest1.Exec call: the call to tkTest1.Exec(`ALTER USER
'test1'@'localhost' WITH MAX_USER_CONNECTIONS 2`) currently checks err with
require.Nil(t, err) — change this to require.NoError(t, err) to match the rest
of the file and provide better diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fbdd3ba0-71a5-48a9-8d84-5cdf667eb25d
📒 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/variable/noop.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tidb_vars.go
💤 Files with no reviewable changes (1)
- pkg/sessionctx/variable/noop.go
✅ Files skipped from review due to trivial changes (1)
- pkg/server/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (16)
- pkg/server/err/error.go
- pkg/parser/parser.go
- errors.toml
- pkg/executor/show.go
- pkg/server/conn.go
- pkg/privilege/privilege.go
- pkg/sessionctx/variable/sysvar.go
- pkg/privilege/privileges/cache.go
- pkg/server/user_connections_test.go
- pkg/sessionctx/variable/tidb_vars.go
- pkg/session/bootstrap.go
- pkg/session/bootstrap_test.go
- pkg/privilege/privileges/privileges.go
- pkg/parser/parser.y
- pkg/server/server.go
- pkg/server/user_connections.go
Member
Author
|
/test unit-test |
|
@bb7133: The specified target(s) for Use 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. |
Member
Author
|
/test unit-test |
|
@bb7133: The specified target(s) for Use 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
force-pushed
the
bb7133/cp-6c7aaa0-release-8.5
branch
from
e37cb81 to
07183cb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
br/pkg/restore/snap_client/systable_restore_test.go (1)
57-59: 💤 Low valueMinor grammar fix: use singular verb.
"user table" is singular and should take "has" rather than "have".
📝 Proposed fix
- // user table in cluster have more columns: still compatible, but should fall + // user table in cluster has more columns: still compatible, but should fall // back to non-physical loading because direct physical loading can fail // checksum validation when the backup snapshot lacks the new columns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@br/pkg/restore/snap_client/systable_restore_test.go` around lines 57 - 59, Update the grammatical error in the comment that currently reads "user table in cluster have more columns: still compatible..." to use the singular verb "has" (i.e., "user table in cluster has more columns..."); locate the comment by searching for the exact phrase "user table in cluster have more columns" in systable_restore_test.go and replace "have" with "has".pkg/executor/show.go (1)
1882-1886: 💤 Low valuePrefer
GetUint64for consistency with otherUNSIGNEDcolumn handling.
Max_user_connectionsis defined asINT UNSIGNEDin the schema. For consistency with how other unsigned columns likePassword_reuse_historyare read (line 1842 usesGetUint64), consider using:maxUserConnections := rows[0].GetUint64(11)Then adjust the comparison and formatting to use
uint64. While the current implementation works (typical values are small and fit safely inint64), using the correct unsigned type improves consistency and future-proofs the code.♻️ Proposed refactor
- maxUserConnections := rows[0].GetInt64(11) + maxUserConnections := rows[0].GetUint64(11) maxUserConnectionsStr := "" if maxUserConnections > 0 { maxUserConnectionsStr = fmt.Sprintf(" WITH MAX_USER_CONNECTIONS %d", maxUserConnections) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/show.go` around lines 1882 - 1886, Replace the signed read with an unsigned read: change rows[0].GetInt64(11) to rows[0].GetUint64(11) and make maxUserConnections a uint64, then update the comparison (keep > 0 but applied to uint64) and pass the uint64 into fmt.Sprintf when building the maxUserConnectionsStr (ensure the format specifier matches the uint64 value). This updates the use of maxUserConnections in the existing block that constructs the " WITH MAX_USER_CONNECTIONS ..." string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@br/pkg/restore/snap_client/systable_restore_test.go`:
- Around line 57-59: Update the grammatical error in the comment that currently
reads "user table in cluster have more columns: still compatible..." to use the
singular verb "has" (i.e., "user table in cluster has more columns..."); locate
the comment by searching for the exact phrase "user table in cluster have more
columns" in systable_restore_test.go and replace "have" with "has".
In `@pkg/executor/show.go`:
- Around line 1882-1886: Replace the signed read with an unsigned read: change
rows[0].GetInt64(11) to rows[0].GetUint64(11) and make maxUserConnections a
uint64, then update the comparison (keep > 0 but applied to uint64) and pass the
uint64 into fmt.Sprintf when building the maxUserConnectionsStr (ensure the
format specifier matches the uint64 value). This updates the use of
maxUserConnections in the existing block that constructs the " WITH
MAX_USER_CONNECTIONS ..." string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ef35c035-f547-4606-976f-bb2c6026d9a4
📒 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/variable/noop.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tidb_vars.go
💤 Files with no reviewable changes (14)
- pkg/server/BUILD.bazel
- pkg/sessionctx/variable/noop.go
- pkg/privilege/privileges/cache.go
- pkg/privilege/privilege.go
- pkg/server/server.go
- pkg/session/bootstrap_test.go
- pkg/server/err/error.go
- pkg/server/conn.go
- pkg/sessionctx/variable/tidb_vars.go
- pkg/sessionctx/variable/sysvar.go
- pkg/server/user_connections_test.go
- pkg/privilege/privileges/privileges.go
- pkg/session/bootstrap.go
- pkg/server/user_connections.go
✅ Files skipped from review due to trivial changes (1)
- errors.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/errno/errname.go
- pkg/parser/parser.y
- pkg/parser/parser.go
- pkg/executor/test/simpletest/simple_test.go
- pkg/executor/simple.go
bb7133
force-pushed
the
bb7133/cp-6c7aaa0-release-8.5
branch
from
07183cb to
fa93b4b
Compare
ti-chi-bot
Bot
added
cherry-pick-approved
D3Hunter
approved these changes
Jun 11, 2026
tiancaiamao
approved these changes
Jun 11, 2026
[LGTM Timeline notifier]Timeline:
|
tangenta
approved these changes
Jun 12, 2026
YuJuncen
approved these changes
Jun 12, 2026
|
[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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an manual cherry-pick of #59197 to release-8.5 branch
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.5max_user_connectionssysvarConflict resolution for release-8.5
These 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