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 lock handling by accurately distinguishing committed vs rolled-back transactions when determining cacheability and bypass behavior.
- Read operations now correctly bypass locks in cases where commit/rollback occurs after the read timestamp, improving consistency.
- Scans now proactively incorporate resolved-lock timing so subsequent scan batches observe consistent results.
Performance
- Reduced unnecessary lock processing for eligible read paths by refining bypass eligibility.
Tests / CI
- Strengthened mock service readiness checks and added targeted lock-resolution test coverage.

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

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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6f487cb7-6791-4122-b383-c76fdbc322dc

📥 Commits

Reviewing files that changed from the base of the PR and between f6d3fdc and 1e367b1.

📒 Files selected for processing (1)

src/kv/LockResolver.cc

💤 Files with no reviewable changes (1)

src/kv/LockResolver.cc

📝 Walkthrough

Walkthrough

TxnStatus now classifies rolled-back transactions explicitly. Read-only lock resolution bypasses eligible locks, propagates resolved-lock timestamps across scans, adds coverage for these paths, and waits for mock TiKV readiness in CI.

Changes

Read lock bypass and scan propagation

Layer / File(s)	Summary
Transaction status classification `include/pingcap/kv/LockResolver.h`	`TxnStatus` defaults `action` to `NoAction`, adds `isRolledBack()`, and derives `isCacheable()` from committed or rolled-back states.
Read-only lock resolution `src/kv/LockResolver.cc`	Adds read-path bypass checks that record eligible transaction IDs in `pushed` while continuing the existing lock-resolution flow.
Scanner integration and validation `src/kv/Scanner.cc`, `src/test/lock_resolve_test.cc`	Carries resolved-lock timestamps between scan batches and tests committed-after-read, committed-before-read, scanner bypass, and rolled-back transaction behavior.

Mock TiKV test readiness

Layer / File(s)	Summary
Mock service lifecycle `ci/build-test.sh`	Waits for mock TiKV liveness and TCP readiness before tests, then centralizes cleanup through an EXIT trap.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Snapshot
  participant Scanner
  participant LockResolver
  participant TxnStatus
  Snapshot->>Scanner: request scan batch
  Scanner->>LockResolver: resolveLocks(for_write=false)
  LockResolver->>TxnStatus: evaluate transaction status
  TxnStatus-->>LockResolver: return bypass eligibility
  LockResolver-->>Scanner: return pushed timestamps
  Scanner->>Scanner: add resolved-lock timestamps to scan context
  Scanner-->>Snapshot: return scan results

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

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

ti-chi-bot · 2026-07-20T04:51:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ekexium, 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~~ [ekexium,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-20T04:51:48Z

[LGTM Timeline notifier]

Timeline:

2026-07-16 10:29:04.201687511 +0000 UTC m=+882330.237782557: ☑️ agreed by gengliqi.
2026-07-20 04:51:47.403912774 +0000 UTC m=+1207693.440007820: ☑️ agreed by ekexium.

windtalker · 2026-07-20T04:58:56Z

/retest

windtalker · 2026-07-20T06:08:33Z

/retest

windtalker · 2026-07-20T06:32:27Z

/retest

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

coderabbitai

🧹 Nitpick comments (2)

ci/build-test.sh (2)
32-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer graceful termination over kill -9.

Using kill -9 (SIGKILL) as a first resort is generally discouraged because it prevents the process from cleaning up resources, temporary files, or child processes. Consider using a standard kill (SIGTERM) to allow mock-tikv to shut down gracefully.
♻️ Proposed refactor
 mock_kv_pid=""
 cleanupMockTiKV() {
     if [ -n "$mock_kv_pid" ]; then
-        kill -9 "$mock_kv_pid" 2>/dev/null || true
+        kill "$mock_kv_pid" 2>/dev/null || true
     fi
 }
 trap cleanupMockTiKV EXIT
🤖 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 `@ci/build-test.sh` around lines 32 - 38, Update cleanupMockTiKV to send the
default graceful termination signal with kill instead of forcing SIGKILL, while
preserving the existing PID check, suppressed errors, and EXIT trap.
48-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Ensure the script fails if mock-tikv is not ready.

If set -e is not active in this script, calling waitForMockTiKV and having it return 1 will not stop the script. The tests would then run (and fail) against a missing mock server. Explicitly handling the failure makes the intent clear and ensures a fast failure in CI.
♻️ Proposed refactor
-waitForMockTiKV
+waitForMockTiKV || exit 1
🤖 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 `@ci/build-test.sh` at line 48, Update the call to waitForMockTiKV in the
build-test script to explicitly stop execution when it returns a nonzero status,
ensuring tests do not run unless mock-tikv is ready.

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

Nitpick comments:
In `@ci/build-test.sh`:
- Around line 32-38: Update cleanupMockTiKV to send the default graceful
termination signal with kill instead of forcing SIGKILL, while preserving the
existing PID check, suppressed errors, and EXIT trap.
- Line 48: Update the call to waitForMockTiKV in the build-test script to
explicitly stop execution when it returns a nonzero status, ensuring tests do
not run unless mock-tikv is ready.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e3ef9f4b-61b4-4fc1-b239-731dcaa27a59

📥 Commits

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

📒 Files selected for processing (3)

ci/build-test.sh
src/kv/Scanner.cc
src/test/lock_resolve_test.cc

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

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 needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels 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>

ekexium approved these changes Jul 20, 2026

View reviewed changes

ti-chi-bot Bot added the lgtm label Jul 20, 2026

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

Wait for mock-tikv in build test

f6d3fdc

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

coderabbitai Bot reviewed Jul 20, 2026

View reviewed changes

Resolve locks after marking them bypassable

1e367b1

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

ti-chi-bot Bot merged commit d64c1d5 into tikv:master Jul 20, 2026
5 checks passed

coderabbitai Bot mentioned this pull request Jul 22, 2026

Support bypassing resolved locks for read #249

Merged

	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!

Walkthrough

Changes

Sequence Diagram(s)

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!

ti-chi-bot Bot commented Jul 20, 2026

Uh oh!

ti-chi-bot Bot commented Jul 20, 2026

[LGTM Timeline notifier]

Uh oh!

windtalker commented Jul 20, 2026

Uh oh!

windtalker commented Jul 20, 2026

Uh oh!

windtalker commented Jul 20, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

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