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.

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

ddl: fix parallel create table if not exists #6286

Merged
merged 10 commits into from
Apr 16, 2018
5 changes: 5 additions & 0 deletions ddl/ddl_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,11 @@ func (d *ddl) CreateTable(ctx sessionctx.Context, s *ast.CreateTableStmt) (err e
err = d.handleAutoIncID(tbInfo, schema.ID)
}
}

// table exists, but if_not_exits flags is true, so we ignore this error.
Copy link
Contributor

Choose a reason for hiding this comment

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

if_not_exists

if infoschema.ErrTableExists.Equal(err) && s.IfNotExists {
return nil
}
err = d.callHookOnChanged(err)
return errors.Trace(err)
}
Expand Down
86 changes: 83 additions & 3 deletions ddl/ddl_db_change_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,6 @@ func (s *testStateChangeSuite) TestShowIndex(c *C) {
}

func (s *testStateChangeSuite) TestParallelDDL(c *C) {
defer testleak.AfterTest(c)()
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Contributor Author

@winkyao winkyao Apr 15, 2018

Choose a reason for hiding this comment

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

It's called in testStateChangeSuite.TeareDownSuite, call testleak.AfterTest(c) here make test unstable.

_, err := s.se.Execute(context.Background(), "use test_db_state")
c.Assert(err, IsNil)
_, err = s.se.Execute(context.Background(), "create table t(a int, b int, c int)")
Expand Down Expand Up @@ -478,14 +477,13 @@ func (s *testStateChangeSuite) TestParallelDDL(c *C) {
c.Assert(err, IsNil)
_, err = se1.Execute(context.Background(), "use test_db_state")
c.Assert(err, IsNil)
wg.Add(2)
go func() {
wg.Add(1)
defer wg.Done()
_, err1 = se.Execute(context.Background(), "ALTER TABLE t MODIFY COLUMN b int FIRST;")
}()

go func() {
wg.Add(1)
defer wg.Done()
_, err2 = se1.Execute(context.Background(), "ALTER TABLE t MODIFY COLUMN b int FIRST;")
}()
Expand All @@ -501,3 +499,85 @@ func (s *testStateChangeSuite) TestParallelDDL(c *C) {
callback = &ddl.TestDDLCallback{}
d.SetHook(callback)
}

func (s *testStateChangeSuite) TestCreateIfNotExists(c *C) {
Copy link
Member

Choose a reason for hiding this comment

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

Add some comments for the test case.

Copy link
Contributor

Choose a reason for hiding this comment

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

s/TestCreateIfNotExists/TestCreateTableIfNotExists

callback := &ddl.TestDDLCallback{}
once := sync.Once{}
callback.OnJobUpdatedExported = func(job *model.Job) {
// sleep a while, let other job enque.
once.Do(func() {
time.Sleep(time.Millisecond * 10)
})
}

defer s.se.Execute(context.Background(), "drop table test_not_exists")

se, err := session.CreateSession(s.store)
_, err = se.Execute(context.Background(), "use test_db_state")
c.Assert(err, IsNil)

se1, err1 := session.CreateSession(s.store)
_, err1 = se1.Execute(context.Background(), "use test_db_state")
c.Assert(err1, IsNil)

d := s.dom.DDL()
d.SetHook(callback)

var err2, err3 error
wg := sync.WaitGroup{}
wg.Add(2)
go func() {
defer wg.Done()
_, err2 = se.Execute(context.Background(), "create table if not exists test_not_exists(a int);")
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is similar to the next test, only the SQL is different. And we will add some test like this. Could this be extracted into a function?

}()

go func() {
defer wg.Done()
_, err3 = se1.Execute(context.Background(), "create table if not exists test_not_exists(a int);")
}()
wg.Wait()
c.Assert(err2, IsNil)
c.Assert(err3, IsNil)
callback = &ddl.TestDDLCallback{}
d.SetHook(callback)
}

func (s *testStateChangeSuite) TestCreateIfNotExists1(c *C) {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

/TestCreateIfNotExists1/TestCreateDBIfNotExists

callback := &ddl.TestDDLCallback{}
once := sync.Once{}
callback.OnJobUpdatedExported = func(job *model.Job) {
// sleep a while, let other job enque.
Copy link
Contributor

Choose a reason for hiding this comment

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

How can we confirm that the other job is in the queue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this PR, the added test failed stably.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this test code is indeed unstable.

once.Do(func() {
time.Sleep(time.Millisecond * 10)
})
}
defer s.se.Execute(context.Background(), "drop database test_not_exists")

se, err := session.CreateSession(s.store)
c.Assert(err, IsNil)

se1, err1 := session.CreateSession(s.store)
c.Assert(err1, IsNil)

var err2, err3 error
wg := sync.WaitGroup{}

d := s.dom.DDL()
d.SetHook(callback)

wg.Add(2)
go func() {
defer wg.Done()
_, err2 = se.Execute(context.Background(), "create database if not exists test_not_exists;")
}()

go func() {
defer wg.Done()
_, err3 = se1.Execute(context.Background(), "create database if not exists test_not_exists;")
}()
wg.Wait()
c.Assert(err2, IsNil)
c.Assert(err3, IsNil)
callback = &ddl.TestDDLCallback{}
d.SetHook(callback)
}