store/copr: add RC byte-budget paging framework#67504
Draft
JmPotato wants to merge 9 commits intopingcap:masterfrom
Draft
store/copr: add RC byte-budget paging framework#67504JmPotato wants to merge 9 commits intopingcap:masterfrom
JmPotato wants to merge 9 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
bot
added
the
size/XXL
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughAdds Resource-Control (RC) byte-budget paging for TiKV DAG coprocessor requests, propagates a per-page byte budget through DistSQL/session/sysvar plumbing and cop task construction, introduces paging-size constants (256 KB–4 MB), updates go module pins and Bazel deps, adds tests and adjusts test sharding/deps. Changes
Sequence Diagram(s)sequenceDiagram
participant Planner as "BuildCopIterator\n(Planner)"
participant Builder as "buildCopTaskOpt\n/ copTask"
participant TiKV as "TiKV\n(Coprocessor)"
participant ResourceCtrl as "Resource Control\n(config)"
rect rgba(200,230,201,0.5)
Planner->>ResourceCtrl: check RC enabled & resource group
ResourceCtrl-->>Planner: RC enabled? (yes/no)
Planner->>Planner: rcPagingEligible(req)?
alt eligible
Planner->>Builder: set paging=true,\nrcPagingSizeBytes, min/max
Builder->>TiKV: send Request(Paging=true,\nPagingSizeBytes=rcPagingSizeBytes)
TiKV-->>Builder: paged response chunk
Builder-->>Planner: assemble remains task\n(propagate pagingSizeBytes)
else not eligible
Planner->>Builder: normal request (no RC paging)
Builder->>TiKV: send Request(Paging=false)
TiKV-->>Builder: full/non-paged response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
JmPotato
force-pushed
the
demo/ru-paging-size
branch
from
1550f30 to
a2c0781
Compare
![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 (2)
go.mod (1)
371-374: Document the temporary nature and release plan for forked dependencies.Since this PR implements a coordinated multi-repository feature (RC byte-budget paging), consider adding comments in go.mod to:
- Explain why these forks are necessary (awaiting upstream releases with paging_size_bytes support)
- Reference the corresponding upstream PRs/issues
- Document the plan for transitioning to official releases
This will help future maintainers understand the dependency strategy and track when these can be replaced.
📝 Proposed documentation addition
replace ( // Downgrade grpc to v1.63.2, as well as other related modules. github.com/apache/arrow-go/v18 => github.com/joechenrh/arrow-go/v18 v18.0.0-20250911101656-62c34c9a3b82 github.com/go-ldap/ldap/v3 => github.com/YangKeao/ldap/v3 v3.4.5-0.20230421065457-369a3bab1117 + // TODO: RC byte-budget paging feature - replace with official releases once available + // Tracking: pingcap/kvproto#1448 and related upstream changes in PD, client-go, TiKV github.com/pingcap/kvproto => github.com/JmPotato/kvproto v0.0.0-20260401045836-48e3f28d9c74 github.com/pingcap/tidb/pkg/parser => ./pkg/parser github.com/tikv/client-go/v2 => github.com/JmPotato/client-go/v2 v2.0.0-20260402023154-0eedbbb67070 github.com/tikv/pd/client => github.com/JmPotato/pd/client v0.0.0-20260402021839-b98ff48814c2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 371 - 374, Add explanatory comments in go.mod next to the replaced module lines (the github.com/pingcap/kvproto, github.com/tikv/client-go/v2, and github.com/tikv/pd/client replacements) stating these are temporary forks to obtain paging_size_bytes support, reference the upstream PRs or issue URLs that add paging_size_bytes, and include a brief transition plan (e.g., watch upstream PRs, replace forks once upstream releases include the feature and update require/replace lines). Ensure comments mention the intended owner/reviewer and a target to remove the forks once official releases are available.pkg/store/copr/coprocessor_test.go (1)
733-778: Exercise the RC wiring throughBuildCopIterator.This test seeds
rcPagingSizeBytesdirectly intobuildCopTasks, so it won't catch regressions in the actual feature wiring insideBuildCopIterator—especially the paging-vs-store-batch interaction. Please add a targeted case with RC enabled andStoreBatchSize > 0that asserts batching is shut off whilepagingSizeBytesstill propagates.Based on learnings: Applies to pkg/store/|pkg/kv/ : For storage behavior changes, perform targeted unit tests; use realtikv tests if behavior depends on real TiKV.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/store/copr/coprocessor_test.go` around lines 733 - 778, Add a test that exercises the RC wiring via BuildCopIterator instead of seeding rcPagingSizeBytes into buildCopTasks: create a BuildCopIterator (or call BuildCopIterator through the same code-path used by production) with RC enabled, set req.Paging as in the existing test, set StoreBatchSize > 0 on the iterator options, and set rcPagingSizeBytes to paging.MaxPagingSizeBytes; assert that the resulting iterator disables store batching (StoreBatchSize behavior or equivalent flag is off) while the iterator or its underlying cop-task still has paging == true and pagingSizeBytes == paging.MaxPagingSizeBytes; use the same helpers (e.g., buildCopRanges, NewRegionCache/pd client bootstrapping) and mirror assertions from TestBuildCopTasksWithRCPagingSizeBytes so regressions in the BuildCopIterator wiring are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 371-374: go.mod currently redirects core modules
(github.com/pingcap/kvproto, github.com/tikv/client-go/v2,
github.com/tikv/pd/client) to a personal JmPotato fork which mismatches versions
in DEPS.bzl; to fix, restore these replace directives to the official upstream
module versions (or remove them), then regenerate DEPS.bzl by running make
bazel_prepare so Bazel and Go use the same versions; if the fork was
intentionally used for testing, document that fact in the PR and ensure you
coordinate upstream releases before merging so the temporary forked replaces are
removed prior to production.
In `@pkg/store/copr/coprocessor.go`:
- Around line 142-156: The code checks checkStoreBatchCopr(req) before
rcPagingEligible(req) can flip req.Paging.Enable, allowing RC-eligible DAG
requests to incorrectly keep StoreBatchSize > 0; after rcPagingEligible(req)
forces paging on, re-run the store-batch gate and clear batching when paging was
enabled (i.e., call checkStoreBatchCopr(req) or explicitly set
req.StoreBatchSize = 0 if req.Paging.Enable was turned on by rcPagingEligible)
so newBatchTaskBuilder never receives a request that has both paging forced by
RC and a positive StoreBatchSize; update the logic around checkStoreBatchCopr,
rcPagingEligible, req.Paging.Enable, and StoreBatchSize accordingly.
---
Nitpick comments:
In `@go.mod`:
- Around line 371-374: Add explanatory comments in go.mod next to the replaced
module lines (the github.com/pingcap/kvproto, github.com/tikv/client-go/v2, and
github.com/tikv/pd/client replacements) stating these are temporary forks to
obtain paging_size_bytes support, reference the upstream PRs or issue URLs that
add paging_size_bytes, and include a brief transition plan (e.g., watch upstream
PRs, replace forks once upstream releases include the feature and update
require/replace lines). Ensure comments mention the intended owner/reviewer and
a target to remove the forks once official releases are available.
In `@pkg/store/copr/coprocessor_test.go`:
- Around line 733-778: Add a test that exercises the RC wiring via
BuildCopIterator instead of seeding rcPagingSizeBytes into buildCopTasks: create
a BuildCopIterator (or call BuildCopIterator through the same code-path used by
production) with RC enabled, set req.Paging as in the existing test, set
StoreBatchSize > 0 on the iterator options, and set rcPagingSizeBytes to
paging.MaxPagingSizeBytes; assert that the resulting iterator disables store
batching (StoreBatchSize behavior or equivalent flag is off) while the iterator
or its underlying cop-task still has paging == true and pagingSizeBytes ==
paging.MaxPagingSizeBytes; use the same helpers (e.g., buildCopRanges,
NewRegionCache/pd client bootstrapping) and mirror assertions from
TestBuildCopTasksWithRCPagingSizeBytes so regressions in the BuildCopIterator
wiring are caught.
🪄 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: 99dd612b-2680-493e-8272-6b0fb21125db
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.modpkg/store/copr/coprocessor.gopkg/store/copr/coprocessor_test.gopkg/util/paging/paging.go
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
JmPotato
force-pushed
the
demo/ru-paging-size
branch
from
a2c0781 to
7a480de
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/sessionctx/variable/sysvar.go (1)
2973-2974: UsetidbOptPositiveInt32to align with the pattern forMinPagingSizeandMaxPagingSize.Line 2974 uses
TidbOptInt64with an explicitint()cast, while the similar paging-size sysvars on lines 2966 and 2970 usetidbOptPositiveInt32withstrconv.Atoi. For consistency and clarity, apply the same safe helper:Suggested fix
{Scope: vardef.ScopeGlobal | vardef.ScopeSession, Name: vardef.TiDBRCPagingSizeBytes, Value: strconv.Itoa(vardef.DefRCPagingSizeBytes), Type: vardef.TypeUnsigned, MinValue: 0, MaxValue: math.MaxInt64, SetSession: func(s *SessionVars, val string) error { - s.RCPagingSizeBytes = int(TidbOptInt64(val, int64(vardef.DefRCPagingSizeBytes))) + s.RCPagingSizeBytes = tidbOptPositiveInt32(val, vardef.DefRCPagingSizeBytes) return nil }},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sessionctx/variable/sysvar.go` around lines 2973 - 2974, The TiDBRCPagingSizeBytes sysvar SetSession currently uses TidbOptInt64 with an int(...) cast; replace that with the same pattern used for MinPagingSize/MaxPagingSize by using tidbOptPositiveInt32 together with strconv.Atoi to parse/validate the value and assign to s.RCPagingSizeBytes, ensuring the helper enforces positivity and 32-bit range; update the SetSession closure for the TiDBRCPagingSizeBytes entry accordingly (referencing TiDBRCPagingSizeBytes, SetSession, s.RCPagingSizeBytes, tidbOptPositiveInt32, and strconv.Atoi).
🤖 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/session/session.go`:
- Line 3378: Add SQL integration tests that exercise the session variable
tidb_rc_paging_size_bytes end-to-end: write tests that SET and SET GLOBAL the
variable, verify scoping (session vs global), and assert that setting it to 0
disables byte-budget paging in actual queries; mirror the structure and
assertions used in tests/integrationtest/t/sessionctx/setvar.test and cover the
behavior covered by TestBuildCopTasksWithRCPagingSizeBytes so DistSQL requests
pick up RCPagingSizeBytes from the session (the code path that reads
RCPagingSizeBytes in session.go should be exercised by these integration tests).
---
Nitpick comments:
In `@pkg/sessionctx/variable/sysvar.go`:
- Around line 2973-2974: The TiDBRCPagingSizeBytes sysvar SetSession currently
uses TidbOptInt64 with an int(...) cast; replace that with the same pattern used
for MinPagingSize/MaxPagingSize by using tidbOptPositiveInt32 together with
strconv.Atoi to parse/validate the value and assign to s.RCPagingSizeBytes,
ensuring the helper enforces positivity and 32-bit range; update the SetSession
closure for the TiDBRCPagingSizeBytes entry accordingly (referencing
TiDBRCPagingSizeBytes, SetSession, s.RCPagingSizeBytes, tidbOptPositiveInt32,
and strconv.Atoi).
🪄 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: 63954ddf-f0ee-4cee-b1a7-41c3f9ead908
📒 Files selected for processing (8)
pkg/distsql/context/context.gopkg/distsql/request_builder.gopkg/kv/kv.gopkg/session/session.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.gopkg/store/copr/coprocessor.go
pkg/session/session.go
Outdated
| EnablePaging: vars.EnablePaging, | ||
| MinPagingSize: vars.MinPagingSize, | ||
| MaxPagingSize: vars.MaxPagingSize, | ||
| RCPagingSizeBytes: vars.RCPagingSizeBytes, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify existing test coverage for the new RC paging byte-budget variable/path.
rg -n --glob '*_test.go' 'tidb_rc_paging_size_bytes|RCPagingSizeBytes|paging_size_bytes'
# Check for integration-style SQL test coverage references (if present in repo).
rg -n --glob '*.test' --glob '*.result' 'tidb_rc_paging_size_bytes|paging_size_bytes'Repository: pingcap/tidb
Length of output: 154
🏁 Script executed:
# Inspect the existing test to see what it covers
head -50 pkg/store/copr/coprocessor_test.go | tail -30
sed -n '733,800p' pkg/store/copr/coprocessor_test.go
# Search for RCPagingSizeBytes in session-related test files
rg -l 'RCPagingSizeBytes' pkg/session --glob '*_test.go'
# Check for integration test patterns (testdata, sqllogictests, etc.)
fd -type f -path '*/testdata*' -name '*paging*' -o -path '*/testdata*' -name '*rc*' | head -20
# Broader search for integration/SQL tests mentioning this variable
rg 'tidb_rc_paging' --glob '*.test' --glob '*.sql' --glob '*.result' --glob '*integration*'Repository: pingcap/tidb
Length of output: 3376
🏁 Script executed:
# Find the variable definition for tidb_rc_paging_size_bytes
rg -n 'tidb_rc_paging_size_bytes|RCPagingSizeBytes' pkg/sessionctx/variable pkg/sessionctx/vardef -A 3 -B 1
# Check the session.go GetDistSQLCtx function context
sed -n '3350,3400p' pkg/session/session.go
# Search for existing integration/SQL tests in testdata
fd 'testdata' -type d | head -5
ls -la testdata/ 2>/dev/null | head -20 || echo "No testdata in root"
# Look for other RC (request-scoped) variables to see their test patterns
rg -n 'RCMaxOpenConnections|RCWriteLatencyBase' pkg/session -A 2 -B 1 | head -40Repository: pingcap/tidb
Length of output: 5924
🏁 Script executed:
# List integration test directories
find . -type d -name testdata 2>/dev/null | head -5
# Search for .test files that might cover RC features
find . -name '*.test' -o -name '*.sql' 2>/dev/null | grep -i 'rc\|paging\|resource' | head -10
# Check if session variable tests exist for similar variables
rg -l 'SET tidb_' tests/ testdata/ --glob '*.test' 2>/dev/null | head -5
# Simpler: look for any test file mentioning variable changes
rg 'SET tidb_rc_paging_size_bytes|SET tidb_max_paging' -type f --max-count 5
# Check nearby session variables to understand test patterns
rg 'TiFlashMaxBytesBeforeExternalJoin|TiFlashMaxQueryMemoryPerNode' pkg/session/session.go -B 2 -A 2Repository: pingcap/tidb
Length of output: 1464
🏁 Script executed:
# Check if RC paging tests exist in integration tests
rg 'rc_paging_size_bytes|RCPagingSizeBytes' tests/integrationtest/t/executor/set.test tests/integrationtest/t/sessionctx/setvar.test
# Check what's in setvar.test to understand test pattern
head -50 tests/integrationtest/t/sessionctx/setvar.test
# Search broader for any RC variable testing
rg 'tidb_rc' tests/integrationtest/ --glob '*.test' | head -20Repository: pingcap/tidb
Length of output: 2292
Add SQL integration tests for the new session variable behavior.
Line 3378 wires tidb_rc_paging_size_bytes (a user-facing SESSION/GLOBAL variable) into DistSQL request behavior. While unit test coverage exists at the coprocessor level (TestBuildCopTasksWithRCPagingSizeBytes), SQL integration tests should validate the end-to-end variable behavior, including SET scoping and 0 disabling byte-budget paging (similar to the pattern in tests/integrationtest/t/sessionctx/setvar.test).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/session/session.go` at line 3378, Add SQL integration tests that exercise
the session variable tidb_rc_paging_size_bytes end-to-end: write tests that SET
and SET GLOBAL the variable, verify scoping (session vs global), and assert that
setting it to 0 disables byte-budget paging in actual queries; mirror the
structure and assertions used in tests/integrationtest/t/sessionctx/setvar.test
and cover the behavior covered by TestBuildCopTasksWithRCPagingSizeBytes so
DistSQL requests pick up RCPagingSizeBytes from the session (the code path that
reads RCPagingSizeBytes in session.go should be exercised by these integration
tests).
Member
Author
There was a problem hiding this comment.
Kept this update scoped to the merge-triggered conflict plus the RC paging/store-batch regression. The new BuildCopIterator coverage exercises the production paging wiring directly; the broader SQL SET/SET GLOBAL integration surface is not changed in this merge commit.
There was a problem hiding this comment.
@JmPotato, understood — keeping the PR focused on the merge conflict resolution and the RC paging/store-batch regression is a reasonable scoping call. The BuildCopIterator unit test does exercise the production wiring path directly.
Would you like me to open a follow-up issue to track adding the end-to-end SQL integration tests (SET/SET GLOBAL scoping + 0-disables-paging assertions) for tidb_rc_paging_size_bytes once the upstream dependencies land?
🧠 Learnings used
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T20:19:06.925Z
Learning: Applies to pkg/session/**|pkg/sessionctx/** : For session variables or protocol behavior changes, perform targeted package tests plus SQL integration tests for user-visible behavior.
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T12:48:57.065Z
Learning: Applies to pkg/executor/** : For SQL behavior changes in executor, perform targeted unit test plus relevant integration test.
Learnt from: zimulala
Repo: pingcap/tidb PR: 67265
File: pkg/util/topsql/reporter/ru_datamodel_test.go:259-308
Timestamp: 2026-03-25T03:46:10.574Z
Learning: In `pkg/util/topsql/reporter/ru_datamodel_test.go` (pingcap/tidb PR `#67265`), `TestRUCollectingOthersWireLabelNoCollisionWithRuntimeUserShape` intentionally uses `"app127.0.0.1"` (not `othersUserWireLabel`) as the runtime user. The regression is scoped to runtime user shapes (`userhost` / empty string); broadening to arbitrary raw user strings matching the wire label is out of contract. The empty-user and merge-path regressions (`TestRUCollectingEmptyUserAndGlobalOthersRemainDistinct`, `TestRUCollectingMergeFromKeepsEmptyUserDistinctFromGlobalOthers`) plus aggregator-side assertion hardening collectively cover the fix.
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T12:05:28.866Z
Learning: Applies to pkg/store/**|pkg/kv/** : For storage behavior changes, perform targeted unit tests; use realtikv tests if behavior depends on real TiKV.
Learnt from: D3Hunter
Repo: pingcap/tidb PR: 67543
File: pkg/util/traceevent/test/integration_test.go:336-349
Timestamp: 2026-04-03T05:25:43.110Z
Learning: In `pkg/util/traceevent/test/integration_test.go` (pingcap/tidb PR `#67543`), the sampling integration test uses `require.Len(t, matchedEvents, 2)` intentionally. The flakiness was fixed not by relaxing the assertion but by adding trace-ID filtering via `eventsBelongToAnyTrace`: the drain loop only counts event batches belonging to the 10 known trace IDs from the test run, so with `sampling=5` over 10 executions exactly 2 hits are deterministic. Do NOT suggest relaxing this to `require.NotEmpty` or `require.GreaterOrEqual` — the exact count is correct and intentional.
Learnt from: joechenrh
Repo: pingcap/tidb PR: 66878
File: tests/realtikvtest/importintotest/import_into_test.go:253-267
Timestamp: 2026-03-11T06:26:14.065Z
Learning: In pingcap/tidb, PR `#66878` (cherry-pick of `#58401` to release-7.5) also bundles the follow-up nil-check fix from PR `#63146`. `TestOnUpdateColumn` in `tests/realtikvtest/importintotest/import_into_test.go` was cherry-picked from PR `#63146` and tests that `IMPORT INTO` on a table with an `ON UPDATE CURRENT_TIMESTAMP` column does NOT panic due to a nil `onDup` map in `resolveGeneratedColumns`. It is NOT intended to cover the transitive generated-column / index-inconsistency fix from `#58401/`#58400.
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-20T13:18:29.186Z
Learning: Applies to **/*_test.go,**/testdata/** : Follow `.agents/skills/tidb-test-guidelines` for placement, naming, `shard_count` guidance, planner-specific casetest rules, and related testdata conventions.
Learnt from: ChangRui-Ryan
Repo: pingcap/tidb PR: 67145
File: pkg/planner/core/casetest/tici/tici_test.go:470-487
Timestamp: 2026-03-24T10:04:49.982Z
Learning: In `pkg/planner/core/casetest/tici/tici_test.go` (pingcap/tidb PR `#67145`), TiCI FTS tests (e.g., TestTiCINonCoveringUsesDoubleRead, TestTiCIJoinNoMPPPlan, TestTiCIWithTiFlashReplicaNoMPPNoTiFlashCop) use positive assertions for `mpp[tiflash]` and do NOT include negative assertions for `mpp[tiflash]` or `ExchangeSender`. This is intentional: TiCI is MPP-only by design, so all TiCI-side plans are expected to include `mpp[tiflash]`. Do not suggest adding `CheckNotContain("mpp[tiflash]")` or `CheckNotContain("ExchangeSender")` to these TiCI test cases.
Learnt from: terry1purcell
Repo: pingcap/tidb PR: 66695
File: pkg/planner/cardinality/BUILD.bazel:62-66
Timestamp: 2026-03-13T01:48:14.212Z
Learning: In pingcap/tidb cherry-pick PRs, unresolved merge conflicts (e.g., conflict markers in BUILD.bazel or other files) are expected artifacts of the cherry-pick process and are already tracked by the ti-chi-bot hold mechanism. Do NOT flag merge conflicts as review issues — they are not a reason to block or comment on the code review.
Learnt from: disksing
Repo: pingcap/tidb PR: 67320
File: go.mod:127-127
Timestamp: 2026-03-26T15:32:12.803Z
Learning: In pingcap/tidb (PR `#67320`), the divergence between TiDB's `DefaultRUV2Config().RUScale` (1.34, in `pkg/config/config.go`) and the TiKV client-go `DefaultRUV2TiKVConfig().RUScale` (1.40, from tikv/client-go commit 728676760deb) is intentional by design. The two scaling factors apply to different components (TiDB and TiKV respectively) and are not required to match. Do not flag this divergence as a bug or misconfiguration.
Learnt from: joechenrh
Repo: pingcap/tidb PR: 66878
File: pkg/planner/core/logical_plan_builder.go:6455-6472
Timestamp: 2026-03-11T06:29:00.122Z
Learning: Ensure code reviews verify the VirtualAssignmentsOffset semantics: the planner sets Update.VirtualAssignmentsOffset = len(update.List). The executor should only apply OrderedList[:VirtualAssignmentsOffset] when composing new rows, and only after the 'changed' check should it group/evaluate OrderedList[VirtualAssignmentsOffset:] per table. This pattern applies to files under pkg/planner/core and pkg/executor (e.g., common_plans.go and update.go). Reviewers should check that updates respect slicing behavior, that the offset is consistently derived from the planner, and that downstream code does not bypass the offset when creating new rows. Add tests validating both branches: the slice before the offset for new rows, and the per-table handling of the slice after the offset.
2dfee7a to
2cc25e2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
DEPS.bzl (1)
8221-8246: Ensure fork references in go.mod are reverted once upstream PRs merge.Both
github.com/tikv/client-go/v2andgithub.com/tikv/pd/clientare currently aliased to JmPotato's forks via go.mod replace directives (lines 373-374) to support the two-phase RU pre-charge/settlement feature. When the corresponding upstream PRs are merged (client-go tikv#1936, PD tikv#10548), remove these replace directives from go.mod. DEPS.bzl will be automatically regenerated on the nextmake bazel_preparerun.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DEPS.bzl` around lines 8221 - 8246, Remove the temporary go.mod replace directives that point github.com/tikv/client-go/v2 and github.com/tikv/pd/client to the JmPotato forks once the upstream PRs (tikv#1936 and tikv#10548) are merged: delete the replace entries for those module paths in go.mod and then run make bazel_prepare so DEPS.bzl (which currently contains the forked strip_prefix/urls for com_github_jmpotato_client_go_v2 and com_github_jmpotato_pd_client) is regenerated to reference upstream modules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@DEPS.bzl`:
- Around line 8221-8246: Remove the temporary go.mod replace directives that
point github.com/tikv/client-go/v2 and github.com/tikv/pd/client to the JmPotato
forks once the upstream PRs (tikv#1936 and tikv#10548) are merged: delete the
replace entries for those module paths in go.mod and then run make bazel_prepare
so DEPS.bzl (which currently contains the forked strip_prefix/urls for
com_github_jmpotato_client_go_v2 and com_github_jmpotato_pd_client) is
regenerated to reference upstream modules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a5b90f6f-6068-472f-9441-002aaa7e9cbb
📒 Files selected for processing (2)
DEPS.bzlpkg/store/copr/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/store/copr/BUILD.bazel
|
@JmPotato: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@JmPotato: The following tests failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
ti-chi-bot
bot
added
the
do-not-merge/work-in-progress
JmPotato
force-pushed
the
demo/ru-paging-size
branch
from
f859773 to
ab9d412
Compare
When Resource Control is enabled, force-enable paging for eligible coprocessor requests and set a byte budget (paging_size_bytes) per page. This limits the scanned bytes of each page so that RU cost is bounded and pre-charged in Phase 1, preventing concurrent workers from all hitting Phase 2 simultaneously. Signed-off-by: JmPotato <github@ipotato.me>
Signed-off-by: JmPotato <github@ipotato.me>
…ation Test rcPagingEligible conditions (RC enabled, resource group, TiKV, DAG) and verify pagingSizeBytes is correctly propagated to copTasks and cleared when small limit disables paging. Signed-off-by: JmPotato <github@ipotato.me>
Signed-off-by: JmPotato <github@ipotato.me>
Add a new session variable `tidb_rc_paging_size_bytes` to allow dynamically controlling the byte budget per page for RC paging via SQL. - Default: 4MB (paging.MaxPagingSizeBytes) - Set to 0 to disable byte-budget paging - Supports both GLOBAL and SESSION scope Usage: SET @@tidb_rc_paging_size_bytes = 4194304; -- 4MB (default) SET @@tidb_rc_paging_size_bytes = 1048576; -- 1MB SET @@tidb_rc_paging_size_bytes = 0; -- disable Signed-off-by: JmPotato <github@ipotato.me>
Signed-off-by: JmPotato <github@ipotato.me>
Increase the bucket count from 10 (max 512KB) to 19 (max 256MB) for the tidb_distsql_copr_resp_size histogram to improve percentile accuracy when observing large coprocessor responses during paging size tuning. Signed-off-by: JmPotato <github@ipotato.me>
JmPotato
force-pushed
the
demo/ru-paging-size
branch
from
ab9d412 to
c703888
Compare
- Rename all RC-prefixed identifiers to generic names (e.g. RCPagingSizeBytes → PagingSizeBytes, rcPagingEligible → pagingBytesEligible) since byte-budget paging is general infrastructure, not RC-specific. - Remove RC gates (EnableResourceControl, ResourceGroupName) from pagingBytesEligible; only TiKV + DAG checks remain. - Remove unused MinPagingSizeBytes/MaxPagingSizeBytes constants. - Change DefPagingSizeBytes default from 4MB to 0 (disabled, opt-in). - Move byte-budget paging force-enable before checkStoreBatchCopr to preserve the paging-disables-batch-copr invariant. - Add tidb_paging_size_bytes to SET_VAR hint allowlist. - Fix copr_resp_size histogram: use float division to preserve sub-KiB precision, extend buckets to 256MB, correct help text unit to KiB. - Fix anonymous Paging struct literal in copr_test to include the new PagingSizeBytes field. - Add PagingSizeBytes to TestContextDetach fixture. - Add integration test for tidb_paging_size_bytes session variable. Signed-off-by: JmPotato <github@ipotato.me>
Update the PD client replace directive to pick up commit fda21466b565 which adds Limiter.RefundTokens and fixes the pre-charge token leakage in paging byte budget scenarios. Signed-off-by: JmPotato <github@ipotato.me>
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found 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
1 participant
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.
What problem does this PR solve?
Issue Number: ref pingcap/kvproto#1448
Problem Summary:
In concurrent coprocessor workloads under Resource Control, row-count-based paging alone cannot prevent "token cliff" effects where multiple workers exhaust RU budgets simultaneously. This PR introduces byte-budget paging to bound per-page resource consumption by byte size (4 MB), enabling PD's resource controller to pre-charge RU more accurately in Phase 1.
What changed and how does it work?
RC Paging Eligibility (
rcPagingEligible):Paging Injection (
BuildCopIterator):rcPagingSizeBytes = 4MBcopTask→handleTaskOnce→ coprocessor RPC (PagingSizeBytesfield)pagingSizeBytesacross region/lock errorsNew Constants (
paging.go):MinPagingSizeBytes = 256 KB,MaxPagingSizeBytes = 4 MBDependency Updates (
go.mod):JmPotato/kvproto(addspaging_size_bytesproto field)JmPotato/client-go(extractsPagingSizeBytesfromRequestInfo)JmPotato/pd(two-phase RU pre-charge/settlement)Check List
Tests
Side effects
Documentation
Related PRs
Release note
Summary by CodeRabbit
New Features
Tests
Chores