Bypass locks invisible to read snapshot (#243) by windtalker · Pull Request #244 · tikv/client-c

windtalker · 2026-07-16T10:25:05Z

This pr cherry pick #243 to master branch

Summary by CodeRabbit

Bug Fixes
- Improved read consistency when transactions are committed or rolled back.
- Corrected transaction outcome handling so completed transactions can be recognized and cached reliably.
- Reads can now safely bypass locks that no longer require resolution, improving lock handling and reducing unnecessary processing.
Performance
- Optimized read-only lock resolution for eligible transaction states.

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

coderabbitai · 2026-07-16T10:25:24Z

Warning

Review limit reached

@windtalker, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 10 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8768abce-674e-48c4-acc5-4d3abfd52a5e

📥 Commits

Reviewing files that changed from the base of the PR and between 8275d48 and de7c073.

📒 Files selected for processing (2)

src/kv/Scanner.cc
src/test/lock_resolve_test.cc

📝 Walkthrough

Walkthrough

TxnStatus now classifies rolled-back transactions explicitly and derives cacheability from terminal states. Read-only lock resolution uses this status to bypass eligible locks and record their transaction IDs.

Changes

Read lock bypass

Layer / File(s)	Summary
Transaction status classification `include/pingcap/kv/LockResolver.h`	`TxnStatus` defaults `action` to `NoAction`, adds `isRolledBack()`, and simplifies `isCacheable()`.
Read-only lock resolution `src/kv/LockResolver.cc`	Adds `canBypassLockForRead` and records eligible lock transaction IDs before stopping the current resolution iteration.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Reader
  participant LockResolver
  participant TxnStatus
  Reader->>LockResolver: resolveLocks(for_write=false)
  LockResolver->>TxnStatus: evaluate transaction status
  TxnStatus-->>LockResolver: return bypass eligibility
  LockResolver-->>Reader: record txn_id in pushed

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: bypassing locks for read snapshots.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

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

ti-chi-bot · 2026-07-16T10:29:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gengliqi

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~~ [gengliqi]

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-07-16T10:29:04Z

[LGTM Timeline notifier]

Timeline:

2026-07-16 10:29:04.201687511 +0000 UTC m=+882330.237782557: ☑️ agreed by gengliqi.

windtalker · 2026-07-17T07:37:27Z

/test pull-build-test

ti-chi-bot · 2026-07-17T07:37:42Z

@windtalker: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/test pull-build-test

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.

gengliqi · 2026-07-17T07:38:36Z

/ok-to-test

windtalker · 2026-07-17T07:40:42Z

/test pull-build-test

ti-chi-bot · 2026-07-17T07:40:59Z

@windtalker: No presubmit jobs available for tikv/client-c@master

Details

In response to this:

/test pull-build-test

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.

windtalker · 2026-07-17T07:41:31Z

/retest

ekexium

Is there any test?

ekexium · 2026-07-17T08:07:04Z

+                if (!for_write && canBypassLockForRead(status, caller_start_ts))
+                {
+                    pushed.push_back(lock->txn_id);
+                    break;


The lock will not be resolved, so it must be put into resolved_locks to skip it in tikv. However the scanner doesn't use the returned pushed?

In client-c, pushed will be treated as min_commit_ts_pushed in

client-c/src/coprocessor/Client.cc

Lines 723 to 727 in 6b91f49

auto before_expired = cluster->lock_resolver->resolveLocks(bo, task.req->start_ts, locks, pushed);

if (!pushed.empty())

{

min_commit_ts_pushed.addTimestamps(pushed);

}

And min_commit_ts_pushed wil be added to add_resolved_locks when constructing request

client-c/src/coprocessor/Client.cc

Lines 705 to 708 in 6b91f49

for (auto ts : min_commit_ts_pushed.getTimestamps())

{

cop_req_context->add_resolved_locks(ts);

}

I mean here

client-c/src/kv/Scanner.cc

Lines 95 to 96 in 6b91f49

std::vector<uint64_t> pushed{};

auto ms_before_expired = snap.cluster->lock_resolver->resolveLocks(bo, snap.version, locks, pushed);

Good catch. Scanner missed this path. Fixed it by adding returned pushed into snap.min_commit_ts_pushed after resolving scan response locks, and by adding snap.min_commit_ts_pushed to ScanRequest.context.resolved_locks on retry. Also added a scanner regression test for the committed-after-read case.

windtalker · 2026-07-17T08:35:37Z

Is there any test?

Test added

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker · 2026-07-17T09:10:04Z

/retest

Bypass locks invisible to read snapshot (tikv#243)

8275d48

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

ti-chi-bot Bot added dco-signoff: yes Indicates the PR's author has signed the dco. contribution This PR is from a community contributor. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 16, 2026

gengliqi approved these changes Jul 16, 2026

View reviewed changes

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

ti-chi-bot Bot added the approved label Jul 16, 2026

ti-chi-bot Bot added the ok-to-test Indicates a PR is ready to be tested. label Jul 17, 2026

ekexium reviewed Jul 17, 2026

View reviewed changes

Add lock resolver bypass tests

d667e41

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker force-pushed the refine_resolve_lock branch from fe5182d to d667e41 Compare July 17, 2026 08:37

ti-chi-bot Bot added dco-signoff: yes Indicates the PR's author has signed the dco. and removed dco-signoff: no Indicates the PR's author has not signed dco. labels Jul 17, 2026

Use pushed locks in scanner

de7c073

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

	auto before_expired = cluster->lock_resolver->resolveLocks(bo, task.req->start_ts, locks, pushed);
	if (!pushed.empty())
	{
	min_commit_ts_pushed.addTimestamps(pushed);
	}

	for (auto ts : min_commit_ts_pushed.getTimestamps())
	{
	cop_req_context->add_resolved_locks(ts);
	}

	std::vector<uint64_t> pushed{};
	auto ms_before_expired = snap.cluster->lock_resolver->resolveLocks(bo, snap.version, locks, pushed);

Uh oh!

Conversation

windtalker commented Jul 16, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

ti-chi-bot Bot commented Jul 16, 2026

Uh oh!

ti-chi-bot Bot commented Jul 16, 2026

[LGTM Timeline notifier]

Uh oh!

windtalker commented Jul 17, 2026

Uh oh!

ti-chi-bot Bot commented Jul 17, 2026

Uh oh!

gengliqi commented Jul 17, 2026

Uh oh!

windtalker commented Jul 17, 2026

Uh oh!

ti-chi-bot Bot commented Jul 17, 2026

Uh oh!

windtalker commented Jul 17, 2026

Uh oh!

ekexium left a comment

Choose a reason for hiding this comment

Uh oh!

ekexium Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

windtalker Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

ekexium Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

windtalker Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

windtalker commented Jul 17, 2026

Uh oh!

windtalker commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

windtalker commented Jul 16, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 16, 2026 •

edited

Loading