bindinfo: fix data race of parser in global handle (#10321)

pingcap · Apr 30, 2019 · cf0ca74 · cf0ca74
1 parent efe9b6a
commit cf0ca74
Show file tree

Hide file tree

Showing 4 changed files with 16 additions and 18 deletions.
diff --git a/bindinfo/bind_test.go b/bindinfo/bind_test.go
@@ -117,7 +117,7 @@ func (s *testSuite) TestBindParse(c *C) {
 	sql := fmt.Sprintf(`INSERT INTO mysql.bind_info(original_sql,bind_sql,default_db,status,create_time,update_time,charset,collation) VALUES ('%s', '%s', '%s', '%s', NOW(), NOW(),'%s', '%s')`,
 		originSQL, bindSQL, defaultDb, status, charset, collation)
 	tk.MustExec(sql)
-	bindHandle := bindinfo.NewBindHandle(tk.Se, s.Parser)
+	bindHandle := bindinfo.NewBindHandle(tk.Se)
 	err := bindHandle.Update(true)
 	c.Check(err, IsNil)
 	c.Check(bindHandle.Size(), Equals, 1)
@@ -178,7 +178,7 @@ func (s *testSuite) TestGlobalBinding(c *C) {
 	c.Check(row.GetString(6), NotNil)
 	c.Check(row.GetString(7), NotNil)
 
-	bindHandle := bindinfo.NewBindHandle(tk.Se, s.Parser)
+	bindHandle := bindinfo.NewBindHandle(tk.Se)
 	err = bindHandle.Update(true)
 	c.Check(err, IsNil)
 	c.Check(bindHandle.Size(), Equals, 1)
@@ -199,7 +199,7 @@ func (s *testSuite) TestGlobalBinding(c *C) {
 	bindData = s.domain.BindHandle().GetBindRecord("select * from t where i > ?", "test")
 	c.Check(bindData, IsNil)
 
-	bindHandle = bindinfo.NewBindHandle(tk.Se, s.Parser)
+	bindHandle = bindinfo.NewBindHandle(tk.Se)
 	err = bindHandle.Update(true)
 	c.Check(err, IsNil)
 	c.Check(bindHandle.Size(), Equals, 0)

diff --git a/bindinfo/handle.go b/bindinfo/handle.go
@@ -59,6 +59,7 @@ type BindHandle struct {
 	bindInfo struct {
 		sync.Mutex
 		atomic.Value
+		parser *parser.Parser
 	}
 
 	// invalidBindRecordMap indicates the invalid bind records found during querying.
@@ -68,7 +69,6 @@ type BindHandle struct {
 		atomic.Value
 	}
 
-	parser         *parser.Parser
 	lastUpdateTime types.Time
 }
 
@@ -78,10 +78,11 @@ type invalidBindRecordMap struct {
 }
 
 // NewBindHandle creates a new BindHandle.
-func NewBindHandle(ctx sessionctx.Context, parser *parser.Parser) *BindHandle {
-	handle := &BindHandle{parser: parser}
+func NewBindHandle(ctx sessionctx.Context) *BindHandle {
+	handle := &BindHandle{}
 	handle.sctx.Context = ctx
 	handle.bindInfo.Value.Store(make(cache, 32))
+	handle.bindInfo.parser = parser.New()
 	handle.invalidBindRecordMap.Value.Store(make(map[string]*invalidBindRecordMap))
 	return handle
 }
@@ -151,14 +152,18 @@ func (h *BindHandle) AddBindRecord(record *BindRecord) (err error) {
 			return
 		}
 
+		// Make sure there is only one goroutine writes the cache and use parser.
+		h.bindInfo.Lock()
 		// update the BindMeta to the cache.
 		hash, meta, err1 := h.newBindMeta(record)
 		if err1 != nil {
 			err = err1
+			h.bindInfo.Unlock()
 			return
 		}
 
 		h.appendBindMeta(hash, meta)
+		h.bindInfo.Unlock()
 	}()
 
 	// remove all the unused sql binds.
@@ -294,7 +299,7 @@ func (h *BindHandle) GetAllBindRecord() (bindRecords []*BindMeta) {
 
 func (h *BindHandle) newBindMeta(record *BindRecord) (hash string, meta *BindMeta, err error) {
 	hash = parser.DigestHash(record.OriginalSQL)
-	stmtNodes, _, err := h.parser.Parse(record.BindSQL, record.Charset, record.Collation)
+	stmtNodes, _, err := h.bindInfo.parser.Parse(record.BindSQL, record.Charset, record.Collation)
 	if err != nil {
 		return "", nil, err
 	}
@@ -311,16 +316,10 @@ func newBindMetaWithoutAst(record *BindRecord) (hash string, meta *BindMeta) {
 // appendBindMeta addes the BindMeta to the cache, all the stale bindMetas are
 // removed from the cache after this operation.
 func (h *BindHandle) appendBindMeta(hash string, meta *BindMeta) {
-	// Make sure there is only one goroutine writes the cache.
-	h.bindInfo.Lock()
 	newCache := h.bindInfo.Value.Load().(cache).copy()
-	defer func() {
-		h.bindInfo.Value.Store(newCache)
-		h.bindInfo.Unlock()
-	}()
-
 	newCache.removeStaleBindMetas(hash, meta)
 	newCache[hash] = append(newCache[hash], meta)
+	h.bindInfo.Value.Store(newCache)
 }
 
 // removeBindMeta removes the BindMeta from the cache.

diff --git a/domain/domain.go b/domain/domain.go
@@ -26,7 +26,6 @@ import (
 	"github.com/ngaut/sync2"
 	"github.com/pingcap/errors"
 	"github.com/pingcap/failpoint"
-	"github.com/pingcap/parser"
 	"github.com/pingcap/parser/ast"
 	"github.com/pingcap/parser/model"
 	"github.com/pingcap/parser/terror"
@@ -784,9 +783,9 @@ func (do *Domain) BindHandle() *bindinfo.BindHandle {
 
 // LoadBindInfoLoop create a goroutine loads BindInfo in a loop, it should
 // be called only once in BootstrapSession.
-func (do *Domain) LoadBindInfoLoop(ctx sessionctx.Context, parser *parser.Parser) error {
+func (do *Domain) LoadBindInfoLoop(ctx sessionctx.Context) error {
 	ctx.GetSessionVars().InRestrictedSQL = true
-	do.bindHandle = bindinfo.NewBindHandle(ctx, parser)
+	do.bindHandle = bindinfo.NewBindHandle(ctx)
 	err := do.bindHandle.Update(true)
 	if err != nil {
 		return err

diff --git a/session/session.go b/session/session.go
@@ -1466,7 +1466,7 @@ func BootstrapSession(store kv.Storage) (*domain.Domain, error) {
 	if err != nil {
 		return nil, err
 	}
-	err = dom.LoadBindInfoLoop(se2, se2.parser)
+	err = dom.LoadBindInfoLoop(se2)
 	if err != nil {
 		return nil, err
 	}