pingcap / pingcap/tidb

stmtsummary: v2 persistent setup/reader bugs — silent Nop logger, FD leak, absolute-path time pruning

Open
#70,174 8 comments 0 reactions 0 assignees View on GitHub
affects-8.5 component/observability severity/critical sig/diagnosis type/bug
Dominant language
Go
Stars
40.5k
Forks
6.2k
PR merge metrics
PR metrics pending

Description

## Bug Report

The `pkg/util/stmtsummary/v2` package (the experimental file-persistent
statement summary backend gated by `tidb_stmt_summary_enable_persistent`)
has three independent defects on `master` that together make the persistent
mode unsafe to enable: it can silently drop rotated windows, leak file
descriptors on time-range queries, and disable file-level time pruning
whenever the configured filename is an absolute path.

### 1. Minimal reproduce step (Required)

The three defects are reachable in production with the defaults shipped by
TiDB; each is exercised by a regression test added in the linked PR.

**1a. `Setup` swallows logger initialization failures and degrades to a Nop logger**

```toml
[instance]
tidb_stmt_summary_enable_persistent = true
tidb_stmt_summary_filename = "/nonexistent/dir/tidb-statements.log"
```

Start `tidb-server`. `stmtsummaryv2.Setup` calls `newStmtLogStorage`
which calls `log.InitLogger`; when the configured path cannot be opened
(unwritable directory, existing directory used as filename, permission
denied, etc.), `log.InitLogger` returns an error, and the legacy
`newStmtLogStorage` swallowed the error and returned
`&stmtLogStorage{logger: zap.NewNop()}`. To the operator persistent mode
looks enabled, but every rotated window is silently written to `/dev/null`.

Worse, even after the first patches surface this error so `NewStmtSummary`
returns `(nil, err)`, `Setup` still leaves `GlobalStmtSummary == nil`
while the cluster config keeps `tidb_stmt_summary_enable_persistent = true`.
The v2 public proxies (`Add`, `Enabled`, `EnabledInternal`, ...) read that
flag and unconditionally dereference `GlobalStmtSummary`; the first SQL
from any session then nil-dereferences and crashes the server, replacing
silent data loss with a hard boot-loop.

**1b. History reader leaks OS file descriptors for files excluded by the time range**

```sql
SELECT * FROM information_schema.STATEMENTS_SUMMARY_HISTORY
WHERE SUMMARY_BEGIN_TIME >= '2026-07-01' AND SUMMARY_END_TIME < '2026-07-27';
```

`newStmtFiles` (in `pkg/util/stmtsummary/v2/reader.go`) enumerates the
log directory with `os.ReadDir`, and for each candidate sibling file
calls `openStmtFile(path)` just to read the first/last record's
`begin`/`end` metadata. If the file does not overlap any requested
`StmtTimeRange` the walk simply returns without closing the FD. Each
excluded rotated file thus leaks one FD for the lifetime of the TiDB
process; on a long-running server with daily rotation this eventually
exhausts the file-descriptor table.

**1c. File-level time pruning is disabled when the configured filename is absolute**

```toml
[instance]
tidb_stmt_summary_enable_persistent = true
tidb_stmt_summary_filename = "/var/log/tidb/tidb-statements.log"
```

`parseEndTs(file)` derives the rotated-name prefix from the *full*
configured filename: with the absolute path above the prefix becomes
`"/var/log/tidb/tidb-statements"` while the rotated base name compared
against is `"tidb-statements-2022-12-27T16-21-20.245"`. The
`strings.HasPrefix` check never matches, so `parseEndTs` silently
returns `0`, which `timeRangeOverlap` treats as `MaxInt64`. Every
rotated sibling file ends up with an effective time range of
`[begin, +inf)` and is opened and scanned regardless of the requested
window. Row-level filtering in the scan worker keeps results correct,
but the absolute-path configuration loses all of the file-level pruning
the directory walk is supposed to provide.

### 2. What did you expect to see? (Required)

* When the configured statement log path cannot be opened, the server
must not silently degrade to a no-op logger while advertising
persistent mode as enabled. Either start-up must fail, or persistent
mode must be explicitly disabled with a clear, observable degraded
state and the v1 in-memory aggregation must take over.
* The history reader must not hold onto OS file descriptors for files
it ultimately excludes from the scan set.
* An absolute path for `tidb_stmt_summary_filename` must behave the
same as a relative one for file-level pruning.

### 3. What did you see instead (Required)

* Silent data loss of every rotated window after a logger init
failure; subsequently, a nil-pointer panic on the first SQL once the
error is surfaced without disabling persistent mode.
* One FD leaked per excluded rotated sibling file, until the
file-descriptor table is exhausted.
* Disabled file-level time pruning under absolute-path configurations,
effectively turning each history query into a full directory scan.

### 4. What is your TiDB version? (Required)

```
$ git rev-parse origin/master
f0599c2f6cddl: add RestoreWithoutTableName flag for modify column generated expression (#66233) (#69934)
```

Reproduced at HEAD of `master` as of the linked PR. The affected code
under `pkg/util/stmtsummary/v2/` was added by the v2 statement-summary
work; the fixes in the linked PR are confined to that package plus the
Bazel test metadata.

Contributor guide

Open the contributing guide

Research direction

Start in pkg/util/stmtsummary/v2/, especially reader.go, Setup, newStmtLogStorage, and parseEndTs. Reproduce the invalid-path, excluded-file time-range, and absolute-path cases, then inspect the regression tests mentioned in the issue. Done means logger failures are observable without a nil global, excluded files do not retain file descriptors, and absolute paths preserve file-level pruning.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.