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

executor: fix data race of "admin check table" #11568

Merged
merged 2 commits into from
Aug 1, 2019
Merged

executor: fix data race of "admin check table" #11568

merged 2 commits into from
Aug 1, 2019

Conversation

zimulala
Copy link
Contributor

@zimulala zimulala commented Aug 1, 2019

What problem does this PR solve?

Fix the data race as follows:

[2019-08-01T03:20:55.984Z] WARNING: DATA RACE
[2019-08-01T03:20:55.984Z] Write at 0x00c011f4b740 by goroutine 375:
[2019-08-01T03:20:55.984Z]   github.com/pingcap/tidb/sessionctx/stmtctx.(*StatementContext).MergeExecDetails()
[2019-08-01T03:20:55.984Z]       /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/sessionctx/stmtctx/stmtctx.go:431 +0x8b
[2019-08-01T03:20:55.984Z]   github.com/pingcap/tidb/distsql.(*selectResult).getSelectResp()
[2019-08-01T03:20:55.984Z]       /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/distsql/select_result.go:197 +0x408
...
/home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/table_reader.go:139 +0x7b
[2019-08-01T03:20:55.984Z]   github.com/pingcap/tidb/executor.(*tableWorker).compareData()
[2019-08-01T03:20:55.984Z]       /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/distsql.go:804 +0x13d8
[2019-08-01T03:20:55.984Z]
[2019-08-01T03:20:55.984Z] Previous write at 0x00c011f4b740 by goroutine 241:
[2019-08-01T03:20:55.984Z]   github.com/pingcap/tidb/executor.ResetContextOfStmt()
[2019-08-01T03:20:55.984Z]       /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/executor.go:1336 +0x2d2
[2019-08-01T03:20:55.984Z]   github.com/pingcap/tidb/session.(*session).execute()
[2019-08-01T03:20:55.984Z]       /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/session/session.go:1073 +0x7d5
[2019-08-01T03:20:55.984Z]   github.com/pingcap/tidb/session.(*session).Execute()
...
[2019-08-01T03:20:55.984Z]   github.com/pingcap/tidb/executor_test.(*testSuite2).TestAdminCleanupIndexMore()
[2019-08-01T03:20:55.984Z]       /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/admin_test.go:379 +0xed0
[2019-08-01T03:20:55.984Z]   github.com/pingcap/tidb/executor_test.(*testSuite2).TestAdminCleanupIndexMore()
[2019-08-01T03:20:55.984Z]       /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/admin_test.go:343 +0x16f

What is changed and how it works?

  • Add a Close for CheckTableExec to close all srcs.

  • Use ExecToErr instead of Exec

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

@zimulala zimulala added type/bugfix This PR fixes a bug. sig/execution SIG execution needs-cherry-pick-3.0 labels Aug 1, 2019
@codecov
Copy link

codecov bot commented Aug 1, 2019 •
edited
Loading

Codecov Report

Merging #11568 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11568   +/-   ##
===========================================
  Coverage   81.3131%   81.3131%           
===========================================
  Files           426        426           
  Lines         92375      92375           
===========================================
  Hits          75113      75113           
  Misses        11893      11893           
  Partials       5369       5369

alivxxx
alivxxx reviewed Aug 1, 2019
View reviewed changes
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 1, 2019
@zimulala
Copy link
Contributor Author

zimulala commented Aug 1, 2019

/run-all-tests

XuHuaiyu
XuHuaiyu approved these changes Aug 1, 2019
View reviewed changes
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 1, 2019
@zimulala zimulala merged commit 576cdcd into pingcap:master Aug 1, 2019
@zimulala zimulala deleted the fix-data-race branch August 1, 2019 10:53
@sre-bot
Copy link
Contributor

sre-bot commented Aug 1, 2019

cherry pick to release-3.0 failed

zimulala added a commit to zimulala/tidb that referenced this pull request Aug 2, 2019
@zimulala
executor: fix data race of "admin check table" (pingcap#11568)
7b52335
@zimulala zimulala mentioned this pull request Aug 2, 2019
Merged
zimulala added a commit to zimulala/tidb that referenced this pull request Aug 8, 2019
@zimulala
executor: fix data race of "admin check table" (pingcap#11568)
96b1137
@zimulala zimulala mentioned this pull request Aug 8, 2019
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

It seems that, not for sure, we failed to cherry-pick this commit to release-3.0. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @zimulala PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alivxxx alivxxx alivxxx left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

Assignees
No one assigned
Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zimulala @sre-bot @XuHuaiyu @alivxxx