Skip to content

*: cherry-pick statement PRs to release-nextgen-202603#69111

Merged
ti-chi-bot[bot] merged 3 commits into
pingcap:release-nextgen-202603from
nolouch:cherry-pick-statement-prs-release-nextgen-202603
Jun 18, 2026
Merged

*: cherry-pick statement PRs to release-nextgen-202603#69111
ti-chi-bot[bot] merged 3 commits into
pingcap:release-nextgen-202603from
nolouch:cherry-pick-statement-prs-release-nextgen-202603

Conversation

@nolouch

@nolouch nolouch commented Jun 11, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: ref #67924, ref #68865

Problem Summary:

Cherry-pick recent statement-related PRs to release-nextgen-202603:

What changed and how does it work?

This PR cherry-picks the three upstream commits and resolves release-branch conflicts in statement summary logging/metrics without bringing unrelated master-only channelz or keyspace-observability changes into the release branch.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

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

Please refer to Release Notes Language Style Guide to write a quality release note.

Add `tidb_stmt_summary_group_by_user` and `tidb_stmt_summary_persist_evicted`, and fix affected rows after restoring session states.

Summary by CodeRabbit

  • New Features

    • Added tidb_stmt_summary_persist_evicted and tidb_stmt_summary_group_by_user system variables for statement summary configuration.
    • New metrics for monitoring evicted statement records persistence.
    • Statement summaries now support grouping by executing user.
  • Bug Fixes

    • Corrected affected rows restoration in SET SESSION statement context.
  • Tests

    • Added comprehensive coverage for statement summary grouping and eviction persistence features.

@ti-chi-bot ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 11, 2026
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR introduces per-record LRU eviction persistence and user-dimension grouping for TiDB's statement summaries, alongside system variables, metrics, and comprehensive test coverage. Changes span v1 grouping infrastructure, v2 async eviction logging with metrics, session-state affected-rows fixes, and supporting test updates.

Changes

Statement Summary User Grouping and Eviction Persistence

Layer / File(s) Summary
System variables, metrics constants, and metric registration
pkg/sessionctx/vardef/tidb_vars.go, pkg/metrics/stmtsummary.go, pkg/metrics/metrics.go, pkg/sessionctx/variable/sysvar.go
Defines TiDBStmtSummaryPersistEvicted and TiDBStmtSummaryGroupByUser system variables with defaults. Introduces StmtSummaryEvictedLogResultPersisted, StmtSummaryEvictedLogResultDropped constants and StmtSummaryEvictedLogCounter Prometheus metric. Registers the counter in RegisterMetrics.
V1 digest key user dimension
pkg/util/stmtsummary/statement_summary.go
StmtDigestKey.Init now accepts a user parameter and appends length-prefixed user bytes to prevent hash collisions when user-based grouping is enabled. Adds encoding/binary import for length-prefix encoding.
V1 StmtSummaryByDigestMap user grouping
pkg/util/stmtsummary/statement_summary.go
Adds optGroupByUser atomic flag, moves digest-key initialization under map lock to coordinate with grouping state, introduces SetGroupByUser and GroupByUser APIs with atomic toggle and cache clearing, and refactors Clear to lock-aware clearLocked helper.
V1 tests updated for user parameter
pkg/util/stmtsummary/statement_summary_test.go, pkg/util/stmtsummary/evicted_test.go
Updates all StmtDigestKey.Init calls to pass empty string for user parameter. Adds TestAddStatementGroupByUser (verifying grouping toggle and cache clearing) and TestStmtDigestKeyBoundary (validating collision-free encoding and hash equivalence).
V2 eviction persistence infrastructure
pkg/util/stmtsummary/v2/stmtsummary.go
Adds evictedCh buffered channel, onEvict callback wired into statement window, async evictedLogLoop goroutine that batches evicted records to storage, tracks dropped records on channel overflow, and periodically reports drop metrics. Defines constants for buffering and flush intervals.
V2 logger and reader for evicted records
pkg/util/stmtsummary/v2/logger.go, pkg/util/stmtsummary/v2/reader.go, pkg/util/stmtsummary/v2/record_test.go, pkg/util/stmtsummary/v2/reader_test.go
Logger writes evicted records as newline-delimited JSON with "evicted": true marker via logEvicted method and increments StmtSummaryEvictedLogCounter. Reader parses stmtPersistedRecord wrapper, skips evicted records during history reads, and maintains distinction between evicted and aggregated records.
V2 user grouping and complete persistence integration
pkg/util/stmtsummary/v2/stmtsummary.go
Adds user-grouping controls to v2 StmtSummary, coordinates user dimension under windowLock in AddStatement, updates Close() with compare-and-swap guard, reinstalls eviction callback during flush() and rotate(), extends stmtEvicted to track aggregates with and without per-record logging, and updates window eviction hook to invoke callback with logging status flag.
V2 public APIs and v1/v2 proxies
pkg/util/stmtsummary/v2/stmtsummary.go
Adds public PersistEvicted(), SetPersistEvicted(bool), GroupByUser(), SetGroupByUser(bool) methods to v2 StmtSummary. Package-level proxies route toggles to both v1 and v2 backends depending on availability.
V2 comprehensive test coverage
pkg/util/stmtsummary/v2/stmtsummary_test.go
Adds counter-reading helper. New tests: TestStmtSummaryPersistEvicted (async logging with timestamps), TestStmtSummaryPersistEvictedDoesNotPersistLoggedRecordsAsAggregate (evicted ≠ aggregated), TestStmtSummaryGroupByUser (grouping toggle behavior with AuthUsers verification). Adds stmtExecInfoWithUser fixture helper.
Session state and executor affected rows handling
pkg/executor/select.go, pkg/sessionctx/variable/session.go, pkg/sessionctx/sessionstates/session_states_test.go
DecodeSessionStates directly assigns PrevAffectedRows; ResetContextOfStmt separates InSetSessionStatesStmt handling from DML logic; test asserts AffectedRows() is zero after select 1.
Metrics testing
pkg/metrics/metrics_internal_test.go
Adds readCounterValue helper to extract Prometheus counter values. Extends TestStmtSummaryMetricLabels to validate StmtSummaryEvictedLogCounter (zero for V1, persisted/dropped labels and values for V2).
Build configuration
pkg/util/stmtsummary/BUILD.bazel, pkg/util/stmtsummary/v2/BUILD.bazel
Increases stmtsummary_test shard count 26→28, v2/stmtsummary_test shard count 15→18, and adds zap dependencies to v2 test target.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • pingcap/tidb#68868: Overlapping fixes for ResetContextOfStmt and DecodeSessionStates to properly handle PrevAffectedRows in SET SESSION contexts, avoiding incorrect ROW_COUNT restoration.
  • pingcap/tidb#68513: Related PR introducing per-eviction JSON logging to v2 statement logs via logEvicted / evictedStmtRecord, upon which this PR builds additional metric counting and grouping features.
  • pingcap/tidb#68512: Related PR introducing tidb_stmt_summary_group_by_user system variable and user-in-key grouping logic, directly overlapping with this PR's user-dimension digest key and v1/v2 grouping implementation.

Suggested labels

size/XXL, cherry-pick-approved, approved, lgtm

Suggested reviewers

  • yibin87
  • yudongusa
  • terry1purcell

Poem

🐰 A summary now remembers each user's fingerprint,
Evictions logged with care, their stories firmly inked.
Windows rotate, groupings toggle, metrics count drops true—
The statement story ripples on, persistent and askew! 📊✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 48.84% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly indicates this is a cherry-pick of statement-related PRs to a release branch, which aligns with the changeset's purpose of backporting three upstream commits.
Description check ✅ Passed The description is complete and follows the template: issue numbers are referenced, problem summary explains the cherry-pick rationale, changes are detailed, test checkboxes are marked, and release notes are provided.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

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

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)

Command failed


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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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/util/stmtsummary/v2/stmtsummary_test.go`:
- Around line 164-174: The test samples persistedBefore too late (after eviction
events are already generated), causing flakiness; move the call to
readCounterValue(metrics.StmtSummaryEvictedLogCounter.WithLabelValues(metrics.StmtSummaryTypeV2,
metrics.StmtSummaryEvictedLogResultPersisted)) so that persistedBefore is
captured before any actions that enqueue evictions or trigger async persistence
(i.e. before creating/inserting summaries and before ss.Close()), then keep the
persistedAfter read after Close and assert persistedAfter - persistedBefore ==
2.0.
🪄 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: 702f48b3-1fa6-44ce-a125-95eacc33f2da

📥 Commits

Reviewing files that changed from the base of the PR and between 966abe8 and 5994b3d.

📒 Files selected for processing (19)
  • pkg/executor/select.go
  • pkg/metrics/metrics.go
  • pkg/metrics/metrics_internal_test.go
  • pkg/metrics/stmtsummary.go
  • pkg/sessionctx/sessionstates/session_states_test.go
  • pkg/sessionctx/vardef/tidb_vars.go
  • pkg/sessionctx/variable/session.go
  • pkg/sessionctx/variable/sysvar.go
  • pkg/util/stmtsummary/BUILD.bazel
  • pkg/util/stmtsummary/evicted_test.go
  • pkg/util/stmtsummary/statement_summary.go
  • pkg/util/stmtsummary/statement_summary_test.go
  • pkg/util/stmtsummary/v2/BUILD.bazel
  • pkg/util/stmtsummary/v2/logger.go
  • pkg/util/stmtsummary/v2/reader.go
  • pkg/util/stmtsummary/v2/reader_test.go
  • pkg/util/stmtsummary/v2/record_test.go
  • pkg/util/stmtsummary/v2/stmtsummary.go
  • pkg/util/stmtsummary/v2/stmtsummary_test.go

Comment on lines +164 to +174
persistedBefore := readCounterValue(t, metrics.StmtSummaryEvictedLogCounter.WithLabelValues(
metrics.StmtSummaryTypeV2,
metrics.StmtSummaryEvictedLogResultPersisted,
))
ss.Close()
persistedAfter := readCounterValue(t, metrics.StmtSummaryEvictedLogCounter.WithLabelValues(
metrics.StmtSummaryTypeV2,
metrics.StmtSummaryEvictedLogResultPersisted,
))
require.Equal(t, 2.0, persistedAfter-persistedBefore)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Capture the persisted-counter baseline before enqueueing evictions.

On Line 164, persistedBefore is sampled after eviction events are already generated. Since persistence is async, the baseline may already include part of this test’s increments, making persistedAfter - persistedBefore flaky.

🔧 Proposed fix
-	ss.Add(GenerateStmtExecInfo4Test("digest1"))
-	ss.Add(GenerateStmtExecInfo4Test("digest2"))
-	ss.Add(GenerateStmtExecInfo4Test("digest3")) // evicts digest1
-	ss.Add(GenerateStmtExecInfo4Test("digest4")) // evicts digest2
 	persistedBefore := readCounterValue(t, metrics.StmtSummaryEvictedLogCounter.WithLabelValues(
 		metrics.StmtSummaryTypeV2,
 		metrics.StmtSummaryEvictedLogResultPersisted,
 	))
+	ss.Add(GenerateStmtExecInfo4Test("digest1"))
+	ss.Add(GenerateStmtExecInfo4Test("digest2"))
+	ss.Add(GenerateStmtExecInfo4Test("digest3")) // evicts digest1
+	ss.Add(GenerateStmtExecInfo4Test("digest4")) // evicts digest2
 	ss.Close()
🤖 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/util/stmtsummary/v2/stmtsummary_test.go` around lines 164 - 174, The test
samples persistedBefore too late (after eviction events are already generated),
causing flakiness; move the call to
readCounterValue(metrics.StmtSummaryEvictedLogCounter.WithLabelValues(metrics.StmtSummaryTypeV2,
metrics.StmtSummaryEvictedLogResultPersisted)) so that persistedBefore is
captured before any actions that enqueue evictions or trigger async persistence
(i.e. before creating/inserting summaries and before ss.Close()), then keep the
persistedAfter read after Close and assert persistedAfter - persistedBefore ==
2.0.

@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.03401% with 44 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (release-nextgen-202603@966abe8). Learn more about missing BASE report.

Additional details and impacted files
@@                     Coverage Diff                     @@
##             release-nextgen-202603     #69111   +/-   ##
===========================================================
  Coverage                          ?   76.3947%           
===========================================================
  Files                             ?       1935           
  Lines                             ?     542249           
  Branches                          ?          0           
===========================================================
  Hits                              ?     414250           
  Misses                            ?     127999           
  Partials                          ?          0           
Flag Coverage Δ
unit 76.3947% <85.0340%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 61.5065% <0.0000%> (?)
parser ∅ <0.0000%> (?)
br 48.7860% <0.0000%> (?)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nolouch

nolouch commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

/retest

@yibin87 yibin87 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 16, 2026
@nolouch

nolouch commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

Hi @tenfyzhong, could you approve this cherry-pick for premium?

@ti-chi-bot

ti-chi-bot Bot commented Jun 18, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: terry1purcell, yibin87

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:

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 approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 18, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 18, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-06-16 02:45:09.174775207 +0000 UTC m=+1446410.245092587: ☑️ agreed by yibin87.
  • 2026-06-18 00:23:52.347693651 +0000 UTC m=+1610733.418011051: ☑️ agreed by terry1purcell.

@ti-chi-bot ti-chi-bot Bot merged commit 969ecb5 into pingcap:release-nextgen-202603 Jun 18, 2026
18 checks passed
@nolouch nolouch deleted the cherry-pick-statement-prs-release-nextgen-202603 branch June 18, 2026 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

3 participants