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

*: add a sysvar to enable mutation checker #28663

Merged
merged 5 commits into from
Oct 19, 2021
Merged

*: add a sysvar to enable mutation checker #28663

merged 5 commits into from
Oct 19, 2021

Conversation

ekexium
Copy link
Member

@ekexium ekexium commented Oct 8, 2021

Signed-off-by: ekexium ekexium@gmail.com

What problem does this PR solve?

Issue Number: #26833

Problem Summary:

Add a system variable tidb_enable_mutation_checker to enable the check.

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

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

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note 

Introduce a system variable `tidb_enable_mutation_checker`

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Oct 8, 2021 •
edited
Loading

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

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 Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 8, 2021
cfzjywxk
cfzjywxk reviewed Oct 9, 2021
View reviewed changes
sessionctx/variable/session.go
@@ -952,6 +952,9 @@ type SessionVars struct {
curr int8
data [2]stmtctx.StatementContext
}

// EnableMutationChecker indicates whether to check data consistency for mutations
EnableMutationChecker bool
Copy link
Contributor

Choose a reason for hiding this comment

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

In the relaese version we may need to use some hidden http api to control the checking mechanism and users will not need to know about it.

@ekexium ekexium force-pushed the sysvar-mutation-checker branch from dd0126e to 35c4020 Compare October 9, 2021 03:55
@ekexium
Copy link
Member Author

ekexium commented Oct 9, 2021

/run-check_dev

@ekexium
Copy link
Member Author

ekexium commented Oct 18, 2021

Comments resolved, PTAL @cfzjywxk @MyonKeminta

MyonKeminta
MyonKeminta reviewed Oct 19, 2021
View reviewed changes
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

sessionctx/variable/tidb_vars.go Outdated
@@ -745,6 +748,7 @@ const (
DefTMPTableSize = 16777216
DefTiDBEnableLocalTxn = false
DefTiDBEnableOrderedResultMode = false
DefTiDBEnableMutationChecker = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we set it false if the cluster is upgraded from an old version?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get it. Why is an upgraded cluster special?

Copy link
Contributor

@MyonKeminta MyonKeminta Oct 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Because I'm afraid there's some risk that the user's working service be broken by false-positive checks if there's bug in our implementation (?)
@cfzjywxk How do you think

Copy link
Contributor

Choose a reason for hiding this comment

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

For upgrading better to make the newly introduced features disabled by default, for newly created clusters, we could default enable it. It's safer to disable it by default and enable it in bootstrap stage checking specific conditions

Copy link
Member Author

Choose a reason for hiding this comment

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

All right

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 19, 2021
cfzjywxk
cfzjywxk approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@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 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 19, 2021
@cfzjywxk
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: a47c225fbf02402a9642549daba0e2d245017162

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 19, 2021
@ekexium
add a sysvar to enable mutation checker
fbb1fe0 
Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium
add an HTTP API to modify tidb_enable_mutation_checker
0aac33d 
Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium
hide tidb_enable_mutation_checker
e600796 
Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium
change the default behavior: only enable mutation checker for new clu…
96ef757 
…sters

Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium ekexium force-pushed the sysvar-mutation-checker branch from a47c225 to 96ef757 Compare October 19, 2021 11:08
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Oct 19, 2021
@ti-chi-bot
Copy link
Member

Merge canceled because a new commit is pushed.

@ekexium
fix name
d64ebe4 
Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium ekexium merged commit 2f8f4d6 into pingcap:ft-data-inconsistency Oct 19, 2021
@ekexium ekexium mentioned this pull request Nov 10, 2021
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfzjywxk cfzjywxk cfzjywxk approved these changes

@MyonKeminta MyonKeminta MyonKeminta approved these changes

Assignees
No one assigned
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. 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.
4 participants
@ekexium @ti-chi-bot @cfzjywxk @MyonKeminta