Skip to content

Doc: Add a design doc for optimizing gc safepoint advance#32934

Merged
ti-chi-bot merged 22 commits intopingcap:masterfrom
TonsnakeLin:dev-design-docs
Apr 24, 2022
Merged

Doc: Add a design doc for optimizing gc safepoint advance#32934
ti-chi-bot merged 22 commits intopingcap:masterfrom
TonsnakeLin:dev-design-docs

Conversation

@TonsnakeLin
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #32725

Problem Summary:

What is changed and how it works?

Check List

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Mar 9, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • MyonKeminta
  • cfzjywxk

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 release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 9, 2022
Signed-off-by: TonsnakeLin <lpbgytong@163.com>
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 9, 2022
@TonsnakeLin
Copy link
Contributor Author

1 similar comment
@TonsnakeLin
Copy link
Contributor Author

@ti-chi-bot
Copy link
Member

@TonsnakeLin: GitHub didn't allow me to request PR reviews from the following users: reviewer.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @Reviewer @cfzjywxk @youjiali1995 @MyonKeminta @sticnarf

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/test-infra repository.

@ti-chi-bot
Copy link
Member

@TonsnakeLin: GitHub didn't allow me to request PR reviews from the following users: reviewer.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @Reviewer @cfzjywxk @youjiali1995 @MyonKeminta @sticnarf

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/test-infra repository.

Signed-off-by: TonsnakeLin <lpbgytong@163.com>
Signed-off-by: TonsnakeLin <lpbgytong@163.com>
@sre-bot
Copy link
Contributor

sre-bot commented Mar 11, 2022

@TonsnakeLin TonsnakeLin requested a review from you06 March 11, 2022 12:47
@TonsnakeLin
Copy link
Contributor Author

/run-unit-test

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 13, 2022
@TonsnakeLin
Copy link
Contributor Author

/run-check_dev


## Introduction

This document describes the design of the feature "optimize gc advance for internal transaction", which adds the startTS(start time stamp) of all internal transactions to the gc(garbage collect) safe point calculation process.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some "time stamp" and "safe point" usage, and I prefer "timestamp" and "safepoint".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


## Motivation or Background

TiDB advances gc safe point every `tidb_gc_run_interval` time with step `tidb_gc_life_time`. If there is a long time transaction from user client lives more than `tidb_gc_life_time`, the safe point can't be advanced until the transaction is finished or lives over 24 hours. This mechanism ensures the continuous advancement of gc safe point and ensures that the data active transactions need to access will not be cleared.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TiDB advances gc safe point every tidb_gc_run_interval time with step tidb_gc_life_time.

This statement is not accurate. Considering a cluster where nothing blocks safepoint's advancing, it's updated every tidb_gc_run_interval and the value are all now - tidb_gc_life_time, thus the step the safepoint advances each time is actually tidb_gc_run_interval too. Consider express it like:

TiDB advances gc safe point every tidb_gc_run_interval time with the value now - tidb_gc_life_time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


#### Store And Delete Internal Sessions

TiDB gets an internal session from a session pool implemented in `(s *session) getInternalSession`, when the transaction finished,it puts the session to the pool. This design stores the internal session to session manger and deletes the internal session from session manager in the function `(s *session) getInternalSession`. It calls the function `infosync.DeleteInternalSession()` to delete the internal session from session manager and calls the function `infosync.StoreInternalSession` to add the internal session to session manger.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TiDB gets an internal session from a session pool implemented in `(s *session) getInternalSession`, when the transaction finished,it puts the session to the pool. This design stores the internal session to session manger and deletes the internal session from session manager in the function `(s *session) getInternalSession`. It calls the function `infosync.DeleteInternalSession()` to delete the internal session from session manager and calls the function `infosync.StoreInternalSession` to add the internal session to session manger.
TiDB gets an internal session from a session pool implemented in `(s *session) getInternalSession`, when the transaction finished,it puts the session to the pool. This design stores the internal session to session manger and deletes the internal session from session manager in the function `(s *session) getInternalSession`. It calls the function `infosync.DeleteInternalSession()` to delete the internal session from session manager and calls the function `infosync.StoreInternalSession` to add the internal session to session manger.

This design stores the internal session to session manger and deletes the internal session from session manager in the function (s *session) getInternalSession.

It actually stores it in getInternalSession, but deletes it in the callback returned by getInternalSession. Consider making it more accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


#### Calculate GC Safe Point

Currently, TiDB calculates gc safe point in the function `(is *InfoSyncer) ReportMinStartTS`. This design add some code in the function `ReportMinStartTS` to consider internal transactions when calculates gc safe point.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer saying ReportMinStartTS calculates the minimum startTS of ongoing transactions (except the ones that exceed the limit), and it's result is useful for calculating the safe point, which is gc_worker's work. The ReportMinStartTS doesn't calculate the safe point directly.

TonsnakeLin and others added 6 commits April 19, 2022 11:02
Co-authored-by: MyonKeminta <9948422+MyonKeminta@users.noreply.github.com>
Co-authored-by: MyonKeminta <9948422+MyonKeminta@users.noreply.github.com>
Co-authored-by: MyonKeminta <9948422+MyonKeminta@users.noreply.github.com>
Signed-off-by: TonsnakeLin <lpbgytong@163.com>
…into dev-design-docs

Signed-off-by: TonsnakeLin <lpbgytong@163.com>
Signed-off-by: TonsnakeLin <lpbgytong@163.com>
@TonsnakeLin
Copy link
Contributor Author

/run-unit-test

Copy link
Contributor

@MyonKeminta MyonKeminta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM.

Co-authored-by: MyonKeminta <9948422+MyonKeminta@users.noreply.github.com>
@TonsnakeLin TonsnakeLin requested a review from MyonKeminta April 22, 2022 07:04
@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 Apr 24, 2022
@cfzjywxk
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

DetailsCommit hash: 9fcb652

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 24, 2022
@ti-chi-bot ti-chi-bot merged commit db07c2e into pingcap:master Apr 24, 2022
@sre-bot
Copy link
Contributor

sre-bot commented Apr 24, 2022

TiDB MergeCI notify

CI Name Result Duration Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test 🟢 all 11 tests passed 14 min Existing passed
idc-jenkins-ci-tidb/tics-test 🟢 all 1 tests passed 13 min Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1 🟢 all 26 tests passed 10 min Existing passed
idc-jenkins-ci-tidb/common-test 🟢 all 12 tests passed 9 min 22 sec Existing passed
idc-jenkins-ci-tidb/integration-ddl-test 🟢 all 6 tests passed 8 min 27 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2 🟢 all 28 tests passed 7 min 39 sec Existing passed
idc-jenkins-ci-tidb/mybatis-test 🟢 all 1 tests passed 4 min 17 sec Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test 🟢 all 1 tests passed 4 min 1 sec Existing passed
idc-jenkins-ci-tidb/plugin-test 🟢 build success, plugin test success 4min Existing passed

espresso98 pushed a commit to espresso98/tidb that referenced this pull request Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 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

Development

Successfully merging this pull request may close these issues.

Optimize gc safepoint advancing for internal transaction

7 participants