storage: fix wrong commit_ts in Get/TableScan when latest write is Op_Lock (#19374) by lcwangchao · Pull Request #19394 · tikv/tikv

lcwangchao · 2026-02-25T08:08:46Z

What is changed and how it works?

Issue Number: Close #19393

What's Changed:

storage: fix wrong commit_ts in Get/TableScan when latest write is Op_Lock

Problem Summary:
- When `load_commit_ts=true` and a key has newer committed `Op_Lock` versions on top of the visible `Op_Put`, TiKV may return the lock commit_ts instead of the put commit_ts.

What changed:
- Fix `PointGetter` to return the commit_ts of the actual write version loaded by `last_change` shortcut, instead of the current cursor version.
- Fix backward scanner to only update `loaded_commit_ts` when selecting a `Put` version, avoiding ts overwrite from `Lock/Rollback` versions.

Tests:
- Add `test_point_get_load_commit_ts_with_top_lock_versions` in `point_getter.rs`.
- Add `test_load_commit_ts_with_top_lock_versions` in `scanner/backward.rs`.
- Verify existing related tests:
  - `test_point_get_load_commit_ts`
  - `test_scan_with_load_commit_ts`

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

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

Release note

Fix wrong commit_ts returned by Get and TableScan when `load_commit_ts` is enabled and newer `Op_Lock` versions exist above the latest `Op_Put`.

Summary by CodeRabbit

New Features
- Added configuration option to enable or disable commit timestamp loading during scan operations.
Bug Fixes
- Fixed commit timestamp handling in backward scans and point queries to correctly cache and reuse timestamp values, preventing incorrect results in edge cases involving lock versions and deletion markers.
Tests
- Expanded test coverage for commit timestamp loading across various scan scenarios and lock configurations.

…_Lock (tikv#19374) close tikv#19312 storage: fix wrong commit_ts in Get/TableScan when latest write is Op_Lock Problem Summary: - When `load_commit_ts=true` and a key has newer committed `Op_Lock` versions on top of the visible `Op_Put`, TiKV may return the lock commit_ts instead of the put commit_ts. What changed: - Fix `PointGetter` to return the commit_ts of the actual write version loaded by `last_change` shortcut, instead of the current cursor version. - Fix backward scanner to only update `loaded_commit_ts` when selecting a `Put` version, avoiding ts overwrite from `Lock/Rollback` versions. Tests: - Add `test_point_get_load_commit_ts_with_top_lock_versions` in `point_getter.rs`. - Add `test_load_commit_ts_with_top_lock_versions` in `scanner/backward.rs`. - Verify existing related tests: - `test_point_get_load_commit_ts` - `test_scan_with_load_commit_ts` Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>

coderabbitai · 2026-02-25T08:09:11Z

📝 Walkthrough

Walkthrough

This PR fixes a bug where TiKV returns incorrect commit timestamps for point-get and reverse-scan operations. It introduces a commit timestamp caching mechanism in the point getter to correctly handle cases where newer lock versions exist above visible put versions, and propagates commit timestamp loading in backward scanners.

Changes

Cohort / File(s)	Summary
Point Getter Commit Timestamp Caching `src/storage/mvcc/reader/point_getter.rs`	Introduces `loaded_write_commit_ts` field to cache and reuse commit timestamps when iterating through last_change shortcuts in get_data path. Ensures subsequent iterations use correct commit_ts for Put entries, preventing incorrect commit_ts usage when newer versions are loaded via last_change. Adds 8 comprehensive tests covering scenarios with top locks, seek bounds, and various write operations.
Backward Scanner Commit Timestamp Propagation `src/storage/mvcc/reader/scanner/backward.rs`	Propagates commit timestamp loading when scanning in reverse by setting loaded_commit_ts to current key's commit_ts during Put write handling. Adds `set_load_commit_ts(bool)` method to ScannerBuilder public API. Includes test validating correct commit_ts extraction from Put versions when newer Lock writes exist.
Scanner Test Module `src/storage/mvcc/reader/scanner/mod.rs`	Updates test imports to include `LastChange` and adds new comprehensive test `test_scan_with_load_commit_ts_top_lock_versions_across_seek_bound_and_delete` validating load_commit_ts behavior across seek bounds and deletions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A lock sat high, a put below,
But timestamps mixed in confusing flow—
Now caches guard each commit with care,
The scanner leaps without despair!
Where locks once tricked the path we'd take,
The right commit_ts we shall make! 🌟

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'storage: fix wrong commit_ts in Get/TableScan when latest write is Op_Lock' clearly and specifically describes the bug fix addressing incorrect commit_ts handling for point-get and table scans.
Description check	✅ Passed	The description includes the required Issue Number (Close `#19393`), detailed 'What's Changed' section with commit message format, test information, and release notes. All key template sections are completed.
Linked Issues check	✅ Passed	The PR code changes directly address issue `#19393`'s core requirements: fixing PointGetter to return correct commit_ts via last_change shortcut and preventing loaded_commit_ts overwrite by Lock/Rollback versions in backward scanner, with corresponding test coverage.
Out of Scope Changes check	✅ Passed	All changes are scope-specific: PointGetter caches commit_ts for last_change paths, backward scanner selectively updates loaded_commit_ts, and tests validate the fixes. The ScannerBuilder API addition (set_load_commit_ts) is directly necessary for enabling the feature.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches

📝 Generate docstrings (stacked PR)
📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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

Copilot

Pull request overview

Fixes incorrect _tidb_commit_ts being returned by point-get and scans when load_commit_ts is enabled and newer committed Op_Lock writes exist above the latest visible Op_Put (per Close #19393 / ref #19374).

Changes:

PointGetter: track and return the commit_ts of the actual Write loaded via the last_change shortcut, instead of decoding ts from the cursor’s current (newer) position.
Backward scanner: only set loaded_commit_ts when a Put version is selected (avoids overwriting with Lock/Rollback commit_ts).
Adds targeted regression tests covering top-lock stacks across seek-bound thresholds and delete visibility.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
`src/storage/mvcc/reader/scanner/mod.rs`	Adds a forward-scan regression test validating `load_commit_ts` correctness with many top `LOCK` writes (both `< SEEK_BOUND` and `>= SEEK_BOUND`) and deleted keys.
`src/storage/mvcc/reader/scanner/backward.rs`	Fixes reverse-scan `loaded_commit_ts` bookkeeping so commit_ts comes from the returned `PUT`, plus adds a regression test.
`src/storage/mvcc/reader/point_getter.rs`	Fixes point-get commit_ts derivation when `last_change` triggers a `get_cf` jump, plus adds regression tests for both `<`/`>= SEEK_BOUND` and delete cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

coderabbitai

🧹 Nitpick comments (1)

src/storage/mvcc/reader/scanner/backward.rs (1)
383-391: The seek-loop fix is correct, but the new test does not exercise this code path.

In test_load_commit_ts_with_top_lock_versions, the PUT@ts=1 is the earliest entry for key "k". The prev() loop starts there (i=0), captures loaded_commit_ts = Some(1), and subsequent iterations only see LOCKs. After the loop, the seek path re-scans ts=REVERSE_SEEK_BOUND+2 down to last_checked_commit_ts and terminates with handle_last_version, which already carries the correct loaded_commit_ts from the prev() loop. Lines 383–386 are never reached in that test.

The specific scenario these lines guard against is when the prev() loop exhausts all REVERSE_SEEK_BOUND iterations without finding a PUT (e.g., all encountered versions are LOCKs), and then the seek loop finds a PUT in the range (last_checked_commit_ts, scan_ts]. A concrete setup:
// PUT above last_checked_commit_ts so seek loop hits it directly
must_prewrite_put(&mut engine, b"k", b"v", b"k", 1);
must_commit(&mut engine, b"k", 1, 1);
// enough LOCKs so prev() loop is exhausted without finding the PUT
for ts in 2..=REVERSE_SEEK_BOUND + 2 {
    must_prewrite_lock(&mut engine, b"k", b"k", ts);
    must_commit(&mut engine, b"k", ts, ts);
}
// a second PUT above everything, visible, so seek loop finds it first
must_prewrite_put(&mut engine, b"k", b"v2", b"k", REVERSE_SEEK_BOUND + 3);
must_commit(&mut engine, b"k", REVERSE_SEEK_BOUND + 3, REVERSE_SEEK_BOUND + 3);
// scan at REVERSE_SEEK_BOUND + 5 so the PUT@REVERSE_SEEK_BOUND+3 is between
// last_checked_commit_ts and scan_ts
Consider adding a test case that forces this code path.

Would you like me to draft the missing test case for the seek-loop PUT path?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/storage/mvcc/reader/scanner/backward.rs` around lines 383 - 391, Add a
unit test that reproduces the seek-loop PUT path by exhausting the prev() loop
with REVERSE_SEEK_BOUND LOCK entries and ensuring the seek loop then finds a PUT
in (last_checked_commit_ts, scan_ts] so lines that call
reverse_load_data_by_write and ValueEntry::new are exercised; specifically,
create a PUT at ts=1, then for ts in 2..=REVERSE_SEEK_BOUND+2 create LOCKs
(prewrite_lock + commit) so prev() is exhausted, then create a PUT at
REVERSE_SEEK_BOUND+3 (prewrite_put + commit) and run the same scan at scan_ts =
REVERSE_SEEK_BOUND+5 (or similar) asserting that the returned ValueEntry uses
reverse_load_data_by_write and carries the correct loaded_commit_ts; name it
e.g. test_load_commit_ts_seek_loop_put_path and use must_prewrite_put,
must_commit, must_prewrite_lock, REVERSE_SEEK_BOUND, reverse_load_data_by_write,
and ValueEntry assertions to locate the behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/storage/mvcc/reader/scanner/backward.rs`:
- Around line 383-391: Add a unit test that reproduces the seek-loop PUT path by
exhausting the prev() loop with REVERSE_SEEK_BOUND LOCK entries and ensuring the
seek loop then finds a PUT in (last_checked_commit_ts, scan_ts] so lines that
call reverse_load_data_by_write and ValueEntry::new are exercised; specifically,
create a PUT at ts=1, then for ts in 2..=REVERSE_SEEK_BOUND+2 create LOCKs
(prewrite_lock + commit) so prev() is exhausted, then create a PUT at
REVERSE_SEEK_BOUND+3 (prewrite_put + commit) and run the same scan at scan_ts =
REVERSE_SEEK_BOUND+5 (or similar) asserting that the returned ValueEntry uses
reverse_load_data_by_write and carries the correct loaded_commit_ts; name it
e.g. test_load_commit_ts_seek_loop_put_path and use must_prewrite_put,
must_commit, must_prewrite_lock, REVERSE_SEEK_BOUND, reverse_load_data_by_write,
and ValueEntry assertions to locate the behavior.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 188d9fd and 709be50.

📒 Files selected for processing (3)

src/storage/mvcc/reader/point_getter.rs
src/storage/mvcc/reader/scanner/backward.rs
src/storage/mvcc/reader/scanner/mod.rs

lcwangchao · 2026-02-25T12:58:02Z

/retest

ti-chi-bot · 2026-02-26T10:24:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, Connor1996

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~~ [Connor1996,cfzjywxk]

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 · 2026-02-26T10:24:36Z

[LGTM Timeline notifier]

Timeline:

2026-02-26 10:16:03.064418551 +0000 UTC m=+350035.579213150: ☑️ agreed by cfzjywxk.
2026-02-26 10:24:34.708649675 +0000 UTC m=+350547.223444284: ☑️ agreed by Connor1996.

lcwangchao · 2026-02-26T13:01:21Z

/cherry-pick release-8.5

ti-chi-bot · 2026-02-26T13:02:43Z

@lcwangchao: new pull request created to branch release-8.5: #19397.

Details

In response to this:

/cherry-pick release-8.5

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 ti-community-infra/tichi repository.

…_Lock (#19374) (#19394) (#19397) close #19393 storage: fix wrong commit_ts in Get/TableScan when latest write is Op_Lock Problem Summary: - When `load_commit_ts=true` and a key has newer committed `Op_Lock` versions on top of the visible `Op_Put`, TiKV may return the lock commit_ts instead of the put commit_ts. What changed: - Fix `PointGetter` to return the commit_ts of the actual write version loaded by `last_change` shortcut, instead of the current cursor version. - Fix backward scanner to only update `loaded_commit_ts` when selecting a `Put` version, avoiding ts overwrite from `Lock/Rollback` versions. Tests: - Add `test_point_get_load_commit_ts_with_top_lock_versions` in `point_getter.rs`. - Add `test_load_commit_ts_with_top_lock_versions` in `scanner/backward.rs`. - Verify existing related tests: - `test_point_get_load_commit_ts` - `test_scan_with_load_commit_ts` Signed-off-by: Chao Wang <cclcwangchao@hotmail.com> Co-authored-by: Chao Wang <cclcwangchao@hotmail.com>

…_Lock (tikv#19374) (tikv#19394) close tikv#19393 storage: fix wrong commit_ts in Get/TableScan when latest write is Op_Lock Problem Summary: - When `load_commit_ts=true` and a key has newer committed `Op_Lock` versions on top of the visible `Op_Put`, TiKV may return the lock commit_ts instead of the put commit_ts. What changed: - Fix `PointGetter` to return the commit_ts of the actual write version loaded by `last_change` shortcut, instead of the current cursor version. - Fix backward scanner to only update `loaded_commit_ts` when selecting a `Put` version, avoiding ts overwrite from `Lock/Rollback` versions. Tests: - Add `test_point_get_load_commit_ts_with_top_lock_versions` in `point_getter.rs`. - Add `test_load_commit_ts_with_top_lock_versions` in `scanner/backward.rs`. - Verify existing related tests: - `test_point_get_load_commit_ts` - `test_scan_with_load_commit_ts` Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>

…_Lock (tikv#19374) (tikv#19394) close tikv#19393 storage: fix wrong commit_ts in Get/TableScan when latest write is Op_Lock Problem Summary: - When `load_commit_ts=true` and a key has newer committed `Op_Lock` versions on top of the visible `Op_Put`, TiKV may return the lock commit_ts instead of the put commit_ts. What changed: - Fix `PointGetter` to return the commit_ts of the actual write version loaded by `last_change` shortcut, instead of the current cursor version. - Fix backward scanner to only update `loaded_commit_ts` when selecting a `Put` version, avoiding ts overwrite from `Lock/Rollback` versions. Tests: - Add `test_point_get_load_commit_ts_with_top_lock_versions` in `point_getter.rs`. - Add `test_load_commit_ts_with_top_lock_versions` in `scanner/backward.rs`. - Verify existing related tests: - `test_point_get_load_commit_ts` - `test_scan_with_load_commit_ts` Signed-off-by: Chao Wang <cclcwangchao@hotmail.com> Signed-off-by: rishabh mittal <mittalrishabh@gmail.com>

Copilot AI review requested due to automatic review settings February 25, 2026 08:08

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has signed the dco. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 25, 2026

Copilot started reviewing on behalf of lcwangchao February 25, 2026 08:09 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes

coderabbitai bot reviewed Feb 25, 2026

View reviewed changes

cfzjywxk approved these changes Feb 26, 2026

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Feb 26, 2026

Connor1996 approved these changes Feb 26, 2026

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Feb 26, 2026

ti-chi-bot bot merged commit eb8aa3a into tikv:master Feb 26, 2026
13 of 14 checks passed

ti-chi-bot bot added this to the Pool milestone Feb 26, 2026

lcwangchao deleted the fix_commit_ts branch February 26, 2026 13:00

ti-chi-bot mentioned this pull request Feb 26, 2026

storage: fix wrong commit_ts in Get/TableScan when latest write is Op_Lock (#19374) (#19394) #19397

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: fix wrong commit_ts in Get/TableScan when latest write is Op_Lock (#19374)#19394

storage: fix wrong commit_ts in Get/TableScan when latest write is Op_Lock (#19374)#19394
ti-chi-bot[bot] merged 1 commit intotikv:masterfrom
lcwangchao:fix_commit_ts

lcwangchao commented Feb 25, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Feb 25, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

Copilot AI left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

lcwangchao commented Feb 25, 2026

Uh oh!

ti-chi-bot bot commented Feb 26, 2026

Uh oh!

ti-chi-bot bot commented Feb 26, 2026

Uh oh!

Uh oh!

lcwangchao commented Feb 26, 2026

Uh oh!

ti-chi-bot commented Feb 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

lcwangchao commented Feb 25, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is changed and how it works?

Related changes

Check List

Release note

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

lcwangchao commented Feb 25, 2026

Uh oh!

ti-chi-bot bot commented Feb 26, 2026

Uh oh!

ti-chi-bot bot commented Feb 26, 2026

[LGTM Timeline notifier]

Uh oh!

Uh oh!

lcwangchao commented Feb 26, 2026

Uh oh!

ti-chi-bot commented Feb 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lcwangchao commented Feb 25, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Feb 25, 2026 •

edited

Loading