pingcap / pingcap/tidb

planner: avoid the unconditional visitInfo copy in VisitInfo4PrivCheck

Open
#71,002 0 comments 0 reactions 0 assignees View on GitHub
component/privilege sig/planner type/enhancement
Dominant language
Go
Stars
40.5k
Forks
6.2k
PR merge metrics
PR metrics pending

Description

## Enhancement

`VisitInfo4PrivCheck` always allocates a new slice and copies every element, even when the
result is byte-for-byte identical to its input. Since column-level visit info was introduced
(#61741), `len(vs)` grew by an order of magnitude for wide `SELECT`s, so this copy went from
negligible to a measurable per-execution cost.

### Where

`pkg/planner/core/optimizer.go`, the `default` branch:

```go
default:
privVisitInfo = make([]visitInfo, 0, len(vs))
for _, v := range vs {
if needCheckTmpTablePriv(ctx, is, v) {
privVisitInfo = append(privVisitInfo, v)
}
}
```

`needCheckTmpTablePriv` returns `false` only for local temporary tables. On a cluster with no
local temporary tables — the common case — every element passes, and `privVisitInfo` ends up
equal to `vs`. The `make` is pure overhead. The `*ast.GrantStmt` branch right above already
returns the input slice directly (`privVisitInfo = vs`), so returning `vs` is an existing
pattern here.

This runs on every statement execution, including prepared-statement executes that hit the
plan cache, via `checkPreparedPriv` (`pkg/planner/core/plan_cache.go`).

### Measurement

Heap profile diff (`alloc_space`), v8.5.5 vs v8.5.6, identical 50s workload, 96 connections,
unistore. Query: `SELECT users.* FROM users JOIN auth ON users.id = auth.user_id WHERE auth.uuid = ?`
(`users` has 22 columns), run as a prepared statement.

| | v8.5.5 | v8.5.6 |
| --- | --- | --- |
| `VisitInfo4PrivCheck` flat | 204.04 MB / 955,158 objects | 2288.69 MB / 781,207 objects |
| bytes per call | **224 B** | **3,072 B** |
| implied `len(vs)` | 2 | ~25–27 |

`visitInfo` is 112 bytes, so 224 B is exactly two table-level entries; 3,072 B is the Go size
class for ~25–27 entries — the 22 `users` columns plus the `auth` columns and table-level
entries. One allocation per call, no hidden per-element allocation: `cap == len(vs)` means the
`append` loop never grows the slice.

Every one of those entries passed `needCheckTmpTablePriv`, so the copy produced an exact
duplicate of the input.

### Proposal

Allocate lazily, on the first element that actually needs to be dropped:

```go
default:
for i, v := range vs {
if needCheckTmpTablePriv(ctx, is, v) {
continue
}
privVisitInfo = make([]visitInfo, i, len(vs)-1)
copy(privVisitInfo, vs[:i])
for _, v := range vs[i+1:] {
if needCheckTmpTablePriv(ctx, is, v) {
privVisitInfo = append(privVisitInfo, v)
}
}
return
}
return vs
```

On clusters without local temporary tables this makes the function allocation-free.

The same shape applies to the `*ast.CreateTableStmt` and `*ast.DropTableStmt` branches — they
only diverge from the input when an entry is rewritten or dropped — but those are DDL paths
and not worth changing for allocation reasons.

### Not covered here

`needCheckTmpTablePriv` calls `is.TableByName` once per entry. With column-level visit info
that is ~27 lookups per statement where only 2 distinct `(db, table)` pairs are involved
(1162.14 MB vs 108.01 MB in the same profile pair). Deduplicating by `(db, table)` is a
separate change; the lazy allocation above does not address it.

Contributor guide

Open the contributing guide

Research direction

Start in pkg/planner/core/optimizer.go at the default branch of VisitInfo4PrivCheck, then trace its prepared-statement path through checkPreparedPriv in pkg/planner/core/plan_cache.go. Preserve the existing filtering behavior while avoiding the copy when no entry is dropped, and verify the relevant planner tests and allocation behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.