privilege/privileges: refactor the data struct for user privilege data | tidb-test=pr/2697#66538
Merged
ti-chi-bot[bot] merged 11 commits intoMar 9, 2026
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
Merged
13 tasks
Member
Author
|
@CbcWestwolf This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
ti-chi-bot
Bot
added
cherry-pick-approved
…61719-to-release-8.5 # Conflicts: # pkg/privilege/privileges/cache.go # pkg/privilege/privileges/cache_test.go
|
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:
📝 WalkthroughWalkthroughRefactors the privilege cache from slice/map-based immutable structures to a concurrency-friendly btree-backed in-memory model, adds bTree wrapper and item types, updates loading/lookup to use tree operations, and centralizes fetch logic for column/table/schema privileges. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
fzzf678
changed the title
Contributor
|
Updates on this cherry-pick PR (#66538) (original: #61719):
Tests:
Title update:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/privilege/privileges/cache_test.go (1)
54-54: Remove stale commented assertions (or replace with new invariants).These commented
require.Lenlines are dead test code now. Please delete them, or replace them with assertions against current public invariants.🧹 Minimal cleanup
- // require.Len(t, p.User(), len(p.UserMap)) @@ - // require.Len(t, p.DB(), len(p.DBMap)) @@ - // require.Len(t, tablesPriv, len(p.TablesPrivMap))Also applies to: 110-110, 129-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/cache_test.go` at line 54, Remove the stale commented test assertions that check lengths (e.g., the commented require.Len(t, p.User(), len(p.UserMap))) — either delete these commented lines or replace them with active assertions that reflect current invariants (for example assert that len(p.User()) equals len(p.UserMap) if that invariant still holds, or assert specific expected entries/keys in p.UserMap). Update all occurrences of these commented assertions (including the ones referencing p.User(), p.UserMap) so the test file contains only live, meaningful assertions.
🤖 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/privilege/privileges/cache.go`:
- Around line 1642-1689: The grantable lookup is currently account-scoped
(grantableUser map and isGrantable) so a table-level GRANT in tables_priv marks
every column row for that account as grantable; change grantableUser to include
table scope (e.g., map[string]map[string]map[string]struct{} keyed by username
-> host -> db.table) and update isGrantable to accept db and table (or a single
dbTable key) so you only mark grantable when tablesPrivRecord.DB and
tablesPrivRecord.TableName match the columns row; when populating grantableUser
from tablesPrivRecords use the DB+TableName as the inner key, and when emitting
rows call isGrantable with columnsPrivRecord.DB and columnsPrivRecord.TableName
(adjust function and call sites in fetchColumnPrivileges, grantableUser,
isGrantable, and the loop over tablesPrivRecords/columnsPrivRecord).
In `@pkg/privilege/privileges/privileges.go`:
- Around line 941-971: The code reads a privilege snapshot into mysqlPrivilege
via p.Handle.Get() inside fetchPrivilege but later other Fetch*Privileges bind
fetchFunc using a different p.Handle.Get(), risking a mixed view if
Handle.Update() runs concurrently; fix by taking a single snapshot once (call
p.Handle.Get() once at the start of the request) and pass that snapshot into any
bound fetchFunc/closures so all privilege reads (the users/hosts assembly and
the fetchFunc) operate on the same snapshot (update usages in functions that
bind fetchFunc to reference the captured mysqlPrivilege instead of calling
p.Handle.Get() again).
---
Nitpick comments:
In `@pkg/privilege/privileges/cache_test.go`:
- Line 54: Remove the stale commented test assertions that check lengths (e.g.,
the commented require.Len(t, p.User(), len(p.UserMap))) — either delete these
commented lines or replace them with active assertions that reflect current
invariants (for example assert that len(p.User()) equals len(p.UserMap) if that
invariant still holds, or assert specific expected entries/keys in p.UserMap).
Update all occurrences of these commented assertions (including the ones
referencing p.User(), p.UserMap) so the test file contains only live, meaningful
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 652b27c4-b190-42a7-ba38-9edf373c2155
📒 Files selected for processing (8)
pkg/ddl/executor.gopkg/privilege/BUILD.bazelpkg/privilege/privilege.gopkg/privilege/privileges/BUILD.bazelpkg/privilege/privileges/cache.gopkg/privilege/privileges/cache_test.gopkg/privilege/privileges/privileges.gopkg/privilege/privileges/tidb_auth_token_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #66538 +/- ##
================================================
Coverage ? 55.5462%
================================================
Files ? 1819
Lines ? 652501
Branches ? 0
================================================
Hits ? 362440
Misses ? 263195
Partials ? 26866
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/privilege/privileges/privileges.go (1)
946-999:⚠️ Potential issue | 🟠 MajorUse one
*MySQLPrivilegesnapshot acrossfetchPrivilegeandfetchFunc.Line 946 reads one snapshot, but Lines 979/988/997 bind
fetchFuncfrom separatep.Handle.Get()calls. A concurrent update can mix old/new privilege views in a single request.Proposed fix
-func (p *UserPrivileges) fetchPrivilege(user *auth.UserIdentity, accessAll bool, fetchFunc func(users, hosts []string) [][]types.Datum) ([][]types.Datum, error) { +func (p *UserPrivileges) fetchPrivilege(mysqlPrivilege *MySQLPrivilege, user *auth.UserIdentity, accessAll bool, fetchFunc func(users, hosts []string) [][]types.Datum) ([][]types.Datum, error) { if SkipWithGrant { return nil, ErrNonexistingGrant.GenWithStackByArgs("root", "%") } - - mysqlPrivilege := p.Handle.Get() users := make([]string, 0, 1) hosts := make([]string, 0, 1) @@ func (p *UserPrivileges) FetchColumnPrivileges(sctx sessionctx.Context, user *auth.UserIdentity) ([][]types.Datum, error) { + mysqlPrivilege := p.Handle.Get() return p.fetchPrivilege( + mysqlPrivilege, user, p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "columns_priv", "", mysql.SelectPriv) && p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "tables_priv", "", mysql.SelectPriv), - p.Handle.Get().fetchColumnPrivileges, + mysqlPrivilege.fetchColumnPrivileges, ) } @@ func (p *UserPrivileges) FetchTablePrivileges(sctx sessionctx.Context, user *auth.UserIdentity) ([][]types.Datum, error) { + mysqlPrivilege := p.Handle.Get() return p.fetchPrivilege( + mysqlPrivilege, user, p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "tables_priv", "", mysql.SelectPriv), - p.Handle.Get().fetchTablePrivileges, + mysqlPrivilege.fetchTablePrivileges, ) } @@ func (p *UserPrivileges) FetchSchemaPrivileges(sctx sessionctx.Context, user *auth.UserIdentity) ([][]types.Datum, error) { + mysqlPrivilege := p.Handle.Get() return p.fetchPrivilege( + mysqlPrivilege, user, p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "db", "", mysql.SelectPriv), - p.Handle.Get().fetchSchemaPrivileges, + mysqlPrivilege.fetchSchemaPrivileges, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/privileges.go` around lines 946 - 999, The code reads a MySQLPrivilege snapshot into mysqlPrivilege but then binds fetchFunc from fresh p.Handle.Get() calls, risking mixed snapshots; change the three bindings (currently p.Handle.Get().fetchColumnPrivileges, p.Handle.Get().fetchTablePrivileges, p.Handle.Get().fetchSchemaPrivileges) to use the captured snapshot (mysqlPrivilege.fetchColumnPrivileges, mysqlPrivilege.fetchTablePrivileges, mysqlPrivilege.fetchSchemaPrivileges) so fetchPrivilege and the fetchFunc it receives operate on the same *MySQLPrivilege instance.
🤖 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/privilege/privileges/privileges.go`:
- Around line 941-999: The fetchPrivilege method signature is too long and the
loop in mysqlPrivilege.user.Ascend shadows the outer variable u; split the
fetchPrivilege signature across multiple lines (break parameters and return
types onto separate lines) so it fits line-length rules, and rename the inner
loop variable in the Ascend closure (e.g., change "for _, u := range itm.data"
to "for _, usr := range itm.data" and update usages of usr.User and usr.Host) to
avoid shadowing the outer u/h variables; after making these edits to
UserPrivileges.fetchPrivilege and its Ascend closure, run make lint and fix any
remaining style issues.
---
Duplicate comments:
In `@pkg/privilege/privileges/privileges.go`:
- Around line 946-999: The code reads a MySQLPrivilege snapshot into
mysqlPrivilege but then binds fetchFunc from fresh p.Handle.Get() calls, risking
mixed snapshots; change the three bindings (currently
p.Handle.Get().fetchColumnPrivileges, p.Handle.Get().fetchTablePrivileges,
p.Handle.Get().fetchSchemaPrivileges) to use the captured snapshot
(mysqlPrivilege.fetchColumnPrivileges, mysqlPrivilege.fetchTablePrivileges,
mysqlPrivilege.fetchSchemaPrivileges) so fetchPrivilege and the fetchFunc it
receives operate on the same *MySQLPrivilege instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dc9cf6a5-54f5-41b0-8e59-4a73ffb0a790
📒 Files selected for processing (1)
pkg/privilege/privileges/privileges.go
Comment on lines
+941
to
+999
| func (p *UserPrivileges) fetchPrivilege(user *auth.UserIdentity, accessAll bool, fetchFunc func(users, hosts []string) [][]types.Datum) ([][]types.Datum, error) { | ||
| if SkipWithGrant { | ||
| return nil, ErrNonexistingGrant.GenWithStackByArgs("root", "%") | ||
| } | ||
|
|
||
| mysqlPrivilege := p.Handle.Get() | ||
| users := make([]string, 0, 1) | ||
| hosts := make([]string, 0, 1) | ||
|
|
||
| u, h := "", "" | ||
| if user != nil { | ||
| u, h = user.Username, user.Hostname | ||
| if len(user.AuthUsername) > 0 && len(user.AuthHostname) > 0 { | ||
| u, h = user.AuthUsername, user.AuthHostname | ||
| } | ||
| } | ||
| if u == "" || accessAll { | ||
| mysqlPrivilege.user.Ascend(func(itm itemUser) bool { | ||
| for _, u := range itm.data { | ||
| users = append(users, u.User) | ||
| hosts = append(hosts, u.Host) | ||
| } | ||
| return true | ||
| }) | ||
| } else { | ||
| users = append(users, u) | ||
| hosts = append(hosts, h) | ||
| } | ||
|
|
||
| return fetchFunc(users, hosts), nil | ||
| } | ||
|
|
||
| // FetchColumnPrivileges implements privilege.Manager interface. | ||
| func (p *UserPrivileges) FetchColumnPrivileges(sctx sessionctx.Context, user *auth.UserIdentity) ([][]types.Datum, error) { | ||
| return p.fetchPrivilege( | ||
| user, | ||
| p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "columns_priv", "", mysql.SelectPriv) && | ||
| p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "tables_priv", "", mysql.SelectPriv), | ||
| p.Handle.Get().fetchColumnPrivileges, | ||
| ) | ||
| } | ||
|
|
||
| // FetchTablePrivileges implements privilege.Manager interface. | ||
| func (p *UserPrivileges) FetchTablePrivileges(sctx sessionctx.Context, user *auth.UserIdentity) ([][]types.Datum, error) { | ||
| return p.fetchPrivilege( | ||
| user, | ||
| p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "tables_priv", "", mysql.SelectPriv), | ||
| p.Handle.Get().fetchTablePrivileges, | ||
| ) | ||
| } | ||
|
|
||
| // FetchSchemaPrivileges implements privilege.Manager interface. | ||
| func (p *UserPrivileges) FetchSchemaPrivileges(sctx sessionctx.Context, user *auth.UserIdentity) ([][]types.Datum, error) { | ||
| return p.fetchPrivilege( | ||
| user, | ||
| p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "db", "", mysql.SelectPriv), | ||
| p.Handle.Get().fetchSchemaPrivileges, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Look for the file in question and understand the PR context
find . -type f -name "privileges.go" | grep -E "pkg/privilege/privileges"Repository: pingcap/tidb
Length of output: 96
🏁 Script executed:
# Search for any lint-related CI output or configuration
find . -type f -name ".golangci.yml" -o -name "Makefile" -o -name "BUILD" | head -20Repository: pingcap/tidb
Length of output: 186
🏁 Script executed:
# Check the actual file to see its current state and if there are any obvious lint issues
head -50 pkg/privilege/privileges/privileges.goRepository: pingcap/tidb
Length of output: 1673
🏁 Script executed:
# Read lines 941-999 of the privileges.go file to see the actual code
sed -n '941,999p' pkg/privilege/privileges/privileges.goRepository: pingcap/tidb
Length of output: 2132
🏁 Script executed:
# Check if there's a recent change to this file
git log --oneline -n 20 -- pkg/privilege/privileges/privileges.go 2>/dev/null || echo "Git history not available"Repository: pingcap/tidb
Length of output: 127
🏁 Script executed:
# Look for any CI configuration or workflow files
find . -type f \( -name ".github" -o -name "*.yml" -o -name "*.yaml" \) -path "*/.github/workflows/*" 2>/dev/null | head -10Repository: pingcap/tidb
Length of output: 157
🏁 Script executed:
# Check the Makefile for the lint target
grep -A 10 "^lint:" Makefile || grep -A 10 "make lint" MakefileRepository: pingcap/tidb
Length of output: 740
🏁 Script executed:
# Read the golangci.yml configuration
head -100 .golangci.ymlRepository: pingcap/tidb
Length of output: 2962
🏁 Script executed:
# Check if any CI workflows run lint checks
grep -r "lint" .github/workflows/ 2>/dev/null || echo "No lint checks in workflows"Repository: pingcap/tidb
Length of output: 83
🏁 Script executed:
# Get more context on the commit that changed this code
git show 6f0ff154 --stat 2>/dev/null | head -30 || echo "Git show not available"Repository: pingcap/tidb
Length of output: 1732
🏁 Script executed:
# Check if there's any mention of lint in the git log message or commits
git log --all --grep="lint" --oneline 2>/dev/null | head -10 || echo "No lint commits found"Repository: pingcap/tidb
Length of output: 733
🏁 Script executed:
# Look for any CI configuration files that might run tests/lint
find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "go test\|make lint" 2>/dev/null | head -10Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
# Check the actual files changed in the commit
git diff 6f0ff154~1 6f0ff154 --name-only 2>/dev/null | grep -E "\.go$|\.bazel|\.bzl|go\.mod|go\.sum" | head -20Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
# Analyze the code snippet for obvious lint violations
python3 << 'EOF'
code = """func (p *UserPrivileges) fetchPrivilege(user *auth.UserIdentity, accessAll bool, fetchFunc func(users, hosts []string) [][]types.Datum) ([][]types.Datum, error) {
if SkipWithGrant {
return nil, ErrNonexistingGrant.GenWithStackByArgs("root", "%")
}
mysqlPrivilege := p.Handle.Get()
users := make([]string, 0, 1)
hosts := make([]string, 0, 1)
u, h := "", ""
if user != nil {
u, h = user.Username, user.Hostname
if len(user.AuthUsername) > 0 && len(user.AuthHostname) > 0 {
u, h = user.AuthUsername, user.AuthHostname
}
}
if u == "" || accessAll {
mysqlPrivilege.user.Ascend(func(itm itemUser) bool {
for _, u := range itm.data {
users = append(users, u.User)
hosts = append(hosts, u.Host)
}
return true
})
} else {
users = append(users, u)
hosts = append(hosts, h)
}
return fetchFunc(users, hosts), nil
}"""
# Check for potential lint issues
issues = []
# Check for long lines (lll rule typically enforces 120 or similar)
for i, line in enumerate(code.split('\n'), 1):
if len(line) > 120:
issues.append(f"Line {i}: Potentially long line ({len(line)} chars)")
# Check for variable shadowing (u, h reused in closure)
if 'for _, u := range' in code and 'u, h := ' in code:
issues.append("Variable shadowing: 'u' is shadowed in the loop closure")
# Check for unused variables
if 'mysqlPrivilege' in code and 'mysqlPrivilege.user' in code:
pass # Used
else:
issues.append("Potential unused variable: mysqlPrivilege")
if issues:
print("Potential lint issues found:")
for issue in issues:
print(f" - {issue}")
else:
print("No obvious lint issues detected in code snippet")
EOFRepository: pingcap/tidb
Length of output: 190
Run make lint and fix the resulting violations for this change.
The code has lint issues that prevent compliance:
- Long function signature (line 941): The
fetchPrivilegemethod signature exceeds line length limits at 162 characters. Consider breaking it across multiple lines. - Variable shadowing (line 962): The loop variable
ushadows the outer scope variableuin the closure. Rename the loop variable to avoid shadowing (e.g.,for _, user := range itm.data).
As per coding guidelines, make lint must be run for all .go changes. After fixing these violations, verify the changes pass linting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/privilege/privileges/privileges.go` around lines 941 - 999, The
fetchPrivilege method signature is too long and the loop in
mysqlPrivilege.user.Ascend shadows the outer variable u; split the
fetchPrivilege signature across multiple lines (break parameters and return
types onto separate lines) so it fits line-length rules, and rename the inner
loop variable in the Ascend closure (e.g., change "for _, u := range itm.data"
to "for _, usr := range itm.data" and update usages of usr.User and usr.Host) to
avoid shadowing the outer u/h variables; after making these edits to
UserPrivileges.fetchPrivilege and its Ascend closure, run make lint and fix any
remaining style issues.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/privilege/privileges/privileges.go (1)
946-947:⚠️ Potential issue | 🟡 MinorUse one privilege snapshot for the whole Fetch*Privileges flow.
fetchPrivilegereadsp.Handle.Get()(Line 946), while each caller bindsfetchFuncfrom anotherp.Handle.Get()(Line 973/982/991). A concurrent update can mix snapshots in one request.🔧 Proposed fix
-func (p *UserPrivileges) fetchPrivilege(user *auth.UserIdentity, accessAll bool, fetchFunc func(users, hosts []string) [][]types.Datum) ([][]types.Datum, error) { +func (p *UserPrivileges) fetchPrivilege(mysqlPrivilege *MySQLPrivilege, user *auth.UserIdentity, accessAll bool, fetchFunc func(users, hosts []string) [][]types.Datum) ([][]types.Datum, error) { if SkipWithGrant { return nil, ErrNonexistingGrant.GenWithStackByArgs("root", "%") } - - mysqlPrivilege := p.Handle.Get() users := make([]string, 0, 1) hosts := make([]string, 0, 1) @@ func (p *UserPrivileges) FetchColumnPrivileges(sctx sessionctx.Context, user *auth.UserIdentity) ([][]types.Datum, error) { + mysqlPrivilege := p.Handle.Get() return p.fetchPrivilege( + mysqlPrivilege, user, p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "columns_priv", "", mysql.SelectPriv) && p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "tables_priv", "", mysql.SelectPriv), - p.Handle.Get().fetchColumnPrivileges, + mysqlPrivilege.fetchColumnPrivileges, ) } @@ func (p *UserPrivileges) FetchTablePrivileges(sctx sessionctx.Context, user *auth.UserIdentity) ([][]types.Datum, error) { + mysqlPrivilege := p.Handle.Get() return p.fetchPrivilege( + mysqlPrivilege, user, p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "tables_priv", "", mysql.SelectPriv), - p.Handle.Get().fetchTablePrivileges, + mysqlPrivilege.fetchTablePrivileges, ) } @@ func (p *UserPrivileges) FetchSchemaPrivileges(sctx sessionctx.Context, user *auth.UserIdentity) ([][]types.Datum, error) { + mysqlPrivilege := p.Handle.Get() return p.fetchPrivilege( + mysqlPrivilege, user, p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "db", "", mysql.SelectPriv), - p.Handle.Get().fetchSchemaPrivileges, + mysqlPrivilege.fetchSchemaPrivileges, ) }Also applies to: 973-973, 982-982, 991-991
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/privileges.go` around lines 946 - 947, The code is using p.Handle.Get() multiple times causing mixed privilege snapshots; instead call p.Handle.Get() once at the start of the entire Fetch*Privileges flow and reuse that single snapshot (e.g., assign to mysqlPrivilege and pass it into fetchPrivilege and into any bound fetchFunc closures) so all operations in Fetch*Privileges reference the same snapshot; update functions fetchPrivilege and any places that bind fetchFunc to accept or close over the single mysqlPrivilege snapshot rather than calling p.Handle.Get() again.
🤖 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/privilege/privileges/privileges.go`:
- Around line 399-406: Change MatchUserResourceGroupName to surface
ExecRestrictedSQL failures instead of treating them as “no dependency”: update
its signature from MatchUserResourceGroupName(exec
sqlexec.RestrictedSQLExecutor, resourceGroupName string) (u string, success
bool) to include an error return (u string, success bool, err error), return the
exec error when ExecRestrictedSQL fails (do not convert it to "", false), and
update all callers (e.g., functions in pkg/ddl/executor.go that call
MatchUserResourceGroupName) to abort the drop operation when a non-nil error is
returned instead of proceeding as if there is no dependent user. Ensure logging
remains contextual (use logutil.BgLogger()/zap) but do not swallow the exec
error.
---
Duplicate comments:
In `@pkg/privilege/privileges/privileges.go`:
- Around line 946-947: The code is using p.Handle.Get() multiple times causing
mixed privilege snapshots; instead call p.Handle.Get() once at the start of the
entire Fetch*Privileges flow and reuse that single snapshot (e.g., assign to
mysqlPrivilege and pass it into fetchPrivilege and into any bound fetchFunc
closures) so all operations in Fetch*Privileges reference the same snapshot;
update functions fetchPrivilege and any places that bind fetchFunc to accept or
close over the single mysqlPrivilege snapshot rather than calling p.Handle.Get()
again.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d3e80c4b-fc81-4bbe-9a89-3f34dd0e6147
📒 Files selected for processing (2)
pkg/privilege/privileges/BUILD.bazelpkg/privilege/privileges/privileges.go
Comment on lines
+399
to
+406
| func (p *UserPrivileges) MatchUserResourceGroupName(exec sqlexec.RestrictedSQLExecutor, resourceGroupName string) (u string, success bool) { | ||
| ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnPrivilege) | ||
| sql := "SELECT user FROM mysql.user WHERE json_extract(user_attributes, '$.resource_group') = %? LIMIT 1" | ||
| rows, _, err := exec.ExecRestrictedSQL(ctx, nil, sql, resourceGroupName) | ||
| if err != nil { | ||
| logutil.BgLogger().Error("execute sql error", zap.String("sql", sql), zap.Error(err)) | ||
| return "", false | ||
| } |
There was a problem hiding this comment.
Do not treat restricted SQL failure as “no dependency found.”
If ExecRestrictedSQL fails (Line 403), returning ("", false) makes callers proceed as if no user depends on the resource group. This should fail closed.
🔧 Proposed fix
-func (p *UserPrivileges) MatchUserResourceGroupName(exec sqlexec.RestrictedSQLExecutor, resourceGroupName string) (u string, success bool) {
+func (p *UserPrivileges) MatchUserResourceGroupName(exec sqlexec.RestrictedSQLExecutor, resourceGroupName string) (u string, matched bool, err error) {
ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnPrivilege)
sql := "SELECT user FROM mysql.user WHERE json_extract(user_attributes, '$.resource_group') = %? LIMIT 1"
rows, _, err := exec.ExecRestrictedSQL(ctx, nil, sql, resourceGroupName)
if err != nil {
- logutil.BgLogger().Error("execute sql error", zap.String("sql", sql), zap.Error(err))
- return "", false
+ return "", false, errors.Annotate(err, "query mysql.user for resource group dependency")
}
if len(rows) > 0 {
- return rows[0].GetString(0), true
+ return rows[0].GetString(0), true, nil
}
- return "", false
+ return "", false, nil
}Also update callers (e.g., pkg/ddl/executor.go) to abort drop operations when err != nil.
As per coding guidelines, "Keep error handling actionable and contextual; avoid silently swallowing errors in Go code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/privilege/privileges/privileges.go` around lines 399 - 406, Change
MatchUserResourceGroupName to surface ExecRestrictedSQL failures instead of
treating them as “no dependency”: update its signature from
MatchUserResourceGroupName(exec sqlexec.RestrictedSQLExecutor, resourceGroupName
string) (u string, success bool) to include an error return (u string, success
bool, err error), return the exec error when ExecRestrictedSQL fails (do not
convert it to "", false), and update all callers (e.g., functions in
pkg/ddl/executor.go that call MatchUserResourceGroupName) to abort the drop
operation when a non-nil error is returned instead of proceeding as if there is
no dependent user. Ensure logging remains contextual (use
logutil.BgLogger()/zap) but do not swallow the exec error.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pkg/privilege/privileges/privileges.go (2)
959-985:⚠️ Potential issue | 🟡 MinorUse a single privilege snapshot for all Fetch*Privileges operations.
Line 938 reads one snapshot (
mysqlPrivilege := p.Handle.Get()), while Lines 965/974/983 bindfetchFuncfrom anotherp.Handle.Get()call. A concurrentHandle.Update()can mix old/new views within a single request.🔧 Suggested fix
-func (p *UserPrivileges) FetchColumnPrivileges(sctx sessionctx.Context, user *auth.UserIdentity) ([][]types.Datum, error) { +func (p *UserPrivileges) FetchColumnPrivileges(sctx sessionctx.Context, user *auth.UserIdentity) ([][]types.Datum, error) { + mysqlPrivilege := p.Handle.Get() return p.fetchPrivilege( + mysqlPrivilege, user, p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "columns_priv", "", mysql.SelectPriv) && p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "tables_priv", "", mysql.SelectPriv), - p.Handle.Get().fetchColumnPrivileges, + mysqlPrivilege.fetchColumnPrivileges, ) }Apply the same pattern to
FetchTablePrivilegesandFetchSchemaPrivileges, and updatefetchPrivilegesignature to accept*MySQLPrivilegeinstead of callingp.Handle.Get()internally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/privileges.go` around lines 959 - 985, The three methods FetchColumnPrivileges, FetchTablePrivileges, and FetchSchemaPrivileges currently call p.Handle.Get() multiple times allowing mixed privilege snapshots; change them to capture a single snapshot (mysqlPrivilege := p.Handle.Get()) and pass that snapshot into fetchPrivilege so all checks and the fetchFunc use the same *MySQLPrivilege instance, and update fetchPrivilege's signature to accept a *MySQLPrivilege instead of calling p.Handle.Get() internally; ensure RequestVerification calls (using sctx.GetSessionVars().ActiveRoles and mysql.SelectPriv) and the fetchFunc (e.g., mysqlPrivilege.fetchTablePrivileges) are all invoked against that single snapshot.
944-950:⚠️ Potential issue | 🟡 MinorVariable shadowing: loop variable
ushadows outer scope.The loop variable
uat line 945 shadows the outerudeclared at line 942. This could cause confusion and is flagged by lint rules.🔧 Suggested fix
mysqlPrivilege.user.Ascend(func(itm itemUser) bool { - for _, u := range itm.data { - users = append(users, u.User) - hosts = append(hosts, u.Host) + for _, record := range itm.data { + users = append(users, record.User) + hosts = append(hosts, record.Host) } return true })As per coding guidelines,
make lintmust be run for all.gochanges.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/privileges.go` around lines 944 - 950, The inner loop in the mysqlPrivilege.user.Ascend callback shadows the outer variable named `u`; rename the inner loop variable (e.g., to `usr` or `item`) in the for _, ... range over itm.data and update the uses in the loop body (the append calls to users and hosts) to avoid shadowing and satisfy the linter (identify the loop inside the mysqlPrivilege.user.Ascend anonymous function and the itemUser.itm.data iteration).pkg/privilege/privileges/cache.go (1)
1656-1711:⚠️ Potential issue | 🟠 Major
IS_GRANTABLEscope is account-level instead of table-level.The
grantableUsermap (lines 1660-1668) is keyed only byuser@host, so a GRANT_OPTION on one table marks all column-privilege rows for that account as grantable. Per the INFORMATION_SCHEMA specification, grantability should be table-scoped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/cache.go` around lines 1656 - 1711, The grantableUser map and isGrantable closure in fetchColumnPrivileges treat GRANT_OPTION at account-level; change the structure to be table-scoped (e.g., grantableUser[userName][hostName][db+"."+tableName] = struct{}{}) and update isGrantable to accept db and tableName and check that nested map key; when populating from tablesPrivRecords.data use tablesPrivRecord.DB and tablesPrivRecord.TableName (and only add when TablePriv&mysql.GrantPriv > 0) and ensure nested maps are initialized before assignment; finally update the call site that builds the Datums (currently calling isGrantable(userName, hostName)) to pass the column record's DB and TableName so grantability is evaluated per table.
🤖 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/privilege/privileges/cache.go`:
- Around line 1217-1231: Rename the misspelled function matchResoureGroup to
matchResourceGroup in pkg/privilege/privileges/cache.go and update all call
sites (e.g., the caller in privileges.go that invokes matchResoureGroup) to use
the new name; ensure you update the function declaration and every reference
(including tests and any interfaces) so the symbol matchResourceGroup is
consistently used.
---
Duplicate comments:
In `@pkg/privilege/privileges/cache.go`:
- Around line 1656-1711: The grantableUser map and isGrantable closure in
fetchColumnPrivileges treat GRANT_OPTION at account-level; change the structure
to be table-scoped (e.g., grantableUser[userName][hostName][db+"."+tableName] =
struct{}{}) and update isGrantable to accept db and tableName and check that
nested map key; when populating from tablesPrivRecords.data use
tablesPrivRecord.DB and tablesPrivRecord.TableName (and only add when
TablePriv&mysql.GrantPriv > 0) and ensure nested maps are initialized before
assignment; finally update the call site that builds the Datums (currently
calling isGrantable(userName, hostName)) to pass the column record's DB and
TableName so grantability is evaluated per table.
In `@pkg/privilege/privileges/privileges.go`:
- Around line 959-985: The three methods FetchColumnPrivileges,
FetchTablePrivileges, and FetchSchemaPrivileges currently call p.Handle.Get()
multiple times allowing mixed privilege snapshots; change them to capture a
single snapshot (mysqlPrivilege := p.Handle.Get()) and pass that snapshot into
fetchPrivilege so all checks and the fetchFunc use the same *MySQLPrivilege
instance, and update fetchPrivilege's signature to accept a *MySQLPrivilege
instead of calling p.Handle.Get() internally; ensure RequestVerification calls
(using sctx.GetSessionVars().ActiveRoles and mysql.SelectPriv) and the fetchFunc
(e.g., mysqlPrivilege.fetchTablePrivileges) are all invoked against that single
snapshot.
- Around line 944-950: The inner loop in the mysqlPrivilege.user.Ascend callback
shadows the outer variable named `u`; rename the inner loop variable (e.g., to
`usr` or `item`) in the for _, ... range over itm.data and update the uses in
the loop body (the append calls to users and hosts) to avoid shadowing and
satisfy the linter (identify the loop inside the mysqlPrivilege.user.Ascend
anonymous function and the itemUser.itm.data iteration).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d38a0a74-9e69-4753-9150-0b21d39f5397
📒 Files selected for processing (2)
pkg/privilege/privileges/cache.gopkg/privilege/privileges/privileges.go
fzzf678
force-pushed
the
cherry-pick-61719-to-release-8.5
branch
from
b51a792 to
f6f604a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/privilege/privileges/privileges.go (1)
933-966:⚠️ Potential issue | 🟡 MinorUse one privilege snapshot for the whole fetch path.
Line 938 reads one snapshot, but Lines 965, 974, and 983 bind
fetchFuncfrom separatep.Handle.Get()calls. A concurrentHandle.Update()can still mix old/new privilege views in one response.🔧 Suggested fix
-func (p *UserPrivileges) fetchPrivilege(user *auth.UserIdentity, accessAll bool, fetchFunc func(users, hosts []string) [][]types.Datum) ([][]types.Datum, error) { +func (p *UserPrivileges) fetchPrivilege( + mysqlPrivilege *MySQLPrivilege, + user *auth.UserIdentity, + accessAll bool, + fetchFunc func(users, hosts []string) [][]types.Datum, +) ([][]types.Datum, error) { if SkipWithGrant { return nil, ErrNonexistingGrant.GenWithStackByArgs("root", "%") } - - mysqlPrivilege := p.Handle.Get() users := make([]string, 0, 1) hosts := make([]string, 0, 1) @@ func (p *UserPrivileges) FetchColumnPrivileges(sctx sessionctx.Context, user *auth.UserIdentity) ([][]types.Datum, error) { + mysqlPrivilege := p.Handle.Get() return p.fetchPrivilege( + mysqlPrivilege, user, p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "columns_priv", "", mysql.SelectPriv) && p.RequestVerification(sctx.GetSessionVars().ActiveRoles, "mysql", "tables_priv", "", mysql.SelectPriv), - p.Handle.Get().fetchColumnPrivileges, + mysqlPrivilege.fetchColumnPrivileges, ) }Also applies to: 971-984
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/privileges.go` around lines 933 - 966, The code reads p.Handle.Get() at the start of fetchPrivilege but later binds fetchFunc by calling p.Handle.Get() again (e.g., in FetchColumnPrivileges), allowing concurrent Handle.Update() to mix snapshots; fix by obtaining a single privilege snapshot once and deriving fetchFunc from that snapshot so the same view is used end-to-end: change FetchColumnPrivileges (and other callers) to call snap := p.Handle.Get() once, compute accessAll using snap if needed, bind fetchFunc from snap.fetchColumnPrivileges (or the corresponding method on the snapshot) and then call p.fetchPrivilege/similar helper passing that snap-bound fetchFunc (or add a snapshot parameter to fetchPrivilege) so no second p.Handle.Get() occurs during the fetch path.pkg/privilege/privileges/cache.go (1)
1658-1704:⚠️ Potential issue | 🟠 MajorScope
IS_GRANTABLEby table, not just account.
grantableUseronly keysuser@host, so oneGRANT OPTIONrow inmysql.tables_privmakes every column-privilege row for that account emitYESat Line 1703, even for other tables. The lookup needsdb + tablescope.🔧 Suggested fix
- grantableUser := make(map[string]map[string]struct{}, 4) - isGrantable := func(username, hostname string) string { - if hostNames, exists := grantableUser[username]; exists { - if _, exists = hostNames[hostname]; exists { - return "YES" - } - } - return "NO" - } + type grantKey struct { + user string + host string + db string + table string + } + grantable := make(map[grantKey]struct{}, 16) + isGrantable := func(username, hostname, db, table string) string { + if _, ok := grantable[grantKey{user: username, host: hostname, db: db, table: table}]; ok { + return "YES" + } + return "NO" + } @@ - if _, exists := grantableUser[userName]; !exists { - grantableUser[userName] = make(map[string]struct{}, 1) - if tablesPrivRecords, exists := p.tablesPriv.Get(itemTablesPriv{username: userName}); exists { - for _, tablesPrivRecord := range tablesPrivRecords.data { - if tablesPrivRecord.TablePriv&mysql.GrantPriv > 0 { - grantableUser[tablesPrivRecord.User][tablesPrivRecord.Host] = struct{}{} - } - } - } + if tablesPrivRecords, exists := p.tablesPriv.Get(itemTablesPriv{username: userName}); exists { + for _, tablesPrivRecord := range tablesPrivRecords.data { + if tablesPrivRecord.TablePriv&mysql.GrantPriv > 0 { + grantable[grantKey{ + user: tablesPrivRecord.User, + host: tablesPrivRecord.Host, + db: tablesPrivRecord.DB, + table: tablesPrivRecord.TableName, + }] = struct{}{} + } + } } @@ - isGrantable(userName, hostName), + isGrantable(userName, hostName, columnsPrivRecord.DB, columnsPrivRecord.TableName), ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/cache.go` around lines 1658 - 1704, The grantable lookup currently keys only by username/host (grantableUser and isGrantable), so a single GRANT OPTION on any table marks all column rows for that account as YES; instead scope grantability by database+table as well. Change grantableUser to include table scope (e.g., map[string]map[string]map[string]struct{} or map[string]map[string]struct{} where the inner key is db+"|"+table) when populating from p.tablesPriv (iterate tablesPrivRecord.DB and tablesPrivRecord.TableName and only record hosts for that specific db/table when tablesPrivRecord.TablePriv&mysql.GrantPriv>0) and update isGrantable to accept/check db and table (use columnsPrivRecord.DB and columnsPrivRecord.TableName when calling) so only the matching table-level grants return "YES".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/privilege/privileges/cache.go`:
- Around line 1658-1704: The grantable lookup currently keys only by
username/host (grantableUser and isGrantable), so a single GRANT OPTION on any
table marks all column rows for that account as YES; instead scope grantability
by database+table as well. Change grantableUser to include table scope (e.g.,
map[string]map[string]map[string]struct{} or map[string]map[string]struct{}
where the inner key is db+"|"+table) when populating from p.tablesPriv (iterate
tablesPrivRecord.DB and tablesPrivRecord.TableName and only record hosts for
that specific db/table when tablesPrivRecord.TablePriv&mysql.GrantPriv>0) and
update isGrantable to accept/check db and table (use columnsPrivRecord.DB and
columnsPrivRecord.TableName when calling) so only the matching table-level
grants return "YES".
In `@pkg/privilege/privileges/privileges.go`:
- Around line 933-966: The code reads p.Handle.Get() at the start of
fetchPrivilege but later binds fetchFunc by calling p.Handle.Get() again (e.g.,
in FetchColumnPrivileges), allowing concurrent Handle.Update() to mix snapshots;
fix by obtaining a single privilege snapshot once and deriving fetchFunc from
that snapshot so the same view is used end-to-end: change FetchColumnPrivileges
(and other callers) to call snap := p.Handle.Get() once, compute accessAll using
snap if needed, bind fetchFunc from snap.fetchColumnPrivileges (or the
corresponding method on the snapshot) and then call p.fetchPrivilege/similar
helper passing that snap-bound fetchFunc (or add a snapshot parameter to
fetchPrivilege) so no second p.Handle.Get() occurs during the fetch path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 13dfdd60-b390-42eb-a070-b8234b1bfcad
📒 Files selected for processing (6)
pkg/privilege/BUILD.bazelpkg/privilege/privileges/BUILD.bazelpkg/privilege/privileges/cache.gopkg/privilege/privileges/cache_test.gopkg/privilege/privileges/privileges.gopkg/privilege/privileges/tidb_auth_token_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/privilege/privileges/BUILD.bazel
- pkg/privilege/BUILD.bazel
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
fzzf678
removed
the
do-not-merge/hold
Contributor
|
/retest |
4 similar comments
Contributor
|
/retest |
Contributor
|
/retest |
Contributor
|
/retest |
Contributor
|
/retest |
…61719-to-release-8.5
Contributor
|
/retest |
…5' into cherry-pick-61719-to-release-8.5
Contributor
|
/retest |
4 similar comments
Contributor
|
/retest |
Contributor
|
/retest |
Contributor
|
/retest |
Contributor
|
/retest |
This was referenced Mar 9, 2026
ti-chi-bot
Bot
added
cherry-pick-approved
13 tasks
Merged
13 tasks
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
4 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 automated cherry-pick of #61719
What problem does this PR solve?
Issue Number: ref #61706
Problem Summary:
What changed and how does it work?
MySQLPrivilegewith btreeFetchColumnPrivileges,FetchTablePrivilegesandFetchSchemaPrivilegesto reusefetchPrivilegeRelated mysql test: https://github.com/PingCAP-QE/tidb-test/pull/2543
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
Refactor
Chores