Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

store: avoid sessions of GC being affected by global variables #24975

Merged
merged 17 commits into from
Jun 15, 2021
Merged

store: avoid sessions of GC being affected by global variables #24975

merged 17 commits into from
Jun 15, 2021

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented May 29, 2021 •
edited
Loading

Signed-off-by: Shuaipeng Yu jackysp@gmail.com

What problem does this PR solve?

Issue Number: close #24976

Problem Summary:
The sessions of GC will be affected by global variables, if the user set global autocommit = 0, then saveValueToSysTable will not commit the transaction automatically, which will make the safe point could not go forward.

What is changed and how it works?

Proposal: xxx

What's Changed:
se.GetSessionVars().CommonGlobalLoaded = true for the sessions of GC.

How it Works:

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Manual test (add detailed scripts or steps below)
  1. start a cluster
  2. set global autocommit = 0
  3. show status or select variable_value from mysql.tidb where variable_name = 'tikv_gc_safe_point'; to check the safe point go forward or not.

Release note

  • avoid sessions of GC being affected by global variables

@jackysp
store: avoid sessions of GC being affected by global variables
0fcd9c6 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp jackysp added type/bug-fix This PR fixes a bug. sig/transaction SIG:Transaction labels May 29, 2021
@ti-chi-bot ti-chi-bot added 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. labels May 29, 2021
@jackysp jackysp marked this pull request as ready for review May 29, 2021 13:26
@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 May 29, 2021
@cfzjywxk
Copy link
Contributor

As @MyonKeminta said we may need a recover test case coupled with GC?

@jackysp
Copy link
Member Author

jackysp commented May 31, 2021

@MyonKeminta do we have a test framework to add this case?

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 31, 2021
@ichn-hu ichn-hu mentioned this pull request Jun 3, 2021
Closed
@jackysp
Revert "store: avoid sessions of GC being affected by global variables"
b1a502d 
This reverts commit 0fcd9c6.
@jackysp
tiny change
0a7d3b0 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@ti-chi-bot ti-chi-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 15, 2021
@jackysp
Copy link
Member Author

jackysp commented Jun 15, 2021

PTAL @lysu @MyonKeminta

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • coocood
  • lysu

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.

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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 15, 2021
@lysu
Copy link
Collaborator

lysu commented Jun 15, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 0a7d3b0

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 15, 2021
@youjiali1995
Copy link
Contributor

/run-unit-test

@youjiali1995
Copy link
Contributor

/merge

@youjiali1995
Copy link
Contributor

/merge

1 similar comment
@youjiali1995
Copy link
Contributor

/merge

@ti-chi-bot ti-chi-bot merged commit 47f6ba2 into pingcap:master Jun 15, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 15, 2021
@jackysp @ti-srebot
cherry pick pingcap#24975 to release-5.1
0136f4f 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jun 15, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-5.1 in PR #25449

ti-chi-bot pushed a commit that referenced this pull request Jun 16, 2021
@ti-srebot
store: avoid sessions of GC being affected by global variables (#24975)…
0198ec4 
… (#25449)
@xiongjiwei
Copy link
Contributor

should this PR cherry-pick to 5.0?

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 21, 2021
@jackysp @ti-srebot
cherry pick pingcap#24975 to release-5.0
b605000 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jun 21, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #25609

ti-chi-bot pushed a commit that referenced this pull request Jun 22, 2021
@ti-srebot
store: avoid sessions of GC being affected by global variables (#24975)…
76eae74 
… (#25609)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lysu lysu lysu approved these changes

@coocood coocood coocood approved these changes

@MyonKeminta MyonKeminta MyonKeminta approved these changes

@crazycs520 crazycs520 Awaiting requested review from crazycs520

@cfzjywxk cfzjywxk Awaiting requested review from cfzjywxk

@ekexium ekexium Awaiting requested review from ekexium

Assignees
No one assigned
Labels
needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 sig/transaction SIG:Transaction size/XS Denotes a PR that changes 0-9 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. type/bug-fix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tikv_gc_safe_point in mysql.tidb does not go forward when autocommit = 0 for global
9 participants
@jackysp @cfzjywxk @ti-chi-bot @lysu @youjiali1995 @ti-srebot @xiongjiwei @coocood @MyonKeminta