Skip to content

resolve-lock: resolve locks, TS of which is bigger than safepoint and expired#35403

Merged
ti-chi-bot merged 24 commits intopingcap:masterfrom
joccau:br-stream-resolve-lock
Jul 5, 2022
Merged

resolve-lock: resolve locks, TS of which is bigger than safepoint and expired#35403
ti-chi-bot merged 24 commits intopingcap:masterfrom
joccau:br-stream-resolve-lock

Conversation

@joccau
Copy link
Contributor

@joccau joccau commented Jun 15, 2022 •
edited by sticnarf
Loading

Signed-off-by: joccau zak.zhao@pingcap.com

What problem does this PR solve?

Issue Number: close #35428

Problem Summary: During the log backup in TiKV endpoints, it maybe cause legacy locks if the tidb-server crash when commit a transaction. This situation causes that: log-backup task will keep checkpoint-ts from advancing and set service-safe-point to keep MVCC data not being GC, which keeps the GC-safepoint not advancing.

What is changed and how it works?

  • maintain a tryResolveLocksTS( tryResolveLocksTS = now() - 5 min)
  • scan locks, ts of which is smaller than tryResolveLocksTS
    • If the tts of lock is equal with or smaller than safepoint, it will rollback the txn, no matter the lock is expired of not.
    • If the tts of lock is larger than safepoint, it will check status of the txn, Resolve the lock if txn is expired, Or do nothing.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Manual test

  • begin a transaction（I have test 2 kinds of txns, OPTIMISTIC and PESSIMISTIC txns）
  • update a big transaction about 540000 line records.
  • during committing this transaction, kill the tidb-server

image

image

as above figure, after kill tidb-server, the legacy locks keep the checkpoint-ts in backup-log from advancing for a while. After tidb-server resolves these legacy locks, the checkpoint-ts advanced normally.

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.

None

gc/resolve lock in which tts is bigger than safepoint
da469da 
Signed-off-by: joccau <zak.zhao@pingcap.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jun 15, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • MyonKeminta
  • sticnarf

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 15, 2022
@joccau
Copy link
Contributor Author

joccau commented Jun 15, 2022

/draft

@joccau joccau changed the title WIP resolve-lock: gc/resolve lock in which tts is bigger than safepoint WIP resolve-lock: gc/resolve lock in which tts is bigger than safepoint(Draft) Jun 15, 2022
make tidy
7ee900a 
Signed-off-by: joccau <zak.zhao@pingcap.com>
MyonKeminta
MyonKeminta reviewed Jun 21, 2022
View reviewed changes
joccau added 3 commits June 29, 2022 16:30
gc/resolve lock in which tts is bigger than safepoint
3060e76 
Signed-off-by: joccau <zak.zhao@pingcap.com>
make tidy
73c4888 
Signed-off-by: joccau <zak.zhao@pingcap.com>
resolve conflicts
8663af5 
Signed-off-by: joccau <zak.zhao@pingcap.com>
@joccau joccau requested a review from a team as a code owner June 29, 2022 08:34
@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 29, 2022
@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 29, 2022
@joccau joccau changed the title WIP resolve-lock: gc/resolve lock in which tts is bigger than safepoint(Draft) resolve-lock: resolve locks, TS of which is bigger than safepoint and expired Jun 29, 2022
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2022
@joccau joccau removed the request for review from a team June 29, 2022 09:43
joccau and others added 5 commits June 29, 2022 17:43
@joccau
Merge branch 'master' into br-stream-resolve-lock
935e4b1
@joccau @MyonKeminta
Update store/gcworker/gc_worker.go
fca99c4 
Co-authored-by: MyonKeminta <9948422+MyonKeminta@users.noreply.github.com>
@joccau @MyonKeminta
Update store/gcworker/gc_worker.go
e65b2f6 
Co-authored-by: MyonKeminta <9948422+MyonKeminta@users.noreply.github.com>
maintain test case
4c14850 
Signed-off-by: joccau <zak.zhao@pingcap.com>
add test case
780a9d9 
Signed-off-by: joccau <zak.zhao@pingcap.com>
@joccau joccau force-pushed the br-stream-resolve-lock branch from a012a26 to 5022f11 Compare July 1, 2022 11:06
@joccau
Copy link
Contributor Author

joccau commented Jul 1, 2022

/run-all-tests

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2022
maintain test case
4678c08 
Signed-off-by: joccau <zak.zhao@pingcap.com>
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 2, 2022
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2022
maintain test case
ae056d5 
Signed-off-by: joccau <zak.zhao@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Jul 2, 2022 •
edited
Loading

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/4791d36b074447811779dcdafb212452e3de5a0c

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 4, 2022
@joccau
Copy link
Contributor Author

joccau commented Jul 4, 2022

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jul 4, 2022

This pull request has been accepted and is ready to merge.

DetailsCommit hash: ae056d5

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 4, 2022
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jul 4, 2022

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

At the same time I will also trigger all tests for you:

/run-all-tests

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.

@joccau
Copy link
Contributor Author

joccau commented Jul 4, 2022

/run-all-tests

@joccau
Copy link
Contributor Author

joccau commented Jul 4, 2022

/run-check_dev_2

@ti-chi-bot ti-chi-bot merged commit af61851 into pingcap:master Jul 5, 2022
@joccau joccau deleted the br-stream-resolve-lock branch January 10, 2023 02:18
@3pointer 3pointer mentioned this pull request Aug 8, 2023
Merged
11 tasks
This was referenced Sep 12, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MyonKeminta MyonKeminta MyonKeminta approved these changes

@sticnarf sticnarf sticnarf approved these changes

@3pointer 3pointer Awaiting requested review from 3pointer

Assignees

No one assigned

Labels

release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BR] log backup checkpoint_ts doesn't move forward when there is residual tidb lock

7 participants

@joccau @ti-chi-bot @TonsnakeLin @sre-bot @3pointer @MyonKeminta @sticnarf