Repository navigation
server: limit the size for COM_STMT_SEND_LONG_DATA in each connection (release-7.5-20260724-v7.5.7) (#70350) | tidb-test=release-7.5.7 tikv=v7.5.7 pd=v7.5.7 tiflash=v7.5.7 - #70442
Conversation
…pingcap#70350) close pingcap#70349 (cherry picked from commit bff1263) Signed-off-by: bb7133 <bb7133@gmail.com>
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note-none
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPrepared-statement long-data bytes now use session memory tracking. Quota refusal is reported separately from packet-size overflow. Reset and Close release tracked memory. Tests cover quota enforcement, cleanup, later acceptance, and statistics lease isolation. ChangesLong-data memory quota enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![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 (1)
pkg/server/conn_stmt_test.go (1)
612-617: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the Reset cleanup path.
This test covers cleanup after failed
COM_STMT_EXECUTEandClose. It does not exercise the newTiDBStatement.Resetcleanup call.Add an accepted long-data sequence, dispatch
COM_STMT_RESET, and verify thatMemTracker.BytesConsumed()returns tobaseand the parameter buffer is cleared.🤖 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/server/conn_stmt_test.go` around lines 612 - 617, Extend the statement cleanup test around TiDBStatement.Reset by dispatching COM_STMT_RESET after an accepted long-data sequence, then assert BoundParams()[0] is cleared and vars.MemTracker.BytesConsumed() returns to base. Keep the existing release and Close assertions intact.
🤖 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/server/driver_tidb.go`:
- Around line 115-120: Update the long-data handling around boundParams and its
release paths to track each parameter’s reserved slice capacity, not only
len(data). Reserve and charge the capacity before appending or replacing
boundParams, store the reservation per parameter, and release that exact
reservation in the cleanup paths at the current release blocks. Add a
multi-chunk test that forces backing-slice growth and verifies session memory
tracking remains balanced.
---
Nitpick comments:
In `@pkg/server/conn_stmt_test.go`:
- Around line 612-617: Extend the statement cleanup test around
TiDBStatement.Reset by dispatching COM_STMT_RESET after an accepted long-data
sequence, then assert BoundParams()[0] is cleared and
vars.MemTracker.BytesConsumed() returns to base. Keep the existing release and
Close assertions intact.
🪄 Autofix
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 Plus
Run ID: dcd23c26-d27b-46f6-9e5f-411b88210c23
📒 Files selected for processing (2)
pkg/server/conn_stmt_test.gopkg/server/driver_tidb.go
| released := int64(len(ts.boundParams[paramID])) | ||
| if released > 0 { | ||
| ts.ctx.GetSessionVars().MemTracker.Consume(-released) | ||
| ts.boundLongDataBytes -= released | ||
| } | ||
| ts.boundParams[paramID] = append(ts.boundParams[paramID], data...) | ||
| ts.boundParams[paramID] = []byte{} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Track retained buffer capacity before appending.
Line 148 charges only len(data). Line 149 can grow the slice backing array to a capacity greater than its length. The connection can therefore retain more long-data memory than the session tracker charges.
Lines 115-120 and 172-179 also release accepted lengths. Track the reserved capacity for each parameter and release that same reservation. Reserve memory before changing boundParams. Add a multi-chunk test that forces slice growth.
Also applies to: 148-150, 172-179
🤖 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/server/driver_tidb.go` around lines 115 - 120, Update the long-data
handling around boundParams and its release paths to track each parameter’s
reserved slice capacity, not only len(data). Reserve and charge the capacity
before appending or replacing boundParams, store the reservation per parameter,
and release that exact reservation in the cleanup paths at the current release
blocks. Add a multi-chunk test that forces backing-slice growth and verifies
session memory tracking remains balanced.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-7.5-20260724-v7.5.7 #70442 +/- ##
================================================================
Coverage ? 73.0208%
================================================================
Files ? 1445
Lines ? 421859
Branches ? 0
================================================================
Hits ? 308045
Misses ? 94534
Partials ? 19280
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
…independent The new TestStmtSendLongDataMemQuotaQuery uses testkit.CreateMockStoreAndDomain, which calls session.DisableStats4Test and leaves the process-global stats lease at -1. TestInitStatsSessionBlockGC then no longer blocks GC as expected when the two tests share a test shard. Reset the lease explicitly (adapted from pingcap#67206, which fixed this on master).
|
/retest |
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djshow832, YangKeao 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
added
lgtm
and removed
needs-1-more-lgtm
4964d33
into
pingcap:release-7.5-20260724-v7.5.7
What problem does this PR solve?
Issue Number: close #70349
Problem Summary:
Cherry-pick #70350 (limit connection-scoped
COM_STMT_SEND_LONG_DATAmemory usage) torelease-7.5-20260724-v7.5.7.What changed and how does it work?
Cherry-pick of bff1263. Conflict adaptations for release-7.5:
GetRowContainerReader()while adding the new long-data release call to statement reset/close.ErrMemoryExceedForQueryerror introduced by *: unified the cancel/kill logic in tidb #47063, so the quota-exceeded path returns the equivalent release-7.5 single-query memory-limit message frompkg/util/memory.The prepared-statement long-data buffers are charged to the session memory tracker, rejected before the existing query-memory quota is reached, and uncharged when the statement is executed, reset, or closed.
Check List
Tests
Local validation:
The focused tests pass. With local Go 1.25.8,
go vet ./pkg/server/additionally reports two pre-existingappend with no valuesdiagnostics inconn_stmt_test.go; both calls are already present in the target branch and are outside this patch.Side effects
Documentation
Release note
Summary by CodeRabbit