Skip to content

Storages: Fix the statistics of user_read_bytes and add metrics#10396

Merged
ti-chi-bot[bot] merged 9 commits intopingcap:masterfrom
JinheLin:fix_concat_read_bytes
Sep 4, 2025
Merged

Storages: Fix the statistics of user_read_bytes and add metrics#10396
ti-chi-bot[bot] merged 9 commits intopingcap:masterfrom
JinheLin:fix_concat_read_bytes

Conversation

@JinheLin
Copy link
Copy Markdown
Contributor

@JinheLin JinheLin commented Sep 3, 2025 •
edited by JaySon-Huang
Loading

What problem does this PR solve?

Issue Number: ref #10380

  • ConcatSkippableBlockInputStream
    • When generating MVCC bitmaps using DeltaIndex, row_id is generated in DeltaMergeBlockInputStream. The getPlacedStream method sets the need_row_id parameter to false when obtaining the stable stream. Therefore, in ConcatSkippableBlockInputStream, need_row_id cannot be used to determine whether MVCC is involved.
    • This can be resolved by switching to a check based on read_tag.
  • DeltaValueReader
    • When we use getInputStreamNormal to read data, delta data is read through DeltaValueReader. Thus, DeltaValueReader needs to add statistics for user_read_bytes.
  • VersionChain
    • Track the read bytes consumed during the construction of MVCC bitmaps.
  • ScanContext
    • Encapsulate the reporting of primary user_read_bytes and metrics within ScanContext.
  • Mark the read operations for updating DeltaIndex and VersionChain as ReadTag::Internal.
  • TiFlashMetrics
    • Add storage-layer RU (Resource Usage) monitoring.
image image

What is changed and how it works?

Check List

Tests

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

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

@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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 3, 2025
@JinheLin JinheLin changed the title WIP Storages: Fix the statistics of user_read_bytes and add metrics Sep 3, 2025
@JinheLin JinheLin requested review from JaySon-Huang, Lloyd-Pottiger and guo-shaoge and removed request for guo-shaoge September 3, 2025 10:41
@ti-chi-bot ti-chi-bot Bot added needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. and removed do-not-merge/needs-linked-issue labels Sep 3, 2025
@ti-chi-bot ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Sep 4, 2025
@JinheLin
Copy link
Copy Markdown
Contributor Author

JinheLin commented Sep 4, 2025

/retest

JaySon-Huang
JaySon-Huang reviewed Sep 4, 2025
View reviewed changes
Comment thread dbms/src/Storages/DeltaMerge/Segment.cpp
Comment thread dbms/src/Common/TiFlashMetrics.h
Comment thread dbms/src/Storages/DeltaMerge/ScanContext.cpp Outdated
{
if (read_tag != ReadTag::Query && read_tag != ReadTag::LMFilter && read_tag != ReadTag::MVCC)
return;
user_read_bytes += bytes;
Copy link
Copy Markdown
Contributor

@JaySon-Huang JaySon-Huang Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now user_read_byte contains the Query, LMFilter and MVCC. Maybe split the number of MVCC bytes to be a standalone variable so that we can tell how many bytes are read for MVCC and how many bytes are for Query+LMFilter?

Copy link
Copy Markdown
Contributor Author

@JinheLin JinheLin Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split user_read_bytes into query_read_bytes and mvcc_read_bytes.

user_read_bytes in tipb equals mvcc_read_bytes plus query_read_bytes. Subsequent changes will be needed for both tipb and tidb.

@ti-chi-bot ti-chi-bot Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2025
@JinheLin
Copy link
Copy Markdown
Contributor Author

JinheLin commented Sep 4, 2025

/retest

JaySon-Huang
JaySon-Huang approved these changes Sep 4, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@JaySon-Huang JaySon-Huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ti-chi-bot
Copy link
Copy Markdown
Contributor

ti-chi-bot Bot commented Sep 4, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang, Lloyd-Pottiger

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [JaySon-Huang,Lloyd-Pottiger]

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

@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 4, 2025
@ti-chi-bot
Copy link
Copy Markdown
Contributor

ti-chi-bot Bot commented Sep 4, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-09-04 01:49:01.156001985 +0000 UTC m=+58085.139099423: ☑️ agreed by Lloyd-Pottiger.
  • 2025-09-04 08:15:03.373611727 +0000 UTC m=+81247.356709175: ☑️ agreed by JaySon-Huang.

@JinheLin
Copy link
Copy Markdown
Contributor Author

JinheLin commented Sep 4, 2025

/retest

@ti-chi-bot ti-chi-bot Bot merged commit 2120b05 into pingcap:master Sep 4, 2025
7 checks passed
ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Sep 4, 2025
@JinheLin @ti-chi-bot
This is an automated cherry-pick of pingcap#10396
c9aeef3 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Sep 4, 2025
Merged
12 tasks
@ti-chi-bot
Copy link
Copy Markdown
Member

ti-chi-bot commented Sep 4, 2025

In response to a cherrypick label: new pull request created to branch release-8.5: #10404.
But this PR has conflicts, please resolve them!

ti-chi-bot Bot pushed a commit that referenced this pull request Sep 9, 2025
@ti-chi-bot @JinheLin
Storages: Fix the statistics of user_read_bytes and add metrics (#10396…
7417e50 
…) (#10404)

ref #10380

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

Co-authored-by: jinhelin <linjinhe33@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JaySon-Huang JaySon-Huang JaySon-Huang approved these changes

@Lloyd-Pottiger Lloyd-Pottiger Lloyd-Pottiger approved these changes

@guo-shaoge guo-shaoge Awaiting requested review from guo-shaoge

Assignees

No one assigned

Labels

approved lgtm needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JinheLin @ti-chi-bot @JaySon-Huang @Lloyd-Pottiger