ddl: Speed up adding index phase #2341

zimulala · 2016-12-28T10:04:08Z

Handle issue #2257
Remove the batch get operation when adding index.

Using 3 TiKV, 1 PD, 1 TiDB , the servers of TiKV and TiDB are not on a computer, two TiKV on a computer.
The lease of TiDB is 1s.
The number of records in this table is 3531200(3.4 M), and the table's structure is as follows:
+------------------+------------------+------+------+---------+-------+
| Field | Type | Null | Key | Default | Extra |
+------------------+------------------+------+------+---------+-------+
| a | varchar(64) | YES | | | |
| b | bigint(20) | NO | | 0 | |
| c | datetime | YES | | NULL | |
| d | int(11) UNSIGNED | NO | | 0 | |
| e | int(11) UNSIGNED | NO | | 0 | |
| f | int(11) UNSIGNED | NO | | 0 | |
| g | varchar(28) | NO | | | |
| h | varchar(28) | NO | | | |
+------------------+------------------+------+------+---------+-------+

tidb-before> alter table battle_begin add index b (event_id);
Query OK, 0 rows affected (8 min 36.40 sec)
tidb-before> alter table battle_begin add index g (sum_id);
Query OK, 0 rows affected (7 min 10.01 sec)

tidb-current> alter table battle_begin add index b (event_id);
Query OK, 0 rows affected (3 min 35.75 sec)
tidb-current> alter table battle_begin add index g (sum_id);
Query OK, 0 rows affected (3 min 46.25 sec)

coocood · 2016-12-29T02:23:20Z

ddl/index.go

-func (d *ddl) fetchRowColVals(txn kv.Transaction, t table.Table, batchOpInfo *indexBatchOpInfo, seekHandle int64) error {
-	cols := t.Cols()
-	idxInfo := batchOpInfo.tblIndex.Meta()
+func isFinish(limit, input int64) bool {


coocood · 2016-12-29T02:52:09Z

ddl/index.go

-			if err1 != nil {
-				return errors.Trace(err1)
+		for i := 0; i < batches; i++ {
+			go d.backfillIndex(t, batchOpInfo, seekHandle, &wg)


We should Add waitgroup outside the goroutine

shenli · 2016-12-29T03:03:06Z

ddl/index.go

+func (b batchRetSlice) Less(i, j int) bool { return b[i].doneHandle < b[j].doneHandle }
+func (b batchRetSlice) Swap(i, j int)      { b[i], b[j] = b[j], b[i] }
+
+type indexRecord struct {


Please add comments for the following structs.

coocood · 2016-12-29T02:53:30Z

ddl/index.go

 }

-const defaultBatchCnt = 1024
-const defaultSmallBatchCnt = 128
+type indexBatchOpInfo struct {


Add comment on this type and its members.

coocood · 2016-12-29T02:53:48Z

ddl/index.go

+	defaultSmallBatches  = 16
+)
+
+type batchRet struct {


Add more comment on this type.

coocood · 2016-12-29T02:55:05Z

ddl/index.go

 	}
 }

-// recordIterFunc is used for low-level record iteration.
-type recordIterFunc func(h int64, rowKey kv.Key, rawRecord []byte) (more bool, err error)
+type handleInfo struct {


Add comments on the type and its members.

coocood · 2016-12-29T03:00:48Z

ddl/index.go

 	seekHandle := reorgInfo.Handle
+	wg := sync.WaitGroup{}


Put this in for loop is better.

coocood · 2016-12-29T05:22:40Z

ddl/index.go

+)
+
+// batchRet is the result of the batch.
+type batchRet struct {


rename to batchResult is better.

coocood · 2016-12-29T05:32:08Z

ddl/index.go

-func (d *ddl) fetchRowColVals(txn kv.Transaction, t table.Table, batchOpInfo *indexBatchOpInfo, seekHandle int64) error {
-	cols := t.Cols()
-	idxInfo := batchOpInfo.tblIndex.Meta()
+func isFinished(limit, input int64) bool {


The function name is too generic.
make it as a method of handleInfo is better.

coocood · 2016-12-29T05:33:27Z

ddl/index.go

-	cols := t.Cols()
-	idxInfo := batchOpInfo.tblIndex.Meta()
+func isFinished(limit, input int64) bool {
+	if limit == 0 || input < limit {


A 0 handle can be valid.

coocood · 2016-12-29T05:35:17Z

ddl/index.go

+		ret.doneHandle = idxRecords[ret.count-1].handle
+	}
+	// Be sure to do this operation only once.
+	handleInfo.once.Do(func() {


We don't have to use once, add a bool member in handleInfo is clearer.

coocood · 2016-12-29T05:38:23Z

ddl/index.go

 			}
-			batchOpInfo.idxRecords = batchOpInfo.idxRecords[:0]
-			err1 = d.backfillIndexInTxn(t, txn, batchOpInfo, seekHandle)
+			seekHandle = handle + 1


rename seekHandle to batchStartHandle is better.

coocood · 2016-12-29T05:41:05Z

ddl/index.go

+		for i := 0; i < batches; i++ {
+			wg.Add(1)
+			go d.backfillIndex(t, batchOpInfo, seekHandle, &wg)
+			handle := <-batchOpInfo.nextCh


name it doneHandle, keep it consistent with the sender's

coocood · 2016-12-29T05:44:05Z

ddl/index.go

+	colMap     map[int64]*types.FieldType
+	batchRetCh chan *batchRet
+	nextCh     chan int64 // It notifies to start the next batch.
+}

 // How to add index in reorganization state?
 //  1. Generate a snapshot with special version.


Update the comment.

ngaut · 2016-12-30T08:34:21Z

PTAL

zimulala · 2017-01-03T08:02:23Z

PTAL @coocood

coocood · 2017-01-03T08:33:06Z

ddl/index.go

+// The above operations are completed in a transaction.
+// When concurrent tasks are processed, the batch result returned by each batch is sorted by handle. Then traverse the
+// batch results, gets the total number of row in the concurrent task and update the processed handle value. If
+// you encounter an error message, exit traversal.


coocood · 2017-01-03T08:38:04Z

ddl/index.go

-//  3. For one row, if the row has been already deleted, skip to next row.
-//  4. If not deleted, check whether index has existed, if existed, skip to next row.
-//  5. If index doesn't exist, create the index and then continue to handle next row.
+// Concurrently process defaultSmallBatches tasks. Each task deals with a handle interval of the index record.


we can use handle range uniformly.

coocood · 2017-01-03T08:38:24Z

ddl/index.go

-//  4. If not deleted, check whether index has existed, if existed, skip to next row.
-//  5. If index doesn't exist, create the index and then continue to handle next row.
+// Concurrently process defaultSmallBatches tasks. Each task deals with a handle interval of the index record.
+// The handle interval is defaultSmallBatchCnt.


handle range size

coocood · 2017-01-03T08:52:07Z

ddl/index.go

-//  5. If index doesn't exist, create the index and then continue to handle next row.
+// Concurrently process defaultSmallBatches tasks. Each task deals with a handle interval of the index record.
+// The handle interval is defaultSmallBatchCnt.
+// Although the length of each handle interval is controllable, but the range of the handle value can't be expected,


Because each handle range depends on the previous one, it's necessary to obtain the handle range sequentially.

coocood · 2017-01-03T09:30:10Z

ddl/index.go

-			return errors.Trace(err)
-		}
-		rk := t.RecordKey(handle)
+func (d *ddl) backfillIndex(t table.Table, batchOpInfo *indexBatchOpInfo, seekHandle int64, wg *sync.WaitGroup) {


s/backfillIndex/doBackfillIndxTask
s/seekHandle/startHandle

coocood · 2017-01-03T09:32:12Z

ddl/index.go

-			break
-		} else if err != nil {
-			return errors.Trace(err)
+		ret = d.backfillIndexInTxn(t, txn, batchOpInfo, handleInfo)


s/backfillIndexInTxn/doBackfillIndexTaskInTxn

coocood · 2017-01-03T09:34:58Z

ddl/index.go

+			go d.backfillIndex(t, batchOpInfo, batchStartHandle, &wg)
+			doneHandle := <-batchOpInfo.nextCh
+			// There is no data to seek.
+			if doneHandle == 0 {


zero handle may be valid.

How about check doneHandle == batchStartHandle

zimulala · 2017-01-03T11:03:21Z

PTAL @ngaut @coocood

coocood · 2017-01-03T10:55:20Z

ddl/index.go

-	seekHandle := reorgInfo.Handle
+	addedCount := job.GetRowCount()
+	batchStartHandle := reorgInfo.Handle
+	wg := sync.WaitGroup{}


Move wg in for loop

coocood · 2017-01-03T11:05:05Z

ddl/index.go

+const (
+	defaultBatchCnt      = 1024
+	defaultSmallBatchCnt = 128
+	defaultSmallBatches  = 16


defaultTasks

coocood · 2017-01-03T11:05:38Z

ddl/index.go

+)
+
+// batchResult is the result of the batch.
+type batchResult struct {


QueenyJin · 2017-01-03T11:29:27Z

ddl/index.go

-//  3. For one row, if the row has been already deleted, skip to next row.
-//  4. If not deleted, check whether index has existed, if existed, skip to next row.
-//  5. If index doesn't exist, create the index and then continue to handle next row.
+// Concurrently process defaultSmallBatches tasks. Each task deals with a handle range of the index record.


defaultSmallBatches tasks -> the defaultSmallBatches tasks.

QueenyJin · 2017-01-03T11:47:29Z

ddl/index.go

+// Because each handle range depends on the previous one, it's necessary to obtain the handle range serially.
+// Real concurrent processing needs to perform after the handle range has been acquired.
+// The operation flow of the each batch of data is as follows:
+//  1. Open a goroutine. Traverse the snapshot to obtain the handle range, while access to the corresponding row key and


access to ->accessing

QueenyJin · 2017-01-03T11:52:35Z

ddl/index.go

+// The operation flow of the each batch of data is as follows:
+//  1. Open a goroutine. Traverse the snapshot to obtain the handle range, while access to the corresponding row key and
+// raw index value. Then notify to start the next batch.
+//  2. Decoding this batch of raw index value gets the corresponding index value.


Decoding this batch of raw index value gets the corresponding index value. ->Decode this batch of raw index value to get the corresponding index value.

QueenyJin · 2017-01-03T11:53:56Z

ddl/index.go

+//  1. Open a goroutine. Traverse the snapshot to obtain the handle range, while access to the corresponding row key and
+// raw index value. Then notify to start the next batch.
+//  2. Decoding this batch of raw index value gets the corresponding index value.
+//  3. Deal with this index records one by one. If the index record exists, skip to the next row.


this index records -> these index records

QueenyJin · 2017-01-03T11:54:17Z

ddl/index.go

+//  2. Decoding this batch of raw index value gets the corresponding index value.
+//  3. Deal with this index records one by one. If the index record exists, skip to the next row.
+// If the index doesn't exist, create the index ande then continue to handle the next row.
+//  4. When the handle of a range is completed, returns the corresponding batch result.


returns ->return

QueenyJin · 2017-01-03T11:54:54Z

ddl/index.go

+//  4. When the handle of a range is completed, returns the corresponding batch result.
+// The above operations are completed in a transaction.
+// When concurrent tasks are processed, the batch result returned by each batch is sorted by the handle. Then traverse the
+// batch results, gets the total number of row in the concurrent task and update the processed handle value. If


gets -> get
row -> rows

QueenyJin · 2017-01-03T11:56:23Z

ddl/index.go

+// The above operations are completed in a transaction.
+// When concurrent tasks are processed, the batch result returned by each batch is sorted by the handle. Then traverse the
+// batch results, gets the total number of row in the concurrent task and update the processed handle value. If
+// we encounter an error message, exit traversal.


-> an error message is displayed, exit the traversal.

coocood · 2017-01-03T12:43:41Z

ddl/index.go

-		colMap:     colMap,
-		handle:     reorgInfo.Handle,
-		idxRecords: make([]*indexRecord, 0, batchCnt),
+	tasks := defaultTaskCnt


coocood · 2017-01-03T12:44:02Z

ddl/index.go

 	}
 }

-// recordIterFunc is used for low-level record iteration.
-type recordIterFunc func(h int64, rowKey kv.Key, rawRecord []byte) (more bool, err error)
+// handleInfo records start ande end handle that is used in a task.


coocood · 2017-01-03T12:50:23Z

LGTM

hanfei1991 · 2017-01-03T12:31:43Z

ddl/index.go

+
+// taskResult is the result of the task.
+type taskResult struct {
+	count      int   // The number of records that has been proceed in the task.


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

ddl/index.go

+	var err error
+	for _, ret := range taskRets {
+		if ret.err != nil {
+			err = ret.err


return directly here? then u don't need to use an err var.

I think that's OK.

hanfei1991 · 2017-01-03T13:10:29Z

ddl/index.go

+				if err == nil {
+					err = err1
+				} else {
+					log.Warnf("[ddl] add index failed when update handle %d, err %v", doneHandle, err)


use err %s and err.Error() ?

This isn't necessary. Using err is clearer.

Then should you log err1 in this place ?

hanfei1991 · 2017-01-04T03:53:58Z

ddl/index.go

 		}

 		// Create the index.
-		handle, err := batchOpInfo.tblIndex.Create(txn, idxRecord.vals, idxRecord.handle)
+		handle, err := taskOpInfo.tblIndex.Create(txn, idxRecord.vals, idxRecord.handle)
 		if err != nil {
 			if terror.ErrorEqual(err, kv.ErrKeyExists) && idxRecord.handle == handle {


When it returns err key exists but the handle is not the same?

It's a unique key, the key value is updated when we create the index.

hanfei1991 · 2017-01-04T03:57:10Z

ddl/index.go

+		ret.doneHandle = idxRecords[ret.count-1].handle
+	}
+	// Be sure to do this operation only once.
+	if !handleInfo.isSent {


When the handleInfo's isSent is true at this time?

This transaction retries.

hanfei1991 · 2017-01-04T04:00:06Z

ddl/index.go

+		handleInfo.endHandle = ret.doneHandle
+		handleInfo.isSent = true
+	}
+	if ret.count == 0 {


Does ret.count always equal to len(idxRecords)? If so, you needn't record it.

Yes. But we need it to update statistics.

hanfei1991

LGTM

coocood reviewed Dec 29, 2016

View reviewed changes

shenli reviewed Dec 29, 2016

View reviewed changes

coocood reviewed Dec 29, 2016

View reviewed changes

coocood reviewed Jan 3, 2017

View reviewed changes

zimulala added 6 commits January 3, 2017 17:08

ddl: speed up add index phase

039a67d

ddl: address comments

c60c131

ddl: add comments

0f433fb

ddl: update the variable name

bda70ca

ddl: update comments

a218cfb

ddl: update comments

1b7e6e7

zimulala force-pushed the zimuxia/add-index-3 branch from bc6b660 to 1b7e6e7 Compare January 3, 2017 09:12

coocood reviewed Jan 3, 2017

View reviewed changes

zimulala added 2 commits January 3, 2017 18:44

ddl: address comments

46f31e8

ddl: update comments

4105be3

coocood reviewed Jan 3, 2017

View reviewed changes

QueenyJin reviewed Jan 3, 2017

View reviewed changes

ddl: rename batch to task

de47459

ddl: update comments

0a29199

coocood reviewed Jan 3, 2017

View reviewed changes

ddl: update comments

ece541c

hanfei1991 reviewed Jan 3, 2017

View reviewed changes

ddl: update comment

1841197

hanfei1991 reviewed Jan 4, 2017

View reviewed changes

hanfei1991 approved these changes Jan 4, 2017

View reviewed changes

Merge branch 'master' into zimuxia/add-index-3

3590102

hanfei1991 merged commit cc5fcae into master Jan 4, 2017

hanfei1991 deleted the zimuxia/add-index-3 branch January 4, 2017 04:33

zimulala mentioned this pull request Jan 6, 2017

Support backfill index concurrently in DDL #1716

Closed

ddl: Speed up adding index phase #2341

ddl: Speed up adding index phase #2341

Conversation

zimulala commented Dec 28, 2016 • 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

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

ngaut commented Dec 30, 2016

zimulala commented Jan 3, 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

coocood Jan 3, 2017 • edited by zimulala 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

zimulala commented Jan 3, 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

QueenyJin Jan 3, 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

coocood commented Jan 3, 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

hanfei1991 left a comment

Choose a reason for hiding this comment

zimulala commented Dec 28, 2016 •

edited

Loading

coocood Jan 3, 2017 •

edited by zimulala

Loading

QueenyJin Jan 3, 2017 •

edited

Loading