ddl: fix concurrent access to `do.infoHandle` has a data race error #8287

ciscoxll · 2018-11-12T17:22:28Z

What problem does this PR solve?

WARNING: DATA RACE
Write at 0x00c0001d4c70 by goroutine 51:
  github.com/pingcap/tidb/ddl_test.(*testFailDBSuite).TestHalfwayCancelOperations()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/domain/domain.go:538 +0x1c80
  github.com/pingcap/tidb/ddl_test.(*testFailDBSuite).TestHalfwayCancelOperations()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/ddl/fail_db_test.go:125 +0x1adb
  github.com/pingcap/tidb/ddl_test.(*testFailDBSuite).TestHalfwayCancelOperations()

 github.com/pingcap/tidb/ddl_test.(*testFailDBSuite).TestHalfwayCancelOperations()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/ddl/fail_db_test.go:69 +0x70
  runtime.call32()
      /usr/local/go/src/runtime/asm_amd64.s:522 +0x3a
  reflect.Value.Call()
      /usr/local/go/src/reflect/value.go:308 +0xc0
  github.com/pingcap/check.(*suiteRunner).forkTest.func1()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/check@v0.0.0-20171206051426-1c287c953996/check.go:798 +0xa04
  github.com/pingcap/check.(*suiteRunner).forkCall.func1()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/check@v0.0.0-20171206051426-1c287c953996/check.go:692 +0x89

Previous read at 0x00c0001d4c70 by goroutine 179:
  github.com/pingcap/tidb/domain.(*Domain).Reload()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/domain/domain.go:304 +0x151
  github.com/pingcap/tidb/domain.(*Domain).loadSchemaInLoop()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/domain/domain.go:401 +0x7f1

What is changed and how it works?

Data race error when concurrently accessing do.infoHandle in this PR test.
So here is the atomic pointer operation on do.infoHandle.

Check List

Tests

Unit test

This change is

ciscoxll · 2018-11-12T17:25:03Z

@tiancaiamao @winkyao @zimulala PTAL

ciscoxll · 2018-11-12T17:48:31Z

/run-all-tests

ciscoxll · 2018-11-13T02:16:59Z

/run-all-tests

tiancaiamao · 2018-11-13T02:53:00Z

domain/domain.go

@@ -535,7 +535,8 @@ func NewDomain(store kv.Storage, ddlLease time.Duration, statsLease time.Duratio

 // ResetHandle resets the domain's infoschema handle. It is used for testing.
 func (do *Domain) ResetHandle(store kv.Storage) {


I think it's better to remove this function, and call do.Reload() instead
@zimulala @ciscoxll

Call do.Reload still has data race.

This function is used to reset the handle. Reload can't do it.

So why it's needed to reset the handle? @zimulala
do.Reload uses mutex.Lock @winkyao

It's used for testing. You can see it in "TestHalfwayCancelOperations". @tiancaiamao

@zimulala What do you mean?

@zimulala @tiancaiamao Are we going to continue using s.dom.ResetHandle?

@winkyao @ciscoxll
I mean we can change test. Maybe we can remove ResetHandle.
At most, the test results are not so good.

@zimulala modified test failed to pass.

domain/domain.go

ciscoxll · 2018-11-13T12:34:34Z

/run-all-tests

winkyao · 2018-11-14T02:26:07Z

domain/domain.go

 // InfoSchema gets information schema from domain.
 func (do *Domain) InfoSchema() infoschema.InfoSchema {
-	return do.infoHandle.Get()


All the read at do.infoHandle need to be protected.

@winkyao Done.

ciscoxll · 2018-11-14T03:37:48Z

/run-all-tests

winkyao · 2018-11-14T04:04:02Z

@XuHuaiyu Will fix the union test failed, @ciscoxll you can just use the command /run-integration-common-test, to rerun the common-test

winkyao · 2018-11-14T04:05:45Z

/run-integration-common-test

winkyao

LGTM

XuHuaiyu · 2018-11-14T10:58:41Z

/run-common-test tidb-test=pr/649
/run-integration-common-test tidb-test=pr/649

winkyao · 2018-11-14T11:45:03Z

This PR need to be cherry-pick to 2.1 and 2.0

tiancaiamao · 2018-11-16T05:09:21Z

@zimulala @crazycs520

ciscoxll · 2018-11-19T03:53:10Z

@zimulala @tiancaiamao @crazycs520 PTAL

ciscoxll · 2018-11-26T16:42:11Z

/run-all-tests

ciscoxll · 2018-11-27T02:52:46Z

/run-all-tests

ddl/fail_db_test.go

ciscoxll · 2018-11-27T05:22:24Z

/run-all-tests

winkyao

LGTM

winkyao · 2018-11-27T07:35:25Z

/run-common-test tidb-test=pr/668

winkyao · 2018-11-27T07:35:37Z

/run-integration-common-test tidb-test=pr/668

ddl/fail_db_test.go

crazycs520

LGTM

tiancaiamao · 2018-11-27T13:04:48Z

LGTM

ciscoxll added the component/DDL-need-LGT3 label Nov 12, 2018

ciscoxll changed the title ~~Fix concurrent access to do.infoHandle has a data race error~~ ddl: fix concurrent access to do.infoHandle has a data race error Nov 12, 2018

tiancaiamao reviewed Nov 13, 2018

View reviewed changes

ciscoxll force-pushed the fix-data-race branch from 36ad682 to 35bbf56 Compare November 13, 2018 04:12

pingcap deleted a comment from winkyao Nov 13, 2018

tiancaiamao mentioned this pull request Nov 13, 2018

docs/design: add the table partition proposal #7969

Merged

ciscoxll force-pushed the fix-data-race branch from 6b02ccf to ce4d765 Compare November 13, 2018 05:29

crazycs520 reviewed Nov 13, 2018

View reviewed changes

domain/domain.go Outdated Show resolved Hide resolved

Address comment

435ebc5

ciscoxll force-pushed the fix-data-race branch from 8bc3405 to 435ebc5 Compare November 13, 2018 09:00

ciscoxll and others added 2 commits November 13, 2018 17:05

Change function comment

75aeb56

Merge branch 'master' into fix-data-race

7c8314d

ciscoxll added the status/all tests passed label Nov 13, 2018

winkyao reviewed Nov 14, 2018

View reviewed changes

ciscoxll added 2 commits November 14, 2018 10:38

Address comment

ad6c842

Fix CI

7d12d89

winkyao reviewed Nov 14, 2018

View reviewed changes

ciscoxll added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 14, 2018

tiancaiamao added the priority/non-release-blocker label Nov 14, 2018

ciscoxll force-pushed the fix-data-race branch from a79fad4 to 1cbd0a3 Compare November 26, 2018 13:07

ciscoxll added 2 commits November 26, 2018 21:26

Test go fail

05fb27b

Fix CI

b57c7b9

winkyao reviewed Nov 27, 2018

View reviewed changes

ddl/fail_db_test.go Outdated Show resolved Hide resolved

Address comment

c46e366

ciscoxll force-pushed the fix-data-race branch from efb6c71 to c46e366 Compare November 27, 2018 03:15

winkyao reviewed Nov 27, 2018

View reviewed changes

zz-jason added the priority/P1 The issue has P1 priority. label Nov 27, 2018

zz-jason reviewed Nov 27, 2018

View reviewed changes

ddl/fail_db_test.go Outdated Show resolved Hide resolved

ddl/fail_db_test.go Outdated Show resolved Hide resolved

Address comment

74ccab6

ciscoxll force-pushed the fix-data-race branch from 77e25cc to 74ccab6 Compare November 27, 2018 08:06

crazycs520 reviewed Nov 27, 2018

View reviewed changes

crazycs520 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 27, 2018

Merge branch 'master' into fix-data-race

e59f644

tiancaiamao approved these changes Nov 27, 2018

View reviewed changes

tiancaiamao added require-LGT3 Indicates that the PR requires three LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Nov 27, 2018

ciscoxll merged commit e5dc251 into pingcap:master Nov 27, 2018

ciscoxll deleted the fix-data-race branch November 27, 2018 14:14

This was referenced Nov 27, 2018

ddl: fix concurrent access to do.infoHandle has a data race error #8481

Merged

ddl: fix concurrent access to do.infoHandle has a data race error #8496

Merged

sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: fix concurrent access to `do.infoHandle` has a data race error #8287

ddl: fix concurrent access to `do.infoHandle` has a data race error #8287

ciscoxll commented Nov 12, 2018 •

edited by c4pt0r

Loading

ciscoxll commented Nov 12, 2018

ciscoxll commented Nov 12, 2018

ciscoxll commented Nov 13, 2018

tiancaiamao Nov 13, 2018

winkyao Nov 13, 2018

zimulala Nov 13, 2018

tiancaiamao Nov 13, 2018

zimulala Nov 13, 2018

zimulala Nov 14, 2018

winkyao Nov 15, 2018

ciscoxll Nov 22, 2018

zimulala Nov 22, 2018

ciscoxll Nov 22, 2018

ciscoxll commented Nov 13, 2018

winkyao Nov 14, 2018

ciscoxll Nov 14, 2018

ciscoxll commented Nov 14, 2018

winkyao commented Nov 14, 2018 •

edited

Loading

winkyao commented Nov 14, 2018

winkyao left a comment

XuHuaiyu commented Nov 14, 2018

winkyao commented Nov 14, 2018

tiancaiamao commented Nov 16, 2018

ciscoxll commented Nov 19, 2018

ciscoxll commented Nov 26, 2018

ciscoxll commented Nov 27, 2018

ciscoxll commented Nov 27, 2018

winkyao left a comment

winkyao commented Nov 27, 2018

winkyao commented Nov 27, 2018

crazycs520 left a comment

tiancaiamao commented Nov 27, 2018

		@@ -535,7 +535,8 @@ func NewDomain(store kv.Storage, ddlLease time.Duration, statsLease time.Duratio

		// ResetHandle resets the domain's infoschema handle. It is used for testing.
		func (do *Domain) ResetHandle(store kv.Storage) {

ddl: fix concurrent access to do.infoHandle has a data race error #8287

ddl: fix concurrent access to do.infoHandle has a data race error #8287

Conversation

ciscoxll commented Nov 12, 2018 • edited by c4pt0r Loading

What problem does this PR solve?

What is changed and how it works?

Check List

ciscoxll commented Nov 12, 2018

ciscoxll commented Nov 12, 2018

ciscoxll commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciscoxll commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciscoxll commented Nov 14, 2018

winkyao commented Nov 14, 2018 • edited Loading

winkyao commented Nov 14, 2018

winkyao left a comment

Choose a reason for hiding this comment

XuHuaiyu commented Nov 14, 2018

winkyao commented Nov 14, 2018

tiancaiamao commented Nov 16, 2018

ciscoxll commented Nov 19, 2018

ciscoxll commented Nov 26, 2018

ciscoxll commented Nov 27, 2018

ciscoxll commented Nov 27, 2018

winkyao left a comment

Choose a reason for hiding this comment

winkyao commented Nov 27, 2018

winkyao commented Nov 27, 2018

crazycs520 left a comment

Choose a reason for hiding this comment

tiancaiamao commented Nov 27, 2018

ddl: fix concurrent access to `do.infoHandle` has a data race error #8287

ddl: fix concurrent access to `do.infoHandle` has a data race error #8287

ciscoxll commented Nov 12, 2018 •

edited by c4pt0r

Loading

winkyao commented Nov 14, 2018 •

edited

Loading