*: implement auto analyze #4141

alivxxx · 2017-08-10T11:54:56Z

This PR implements the auto analyze.

It uses the OwnerManager to choose the stats owner, and only the stats owner can do auto analyze.
It will only auto analyze for the newly created table and index. For the table, only after 20*statsLease with no insert, the analyze operation will be triggered. For the index, we will analyze immediately after we know it's existence.
To reduce the impact on the system, it will only analyze only one table or index in one time. Also, the priority of the analyze is set to low priority.

PTAL @hanfei1991 @winoros

alivxxx · 2017-08-10T11:57:56Z

Currently, if the statsLease is 0, there will be no auto analyze. Wondering if it needs a separate parameter to control it.

zimulala · 2017-08-14T04:41:46Z

ddl/ddl.go

@@ -273,7 +273,7 @@ func newDDL(ctx goctx.Context, etcdCli *clientv3.Client, store kv.Storage,
 		manager = NewMockOwnerManager(id, cancelFunc)
 		syncer = NewMockSchemaSyncer()
 	} else {
-		manager = NewOwnerManager(etcdCli, id, cancelFunc)


Add ddl as a constant.

zimulala · 2017-08-14T04:43:01Z

ddl/owner_manager.go

 }

 // NewOwnerManager creates a new OwnerManager.
-func NewOwnerManager(etcdCli *clientv3.Client, id string, cancel goctx.CancelFunc) OwnerManager {
+func NewOwnerManager(etcdCli *clientv3.Client, prompt string, id string, key string, cancel goctx.CancelFunc) OwnerManager {


s/prompt string, id string, key string/prompt, id, key string

zimulala · 2017-08-14T04:45:47Z

domain/domain.go

+		if do.etcdClient == nil {
+			statsOwner = ddl.NewMockOwnerManager(id, cancelFunc)
+		} else {
+			statsOwner = ddl.NewOwnerManager(do.etcdClient, "stats", id, statistics.StatsOwnerKey, cancelFunc)


The same as stats.

zimulala · 2017-08-14T04:47:13Z

domain/domain.go

+		} else {
+			statsOwner = ddl.NewOwnerManager(do.etcdClient, "stats", id, statistics.StatsOwnerKey, cancelFunc)
+		}
+		statsOwner.CampaignOwners(cancelCtx)


Do we need to check the error?

zimulala · 2017-08-14T04:49:07Z

domain/domain.go

+			if statsOwner.IsOwner() {
+				err := do.statsHandle.HandleAutoAnalyze(do.InfoSchema())
+				if err != nil {
+					log.Error(errors.ErrorStack(err))


Complete the error message. Add the package and function information. You can refer to other logs.

hanfei1991 · 2017-08-14T17:05:55Z

@lamxTyler Can we write a owner pakcage independent of ddl package ?

zimulala · 2017-08-15T02:37:33Z

ddl/owner_manager.go

-			log.Infof("[ddl] %s etcd session is done, creates a new one", idInfo)
-			etcdSession, err = newSession(ctx, idInfo, m.etcdCli, newSessionRetryUnlimited, ManagerSessionTTL)
+			log.Infof("[%s] %s etcd session is done, creates a new one", m.prompt, idInfo)
+			etcdSession, err = newSession(ctx, m.prompt, idInfo, m.etcdCli, newSessionRetryUnlimited, ManagerSessionTTL)


Can we use a log prefix instead of m.prompt and idInfo here? We can pass an argument.

zimulala · 2017-08-15T02:41:12Z

ddl/owner_manager.go

@@ -260,10 +259,3 @@ func (m *ownerManager) watchOwner(ctx goctx.Context, etcdSession *concurrency.Se
 		}
 	}
 }
-
-func init() {
-	err := setManagerSessionTTL()


Why remove this ？

I search the project and find that no one uses it. Need to add back?

zimulala · 2017-08-15T02:41:54Z

ddl/syncer.go

@@ -123,7 +123,7 @@ func (s *schemaVersionSyncer) Init(ctx goctx.Context) error {
 	if err != nil {
 		return errors.Trace(err)
 	}
-	s.session, err = newSession(ctx, s.selfSchemaVerPath, s.etcdCli,
+	s.session, err = newSession(ctx, "ddl", s.selfSchemaVerPath, s.etcdCli,


Use ddlPrompt directly.

zimulala · 2017-08-15T02:42:01Z

ddl/syncer.go

@@ -140,7 +140,7 @@ func (s *schemaVersionSyncer) Done() <-chan struct{} {

 // Restart implements SchemaSyncer.Restart interface.
 func (s *schemaVersionSyncer) Restart(ctx goctx.Context) error {
-	session, err := newSession(ctx, s.selfSchemaVerPath, s.etcdCli,
+	session, err := newSession(ctx, "ddl", s.selfSchemaVerPath, s.etcdCli,


zimulala · 2017-08-15T02:44:27Z

domain/domain.go

@@ -575,6 +575,29 @@ func (do *Domain) UpdateTableStatsLoop(ctx context.Context) error {
 			}
 		}
 	}(do)
+	go func(do *Domain) {


This function is too long. Can we extract this logic into a function?

zimulala · 2017-08-15T02:48:43Z

domain/domain.go

+			statsOwner = ddl.NewOwnerManager(do.etcdClient, statistics.StatsPrompt, id, statistics.StatsOwnerKey, cancelFunc)
+		}
+		err := statsOwner.CampaignOwner(cancelCtx)
+		if err != nil {


If we campaign failed, it may be not an owner always. Do we need to do something to handle it?

As long as there is at most one owner, it will be ok.

If all TiDB are failed, then what should we do？

Then there will be no auto analyze since there is no owner. Or do we have better solutions?

zimulala · 2017-08-15T02:54:54Z

statistics/update.go

+	return true
+}
+
+// HandleAutoAnalyze analyze the newly created table or index.


s/analyze/analyzes

zimulala · 2017-08-15T02:58:29Z

statistics/update.go

+				sql := fmt.Sprintf("analyze table %s", tblName)
+				log.Infof("[stats] auto analyze table %s now", tblName)
+				_, _, err := h.ctx.(sqlexec.RestrictedSQLExecutor).ExecRestrictedSQL(h.ctx, sql)
+				return errors.Trace(err)


Do we need to analyze the next table? Here we only print the log?

Nope, currently every time we only analyze one table or index, to reduce the impact on the server.

Only the log? It also analyzes the table.

About the first point of view，you use db.Tables and haven't a break after handling one table or index, so how do we do it?

There is a return after the execute?

hanfei1991 · 2017-08-15T07:33:19Z

@lamxTyler I think owner is not a kind of "util". We'd better isolate this package.

zimulala · 2017-08-16T07:37:10Z

ddl/syncer.go

@@ -34,6 +35,9 @@ const (
 	// DDLGlobalSchemaVersion is the path on etcd that is used to store the latest schema versions.
 	// It's exported for testing.
 	DDLGlobalSchemaVersion = "/tidb/ddl/global_schema_version"
+	// DDLOwnerKey is the ddl owner path that is saved to etcd, and it's exported for testing.
+	DDLOwnerKey = "/tidb/ddl/fg/owner"
+	ddlPrompt   = "ddl"


It's better to put this code into ddl.go.

zimulala · 2017-08-16T07:40:17Z

owner/manager.go

-// OwnerManager is used to campaign the owner and manage the owner information.
-type OwnerManager interface {
-	// ID returns the ID of DDL.
+// Manager is used to campaign the owner and manage the owner information.


Why change this name?

Because the package's name is owner, so it will make golint unhappy if the names' prefix is owner.

zimulala · 2017-08-16T07:45:58Z

statistics/update.go

+				sql := fmt.Sprintf("analyze table %s", tblName)
+				log.Infof("[stats] auto analyze table %s now", tblName)
+				_, _, err := h.ctx.(sqlexec.RestrictedSQLExecutor).ExecRestrictedSQL(h.ctx, sql)
+				return errors.Trace(err)


hanfei1991 · 2017-08-17T06:52:38Z

statistics/update.go

+		for _, tbl := range tbls {
+			tblInfo := tbl.Meta()
+			statsTbl := h.GetTableStats(tblInfo.ID)
+			if statsTbl.Pseudo || statsTbl.Count == 0 {


We should analyze table with least version firstly. We can use a 'select table_id from stats_meta where modify_count >= count and count >= 1000 order by version limit 1.

If a table is deleted, it's count is not zero but histogram is empty. We shouldn't analyze it. If its version is old enough, we can delete it.

First, There is no direct mapping from table_id to table_name, which would make it hard to produce the analyze statement. Second, modify_count >= count does not ensure that this is a newly created table. Third, Since I will implement a job queue, the job queue could be a priority queue, which I think is a better solution.

Well, I did not notice it before. A deleted table has non-zero count is unreasonable. So, let's directly make the deleted table's count 0.

you can refine 1 in the future. And currently the deleted table is not with count 0, so we still need to process this case. Owner's grabage collection is also nessesary.

Make the deleted table 0 could be done in this PR. Garbage collection is not relevant with auto analyze, so I will do it in another PR.

Also, even if the deleted table's count is not 0, we do not need to process it, because if it is deleted, then it will not be in the info schema or the stats cache.

hanfei1991 · 2017-08-17T12:24:04Z

LGTM

zimulala · 2017-08-18T03:19:55Z

owner/manager.go

@@ -256,9 +240,11 @@ func (m *ownerManager) watchOwner(ctx goctx.Context, etcdSession *concurrency.Se
 	}
 }

-func init() {
-	err := setManagerSessionTTL()


Why remove this function？ It's useful.

But it is not used?

It's used in the test.

zimulala · 2017-08-18T04:16:03Z

domain/domain.go

+	} else {
+		statsOwner = owner.NewOwnerManager(do.etcdClient, statistics.StatsPrompt, id, statistics.StatsOwnerKey, cancelFunc)
+	}
+	err := statsOwner.CampaignOwner(cancelCtx)


I think we need to do something when err != nil.

The network between PD and TiDB is unstable

Try until the err is not nil?

It‘s better to put it out of a goroutine. If it hard to handle, you can add a TODO here.

OK, I add a TODO here.

zimulala · 2017-08-18T04:21:34Z

statistics/update_test.go

@@ -223,3 +223,50 @@ func (s *testStatsUpdateSuite) TestTxnWithFailure(c *C) {
 	stats1 = h.GetTableStats(tableInfo1.ID)
 	c.Assert(stats1.Count, Equals, int64(rowCount1+1))
 }
+
+func (s *testStatsUpdateSuite) TestAutoUpdate(c *C) {
+	store, do, err := newStoreWithBootstrap()


Can we use a unified store in testStatsUpdateSuite? Bootstrap make the test run a long time.

Do it in another pr? There are lots of bootstrap in this package.

zimulala · 2017-08-18T04:29:22Z

owner/manager.go

+	}
+}
+
+func isContextDone(ctx goctx.Context) bool {


Is this function useless?

Nope. In fact, this function also exists in DDL package, but it seems awkward to export it here. Any advice?

It's OK now.

zimulala · 2017-08-18T04:30:40Z

domain/domain.go

+	} else {
+		statsOwner = owner.NewOwnerManager(do.etcdClient, statistics.StatsPrompt, id, statistics.StatsOwnerKey, cancelFunc)
+	}
+	err := statsOwner.CampaignOwner(cancelCtx)


The network between PD and TiDB is unstable

zimulala · 2017-08-18T10:30:20Z

LGTM
Please resolve the conflict. @lamxTyler

alivxxx · 2017-08-18T10:39:57Z

@zimulala Resolved.

Haibin Xie added 6 commits August 10, 2017 19:28

*: implement auto analyze

22d3419

Merge branch 'master' of github.com:pingcap/tidb into xhb/auto

db1d979

rm debug file

0e683d6

tiny fix

1d837b0

tiny fix

4e30ada

Merge branch 'xhb/auto' of github.com:pingcap/tidb into xhb/auto

e7fc3ef

zimulala reviewed Aug 14, 2017

View reviewed changes

Haibin Xie added 3 commits August 14, 2017 13:45

Merge branch 'master' of github.com:pingcap/tidb into xhb/auto

ce569f5

address comment

2216f2b

fix race

d89d2ee

zimulala reviewed Aug 15, 2017

View reviewed changes

Haibin Xie added 2 commits August 15, 2017 15:18

address comment

64d9663

fix golint

2688976

Haibin Xie added 3 commits August 15, 2017 15:39

address comment

6410708

export managerSessionTTL

b176156

rename file

ff5513b

zimulala reviewed Aug 16, 2017

View reviewed changes

address comment

f2c51ab

hanfei1991 reviewed Aug 17, 2017

View reviewed changes

Haibin Xie added 2 commits August 17, 2017 19:13

Merge branch 'master' of github.com:pingcap/tidb into xhb/auto

20707cf

Merge branch 'master' of github.com:pingcap/tidb into xhb/auto

5bcf6ae

hanfei1991 added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 17, 2017

zimulala reviewed Aug 18, 2017

View reviewed changes

add back functions

58b87a5

zimulala reviewed Aug 18, 2017

View reviewed changes

add todo

d400708

zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 18, 2017

zimulala previously approved these changes Aug 18, 2017

View reviewed changes

Merge branch 'master' of github.com:pingcap/tidb into xhb/auto

f3a6191

alivxxx dismissed zimulala’s stale review via f3a6191 August 18, 2017 10:33

zimulala approved these changes Aug 18, 2017

View reviewed changes

alivxxx merged commit 7c273c0 into master Aug 18, 2017

alivxxx deleted the xhb/auto branch August 18, 2017 10:42

*: implement auto analyze #4141

*: implement auto analyze #4141

Conversation

alivxxx commented Aug 10, 2017

alivxxx commented Aug 10, 2017

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

hanfei1991 commented Aug 14, 2017

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

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

hanfei1991 commented Aug 15, 2017

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

alivxxx Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

hanfei1991 Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanfei1991 commented Aug 17, 2017

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

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

zimulala commented Aug 18, 2017

alivxxx commented Aug 18, 2017

alivxxx Aug 17, 2017 •

edited

Loading

hanfei1991 Aug 17, 2017 •

edited

Loading