Skip to content

resolved-ts: use min_lock_ts-1 as the candidate of resolved-ts#17730

Merged
ti-chi-bot[bot] merged 6 commits intotikv:masterfrom
ekexium:fix-resolved-ts-equal-min-commit-ts
Nov 5, 2024
Merged

resolved-ts: use min_lock_ts-1 as the candidate of resolved-ts#17730
ti-chi-bot[bot] merged 6 commits intotikv:masterfrom
ekexium:fix-resolved-ts-equal-min-commit-ts

Conversation

@ekexium
Copy link
Contributor

@ekexium ekexium commented Oct 29, 2024

What is changed and how it works?

Issue Number: Close #17728

What's Changed:

The problem

By the definition of resolved-ts, we require:
commit_ts > resolved_ts, if the commit happens after the calculation of the resolved-ts.

Our implementation uses min_commit_ts to calculate resolved-ts, so that what we have is:
commit_ts >= min_commit_ts >= resolved_ts

resolved_ts may be equal to commit_ts, which breaks the definition of resolved-ts.

The fix

This PR ensures min_commit_ts > resolved_ts to fix the problem

Use min_lock_ts-1 as the candidate of resolved-ts, to ensure resolved_ts < lock.min_commit_ts( <= 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

None

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-triage-completed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. dco-signoff: yes Indicates the PR's author has signed the dco. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2024
@ekexium ekexium force-pushed the fix-resolved-ts-equal-min-commit-ts branch from bd8126f to 3e2c1e9 Compare October 29, 2024 10:14
Signed-off-by: ekexium <eke@fastmail.com>
@ekexium
Copy link
Contributor Author

ekexium commented Oct 31, 2024

/retest

…nt resolved-ts decreasing across TiKV upgrades.

Signed-off-by: ekexium <eke@fastmail.com>
@ekexium ekexium force-pushed the fix-resolved-ts-equal-min-commit-ts branch from 953ecb2 to 5e56746 Compare October 31, 2024 09:41
@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 31, 2024
Copy link
Collaborator

@cfzjywxk cfzjywxk 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 ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Oct 31, 2024
@ekexium ekexium force-pushed the fix-resolved-ts-equal-min-commit-ts branch from 5e56746 to 8b7a2db Compare November 1, 2024 03:44
@ekexium ekexium requested review from you06 and zyguan November 4, 2024 08:04
Co-authored-by: you06 <you1474600@gmail.com>
Signed-off-by: ekexium <eke@fastmail.com>
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 4, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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 lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 4, 2024
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 4, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-10-31 14:16:29.716967988 +0000 UTC m=+532102.556123759: ☑️ agreed by cfzjywxk.
  • 2024-11-04 08:42:11.407954873 +0000 UTC m=+857644.247110418: ☑️ agreed by you06.

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 4, 2024

@ekexium: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

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.

@ekexium
Copy link
Contributor Author

ekexium commented Nov 5, 2024

/retest

@ti-chi-bot ti-chi-bot bot merged commit 7f31c92 into tikv:master Nov 5, 2024
@ti-chi-bot ti-chi-bot bot added this to the Pool milestone Nov 5, 2024
@ti-chi-bot ti-chi-bot bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Nov 5, 2024
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-8.5: #17768.

ti-chi-bot pushed a commit to ti-chi-bot/tikv that referenced this pull request Nov 5, 2024
…17730)

close tikv#17728

Use min_lock_ts-1 as the candidate of resolved-ts, to ensure resolved_ts < lock.min_commit_ts( <= commit_ts).

Signed-off-by: ekexium <eke@fastmail.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
Co-authored-by: you06 <you1474600@gmail.com>
ti-chi-bot bot added a commit that referenced this pull request Nov 5, 2024
… (#17768)

close #17728

Use min_lock_ts-1 as the candidate of resolved-ts, to ensure resolved_ts < lock.min_commit_ts( <= commit_ts).

Signed-off-by: ekexium <eke@fastmail.com>

Co-authored-by: ekexium <eke@fastmail.com>
Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
Co-authored-by: you06 <you1474600@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved dco-signoff: yes Indicates the PR's author has signed the dco. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resolved-ts must be strictly smaller than locks' min_commit_ts when calculating it

4 participants