statistics: refactor package. #2913

hanfei1991 · 2017-03-22T11:37:48Z

change the way of store stats info.

shenli · 2017-03-22T14:01:07Z

We should find a safe way to do DDL in bootstrap stage.

hanfei1991 · 2017-03-24T06:14:28Z

@lamxTyler PTAL

alivxxx · 2017-03-26T02:26:35Z

statistics/statscache.go

-				tbl = statistics.PseudoTable(tableInfo)
-			}
-			SetStatisticsTableCache(tableID, tbl, version)
+			log.Errorf("Error occured when convert pb table for table id %d", tableID)


This log should be updated.

alivxxx · 2017-03-26T02:28:33Z

statistics/statistics.go

+		return errors.Trace(err)
+	}
+	for _, col := range t.Columns {
+		col.saveToStorage(ctx, t.Info.ID, false)


Why ignore the error?

alivxxx · 2017-03-26T02:42:34Z

bootstrap.go

+		index tbl(table_id)
+	);`
+
+	// CreateStatsBucketsTable stores the histogram info for every table columns.


Need to point out that the count is not the prefix sum?

alivxxx · 2017-03-26T02:47:51Z

statistics/statistics.go

@@ -68,6 +68,43 @@ type Column struct {
 	Repeats []int64


The comment in line 63-65 can be removed? Because we do not serialize it now.

alivxxx · 2017-03-26T02:59:42Z

statistics/statistics.go

-		if err != nil {
-			return nil, errors.Trace(err)
-		}
+func colStatsFromStorage(ctx context.Context, tableID int64, colID int64, tp *types.FieldType, distinct int64, isIndex bool) (*Column, error) {


Why sperate the function (c *Column) saveToStorage and this function? It is better to put them in the same place as the Table do.

alivxxx · 2017-03-26T03:07:35Z

statistics/statistics.go

-			return nil, errors.Trace(err)
+	bucketSize := len(rows)
+	if bucketSize == 0 {
+		return nil, nil


Why return nil? Even if it is an empty table, currently it will have at least 1 bucket. bucketSize is 0 may mean that this row is added after analyze.

alivxxx · 2017-03-26T03:09:37Z

statistics/statistics.go

+			if err != nil {
+				return nil, errors.Trace(err)
+			}
+			table.Indices = append(table.Indices, col)


This should also check if this index is deleted.

alivxxx · 2017-03-26T03:10:22Z

statistics/statistics.go

+			}
+		}
+	}
+	return table, nil


What if there are newly added columns?

I think that load table from storage in this way needs too much check. May be it is better to iterate through the table info to load stats.

hanfei1991 · 2017-03-27T07:04:48Z

@shenli It's safe to create table when bootstrap.

alivxxx · 2017-03-27T07:54:53Z

statistics/statistics.go

+			var col *Column
+			for _, idxInfo := range info.Indices {
+				if idxID == idxInfo.ID {
+					col, err = colStatsFromStorage(ctx, info.ID, idxID, nil, distinct, true)


If line 120 return nil, this means that this index do not have histograms as line 727 shows, as those who have histograms must have at least 1 bucket even if it is empty columns. So I suggest that line 120 return the pseudo cols, and update the comment in 727.

I will handle all problems like this in next PR.

alivxxx · 2017-03-27T08:27:59Z

statistics/statistics.go

+		}
+	}
+	if indexCount != len(info.Indices) {
+		return nil, errors.New("The number of indices doesn't match with the schema")


Does it need . at the end?
(update: golint said no.

alivxxx · 2017-03-27T08:28:56Z

statistics/statistics.go

+		} else {
+			count = c.Numbers[i] - c.Numbers[i-1]
+		}
+		// set value


add . at the end.

alivxxx · 2017-03-27T08:31:48Z

Rest LGTM.

alivxxx · 2017-03-27T09:00:41Z

LGTM

coocood · 2017-03-27T08:58:53Z

statistics/statistics.go

+		Info:  info,
+		Count: count,
+	}
+	selSQL := fmt.Sprintf("select * from mysql.stats_columns where table_id = %d", info.ID)


It would be easier to read and have better compatibility if we list the columns in the SQL.

coocood · 2017-03-27T09:32:24Z

bootstrap.go

+		count bigint(64) NOT NULL,
+		repeats bigint(64) NOT NULL,
+		value blob NOT NULL,
+		index tbl(table_id, col_id, index_id, bucket_id)


This can be unique.

coocood · 2017-03-27T09:33:25Z

bootstrap.go

+		distinct_ratio double(64) NOT NULL DEFAULT 0,
+		use_count_to_estimate tinyint(2) NOT NULL DEFAULT 0,
+		version bigint(64) unsigned NOT NULL DEFAULT 0,
+		index tbl(table_id)


unique tbl(table_id, col_id, index_id)?

zimulala · 2017-03-27T09:08:09Z

statistics/statistics.go

-	Indices []*Column
-	Count   int64 // Total row count in a table.
-	Pseudo  bool
+// TableStatsFromStorage load table stats info from storage.


s/load/loads

zimulala · 2017-03-27T09:08:26Z

statistics/statistics.go

+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	// indexCount and columnCount records the number of indices and columns in table stats. If the number don't match with


s/records/record

coocood · 2017-03-27T10:08:52Z

statistics/statistics.go

+	return nil
+}
+
+func colStatsFromStorage(ctx context.Context, tableID int64, colID int64, tp *types.FieldType, distinct int64, isIndex bool) (*Column, error) {


We should use different structs to represent column stats and index stats.
index stats may contain multiple columns.

This will be changed in next pr

coocood · 2017-03-27T10:39:58Z

statistics/statscache_test.go

+	testKit := testkit.NewTestKit(c, store)
+	testKit.MustExec("use test")
+	testKit.MustExec("create table t (c1 int, c2 int)")
+	for i := 0; i < 100000; i++ {


This test may take too much time.

coocood · 2017-03-27T10:45:31Z

statistics/statscache_test.go

+	c.Assert(err, IsNil)
+	statsTbl2, err := statistics.TableStatsFromStorage(se, tableInfo, 100000)
+	c.Assert(err, IsNil)
+


Check count of stats.

coocood · 2017-03-27T10:48:47Z

bootstrap.go

@@ -116,14 +116,38 @@ const (
  		UNIQUE KEY name (name)
 		) ENGINE=InnoDB DEFAULT CHARSET=utf8 STATS_PERSISTENT=0 COMMENT='help topics';`

-	// CreateStatsMetaTable store's the meta of table statistics.
+	// CreateStatsMetaTable stores the meta of table statistics.
 	CreateStatsMetaTable = `CREATE TABLE if not exists mysql.stats_meta (
 		version bigint(64) unsigned NOT NULL,
 		table_id bigint(64) NOT NULL,
 		modify_count bigint(64) NOT NULL DEFAULT 0,
 		count bigint(64) unsigned NOT NULL DEFAULT 0,
 		index idx_ver(version)


Need an index for table_id.

zimulala · 2017-03-27T12:09:17Z

statistics/statistics.go

+
+// SaveToStorage saves stats table to storage.
+func (t *Table) SaveToStorage(ctx context.Context) error {
+	_, err := ctx.(sqlexec.SQLExecutor).Execute("begin")


Check this error.

zimulala · 2017-03-27T12:33:49Z

statistics/statistics.go

+	table := &Table{
+		Info:  info,
+		Count: count,
+	}


You can use table as an argument directly.

I think this way is clearer.

zimulala · 2017-03-27T12:40:04Z

statistics/statistics.go

+			}
+		}
+	}
+	if indexCount != len(info.Indices) {


This condition can move to line366 that can make code clearer.

We can't do this. The check must be after loop.

hanfei1991 · 2017-03-27T13:03:01Z

@zimulala @coocood PTAL

zimulala · 2017-03-28T06:44:26Z

domain/domain.go

+		return nil
+	}
+	go func(do *Domain) {
+		ticker := time.NewTicker(lease)


Add the function of Stop for the ticker.

hanfei1991 · 2017-03-28T11:32:09Z

@zimulala PTAL

coocood · 2017-03-29T01:01:00Z

bootstrap.go

+	);`
+
+	// CreateStatsColsTable stores the statistics of table columns.
+	CreateStatsColsTable = `CREATE TABLE if not exists mysql.stats_columns (


How about name the table stats_histgram?
Because it contains both index and column.

coocood · 2017-03-29T01:06:42Z

statistics/statistics.go

+type Table struct {
+	Info    *model.TableInfo
+	Columns []*Column
+	Indices []*Column


How about use a different type for indices?
An index may contain multiple columns, it has a different algorithm to calculate partial column condition.

I will change it in next pr.

I have a different opinion. What you said is true, but they have more in common than difference, especially when you consider how they are used, for example, get row count in a range, once you encode the indices, the rest is the same. If we use a different type, will the same logic be written twice? I do not know if this is worth the complexity.

@lamxTyler
We can extract the same logic into a type like histogram, then wrap it with Column and Index.

coocood

LGTM

zimulala · 2017-03-29T03:17:16Z

LGTM

hanfei1991 added 3 commits March 22, 2017 19:23

statistics: refactor stats package.

6fefdd7

Merge branch 'master' into hanfei/stats

67e1a81

add some test.

4d46d80

hanfei1991 force-pushed the hanfei/stats branch from 6ac64fc to 4d46d80 Compare March 22, 2017 13:21

shenli changed the title ~~statistics: refactor package.~~ [DNM]statistics: refactor package. Mar 22, 2017

hanfei1991 added 2 commits March 22, 2017 23:06

fix golint

ee5cb61

remove useless code.

3802853

hanfei1991 force-pushed the hanfei/stats branch from 3c8e5f1 to 3802853 Compare March 24, 2017 06:10

Merge branch 'master' into hanfei/stats

6727662

alivxxx reviewed Mar 26, 2017

View reviewed changes

tiny changes.

5effd89

alivxxx reviewed Mar 27, 2017

View reviewed changes

shenli changed the title ~~[DNM]statistics: refactor package.~~ statistics: refactor package. Mar 27, 2017

alivxxx reviewed Mar 27, 2017

View reviewed changes

hanfei1991 added 2 commits March 27, 2017 16:45

address comment.

e21cb33

fix golint

49ea57a

alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 27, 2017

coocood reviewed Mar 27, 2017

View reviewed changes

zimulala reviewed Mar 27, 2017

View reviewed changes

coocood reviewed Mar 27, 2017

View reviewed changes

hanfei1991 added 2 commits March 27, 2017 19:13

add tests.

117fe39

tiny change.

30d13db

zimulala assigned zimulala and unassigned zimulala Mar 27, 2017

zimulala reviewed Mar 27, 2017

View reviewed changes

add error check.

cfde810

tiny change.

915a8b1

hanfei1991 force-pushed the hanfei/stats branch from 7d7abba to 915a8b1 Compare March 27, 2017 15:04

zimulala reviewed Mar 28, 2017

View reviewed changes

tiny change.

9ad30a3

hanfei1991 force-pushed the hanfei/stats branch from 1e973ef to 9ad30a3 Compare March 28, 2017 11:29

tiny change.

fc8c164

hanfei1991 force-pushed the hanfei/stats branch from 5b9fc71 to fc8c164 Compare March 28, 2017 12:19

coocood reviewed Mar 29, 2017

View reviewed changes

change table name.

ce6261d

coocood approved these changes Mar 29, 2017

View reviewed changes

Merge branch 'master' into hanfei/stats

effb848

hanfei1991 merged commit ee7df05 into master Mar 29, 2017

hanfei1991 deleted the hanfei/stats branch March 29, 2017 03:32

statistics: refactor package. #2913

statistics: refactor package. #2913

Conversation

hanfei1991 commented Mar 22, 2017

shenli commented Mar 22, 2017

hanfei1991 commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alivxxx Mar 26, 2017 • edited Loading

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 Mar 27, 2017

alivxxx Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alivxxx Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alivxxx commented Mar 27, 2017

alivxxx commented Mar 27, 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

hanfei1991 commented Mar 27, 2017

Choose a reason for hiding this comment

hanfei1991 commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alivxxx Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood left a comment

Choose a reason for hiding this comment

zimulala commented Mar 29, 2017

alivxxx Mar 26, 2017 •

edited

Loading

alivxxx Mar 27, 2017 •

edited

Loading

alivxxx Mar 27, 2017 •

edited

Loading

alivxxx Mar 29, 2017 •

edited

Loading