Skip to content

store/copr: refine RC paging pre-charge with per-scan EMA#67759

Draft
YuhaoZhang00 wants to merge 11 commits intopingcap:masterfrom
YuhaoZhang00:demo/ema-precharge
Draft

store/copr: refine RC paging pre-charge with per-scan EMA#67759
YuhaoZhang00 wants to merge 11 commits intopingcap:masterfrom
YuhaoZhang00:demo/ema-precharge

Conversation

@YuhaoZhang00
Copy link
Copy Markdown

What problem does this PR solve?

Issue Number: ref #N/A (draft — issue to be filed)

Problem Summary:

Under Resource Control, TiDB pre-charges RUs for each paging coprocessor
RPC using a fixed paging_size_bytes (currently 4 MiB, set by #67504). This
worst-case charge typically overshoots the actual scanned bytes by a large
margin, and under a tight resource tier it stalls concurrent workers at
Phase 1 (token acquisition) even when the cluster has spare capacity. The
result is that large paging_size_bytes values collapse QPS on workloads
like a 1500-thread JOIN benchmark, while small values (256 KiB) look fine.

What changed and how does it work?

Maintain a time-aware EMA of the actual per-RPC MVCC read bytes on
copTask, and once enough samples are in (>= 2), pass the prediction to
PD via the new tikvrpc.Request.PredictedReadBytes hint so the pre-charge
uses the learned estimate instead of the 4 MiB cap. On cold start and for
non-paging tasks the behavior is unchanged (falls back to PagingSizeBytes).

The EMA state lives on copTask and is naturally scoped to one logical scan
across its paging rounds — no cross-RPC or cross-process sync. The
prediction is propagated across region/lock retries so learning is not lost.

Stacked with:

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

Draft note: E2E validation on the simulation cluster is pending. Once
the client-go + PD companion PRs land (or are pointed to via go.mod replace
for CI), this PR's CI will turn green. This is a draft for early visibility
and discussion.

JmPotato and others added 10 commits April 7, 2026 17:35
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>
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>
- 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>
Maintain a time-aware EMA of actual per-RPC MVCC read bytes on copTask
and, once enough samples are in, pass the prediction to PD via the new
tikvrpc.Request.PredictedReadBytes hint so pre-charge uses the learned
estimate instead of the 4 MiB worst-case cap. Falls back to
PagingSizeBytes on cold start. Propagated across region/lock retries;
no-op for non-paging tasks.

Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 14, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot bot commented Apr 14, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cfzjywxk, terry1purcell, xuhuaiyu for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tiprow
Copy link
Copy Markdown

tiprow bot commented Apr 14, 2026

Hi @YuhaoZhang00. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

Instructions 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.

@ti-chi-bot ti-chi-bot bot added needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. contribution This PR is from a community contributor. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 14, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot bot commented Apr 14, 2026

Hi @YuhaoZhang00. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 14, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ca52f1e2-86c7-4b0c-af02-94c8d483114d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Adds tidb_distsql_copr_ema_observation_total with a {state=cold|ready}
label to quantify, in aggregate, how many per-logical-scan copTask EMA
Observe() calls happen while the EMA is still below its readiness
threshold vs after. This complements the PD-side paging pre-charge
source/bytes counters by showing how often the TiDB side could emit a
non-zero PredictedReadBytes hint for downstream pre-charge.

Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot bot commented Apr 14, 2026

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-linked-issue label, please provide the linked issue number on one line in the PR body, for example: Issue Number: close #123 or Issue Number: ref #456.

📖 For more info, you can check the "Contribute Code" section in the development guide.

@YuhaoZhang00
Copy link
Copy Markdown
Author

Added one observability metric on the TiDB side for upcoming e2e validation of the per-logical-scan EMA path:

  • tidb_distsql_copr_ema_observation_total{type=cold|ready} — counts each copTask.ema.Observe call, labelled by whether the EMA was already at its readiness threshold before the observation (i.e. whether the corresponding paging pre-charge on the PD side went through the predicted-hint path or the cold fallback).

Paired with the 4 PD-side paging pre-charge metrics (see tikv/pd#10599 comment), this lets the validation report show the cold-start window quantitatively rather than inferring it from QPS alone. Draft status unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution This PR is from a community contributor. do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants