executor: support cleanup dangling index data command #6102

jackysp · 2018-03-20T11:43:41Z

Support admin cleanup index table_name index_name to clean up the dangling index.
PTAL @zimulala @coocood

zimulala · 2018-03-21T06:54:23Z

executor/admin.go

+	idxResult        distsql.SelectResult
+
+	//tblChunk         *chunk.Chunk
+	//tblResult        distsql.SelectResult


Could we remove these comments？

zimulala · 2018-03-21T08:22:39Z

parser/parser.y

@@ -260,6 +260,7 @@ import (
 	cascaded	"CASCADED"
 	charsetKwd	"CHARSET"
 	checksum	"CHECKSUM"
+	cleanup     "CLEANUP"


Keep alignment.

zimulala · 2018-03-21T08:23:37Z

parser/parser.y

@@ -4782,6 +4783,14 @@ AdminStmt:
 			Index: string($5),
 		}
 	}
+|	"ADMIN" "CLEANUP" "INDEX" TableName Identifier


Do we need to add tests in parser_test.go ?

zimulala · 2018-03-21T08:31:13Z

executor/admin.go

+}
+
+type idxData struct {
+	matched bool


It seems we don't use this field.

zimulala · 2018-03-21T08:35:34Z

executor/admin.go

+		return nil
+	}
+	handleIdx := len(e.idxCols) - 1
+	errInTxn := kv.RunInNewTxn(e.ctx.GetStore(), true, func(txn kv.Transaction) error {


Do we need to handle this code in batch? I'm afraid that we need to clean up a lot of data. At the same time, some people also modified the data. Then we need to redo it.

jackysp · 2018-03-23T11:51:04Z

/run-all-tests

coocood · 2018-03-23T12:23:23Z

executor/admin.go

+		if err != nil {
+			return errors.Trace(err)
+		}
+		if e.idxChunk.NumRows() == 0 {


We should check the e.scanRowCnt to return.

friendly ping @jackysp Please resolve reviewer's comment

jackysp · 2018-03-25T14:09:29Z

PTAL @coocood @zimulala

coocood · 2018-03-26T05:42:24Z

LGTM

zimulala · 2018-03-26T11:54:06Z

/run-all-tests

jackysp · 2018-03-26T12:30:26Z

/run-integration-common-test

zimulala · 2018-03-27T12:57:15Z

executor/admin.go

+	}
+	count := 0
+	for {
+		count++


count doesn't be used.

jackysp · 2018-03-28T05:40:19Z

PTAL @zimulala

zimulala · 2018-03-28T06:00:17Z

executor/admin.go

+	return nil
+}
+
+func (e *CleanupIndexExec) extractIdxVals(row chunk.Row, idxVals []types.Datum) []types.Datum {


This function logic is the same as RecoverIndexExec's extractIdxVals. Could we use the same one?

zimulala · 2018-03-28T06:14:32Z

executor/admin.go

+			handle := row.GetInt64(len(e.idxCols) - 1)
+			idxVals := e.extractIdxVals(row, e.idxValsBufs[e.scanRowCnt])
+			e.idxValsBufs[e.scanRowCnt] = idxVals
+			e.matchedIndex[handle] = idxData{false, idxVals}


Could we remove the field ofmatched ?

zimulala · 2018-03-28T06:20:20Z

executor/admin.go

+	err := plan.SetPBColumnsDefaultValue(e.ctx, dagReq.Executors[0].IdxScan.Columns, e.idxCols)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}


Can these codes be extracted into a function？
The logic is used in many places.

They are looked similar, but not exactly the same. It needs a refactor. Maybe in the next pr.

It's the same as CheckIndexRangeExec's buildDAGPB.
But if you want to refactor some of the exec in admin, you can handle it in the next PR.

jackysp · 2018-03-28T07:00:52Z

PTAL @zimulala

zimulala · 2018-03-29T03:14:07Z

executor/builder.go

+		return nil
+	}
+	idxName := strings.ToLower(v.IndexName)
+	indices := t.WritableIndices()


Is it enough if we only handle the public index?

zimulala · 2018-03-29T03:19:23Z

executor/admin.go

+	err := plan.SetPBColumnsDefaultValue(e.ctx, dagReq.Executors[0].IdxScan.Columns, e.idxCols)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}


It's the same as CheckIndexRangeExec's buildDAGPB.
But if you want to refactor some of the exec in admin, you can handle it in the next PR.

zimulala · 2018-03-29T03:22:11Z

Rest LGTM

zimulala · 2018-03-29T06:07:23Z

executor/builder.go

 	var index table.Index
-	for _, idx := range indices {


Indices() isn't equal to public indices.

jackysp · 2018-03-31T02:05:59Z

PTAL @zimulala , sorry for the late update.

zimulala · 2018-04-02T07:11:16Z

executor/builder.go

@@ -321,7 +321,10 @@ func (b *executorBuilder) buildCleanupIndex(v *plan.CleanupIndex) Executor {
 	}
 	idxName := strings.ToLower(v.IndexName)
 	var index table.Index
-	for _, idx := range t.WritableIndices() {
+	for _, idx := range t.Indices() {
+		if idx.Meta().State != model.StatePublic {


I think to do this check as line 334 is better.

zimulala

LGTM

admin cleanup index

e1da0cc

jackysp added the WIP label Mar 20, 2018

zimulala reviewed Mar 21, 2018

View reviewed changes

jackysp added 3 commits March 23, 2018 11:27

use batch

f42bcc9

add test

25f0efc

address comments

54e5a9f

jackysp removed the WIP label Mar 23, 2018

resolve conflicts

2919e28

coocood reviewed Mar 23, 2018

View reviewed changes

jackysp and others added 3 commits March 23, 2018 20:23

limit the batch

8e4925e

address comment

1f78194

Merge branch 'master' into recover_table

ee99255

zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 26, 2018

zimulala added the all-tests-passed label Mar 26, 2018

Merge branch 'master' into recover_table

4b34f6f

jackysp removed the all-tests-passed label Mar 26, 2018

Merge branch 'master' into recover_table

651b8c8

jackysp added the all-tests-passed label Mar 27, 2018

zimulala reviewed Mar 27, 2018

View reviewed changes

executor/admin.go Outdated

}

count := 0

for {

count++

Copy link

Contributor

zimulala Mar 27, 2018

Choose a reason for hiding this comment

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

count doesn't be used.

jackysp added 2 commits March 28, 2018 13:37

address comments

4a0795c

resolve conflicts

2dc762a

zimulala reviewed Mar 28, 2018

View reviewed changes

address comments

e70601a

zimulala reviewed Mar 29, 2018

View reviewed changes

jackysp added 2 commits March 29, 2018 13:50

address comments

e5d491d

address comments

5d426cd

zimulala reviewed Mar 29, 2018

View reviewed changes

executor/builder.go Outdated

var index table.Index

for _, idx := range indices {

Copy link

Contributor

zimulala Mar 29, 2018

Choose a reason for hiding this comment

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

Indices() isn't equal to public indices.

jackysp and others added 2 commits March 31, 2018 09:55

Merge branch 'master' into recover_table

5989bf6

check index by public only indices

9617686

zimulala reviewed Apr 2, 2018

View reviewed changes

zimulala approved these changes Apr 2, 2018

View reviewed changes

jackysp added 2 commits April 2, 2018 16:53

Merge branch 'master' into recover_table

303bad4

Merge branch 'master' into recover_table

04224eb

jackysp merged commit 39a2f84 into pingcap:master Apr 2, 2018

jackysp deleted the recover_table branch May 22, 2018 06:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: support cleanup dangling index data command #6102

executor: support cleanup dangling index data command #6102

jackysp commented Mar 20, 2018 •

edited

Loading

zimulala Mar 21, 2018

zimulala Mar 21, 2018

zimulala Mar 21, 2018

zimulala Mar 21, 2018

zimulala Mar 21, 2018

jackysp commented Mar 23, 2018

coocood Mar 23, 2018

zhexuany Mar 27, 2018

jackysp commented Mar 25, 2018

coocood commented Mar 26, 2018

zimulala commented Mar 26, 2018

jackysp commented Mar 26, 2018

zimulala Mar 27, 2018

jackysp commented Mar 28, 2018

zimulala Mar 28, 2018

zimulala Mar 28, 2018

zimulala Mar 28, 2018

jackysp Mar 28, 2018 •

edited

Loading

zimulala Mar 29, 2018

jackysp commented Mar 28, 2018

zimulala Mar 29, 2018

zimulala Mar 29, 2018

zimulala commented Mar 29, 2018

zimulala Mar 29, 2018

jackysp commented Mar 31, 2018

zimulala Apr 2, 2018

zimulala left a comment

executor: support cleanup dangling index data command #6102

executor: support cleanup dangling index data command #6102

Conversation

jackysp commented Mar 20, 2018 • 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

jackysp commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackysp commented Mar 25, 2018

coocood commented Mar 26, 2018

zimulala commented Mar 26, 2018

jackysp commented Mar 26, 2018

Choose a reason for hiding this comment

jackysp commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackysp Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackysp commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala commented Mar 29, 2018

Choose a reason for hiding this comment

jackysp commented Mar 31, 2018

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

jackysp commented Mar 20, 2018 •

edited

Loading

jackysp Mar 28, 2018 •

edited

Loading