ddl: fix a bug that we miss adding last handle index in some case. #7142

winkyao · 2018-07-24T09:51:51Z

What have you changed? (mandatory)

This PR fixes a bug that, when the last remains indices count is just defaultTaskHandleCnt+1, we do not use handleRange.endIncluded outside, so we may miss adding the last index.

In this case, we break fetchRowColVals by len(w.idxRecords) >= w.batchCnt, then the handleOutOfRange is false, so the nextHandle = idxRecords[len(idxRecords)-1].handle + 1, it is exactly equal to endHandle, so in handleBackfillTask, we break the loop byfinishTask = handleRange.startHandle >= handleRange.endHandle mistakenly.

master branch will fail the newly added test case with:

db_integration_test.go:135:
    tk.MustExec("admin check index t b")
/Users/wink/go_dir/src/github.com/pingcap/tidb/util/testkit/testkit.go:160:
    tk.c.Assert(err, check.IsNil, check.Commentf("sql:%s, %v, error stack %v", sql, args, errors.ErrorStack(err)))
... value *errors.Err = &errors.Err{message:"", cause:(*errors.Err)(0xc420877130), previous:(*errors.Err)(0xc4208773b0), file:"/Users/wink/go_dir/src/github.com/pingcap/tidb/util/testkit/testkit.go", line:134} ("table count 129 != index(b) count 128")
... sql:admin check index t b, [], error stack /Users/wink/go_dir/src/github.com/pingcap/tidb/util/admin/admin.go:204: table count 129 != index(b) count 128
/Users/wink/go_dir/src/github.com/pingcap/tidb/executor/executor.go:452:
/Users/wink/go_dir/src/github.com/pingcap/tidb/executor/adapter.go:269:
/Users/wink/go_dir/src/github.com/pingcap/tidb/session/tidb.go:189:
/Users/wink/go_dir/src/github.com/pingcap/tidb/session/session.go:745:
/Users/wink/go_dir/src/github.com/pingcap/tidb/session/session.go:808:
/Users/wink/go_dir/src/github.com/pingcap/tidb/session/session.go:757:
/Users/wink/go_dir/src/github.com/pingcap/tidb/util/testkit/testkit.go:134:

What is the type of the changes? (mandatory)

Bug fix (non-breaking change which fixes an issue)

How has this PR been tested? (mandatory)

ut

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

NO

Does this PR affect tidb-ansible update? (mandatory)

NO

Does this PR need to be added to the release notes? (mandatory)

YES

fix a bug that we miss adding last handle index in some case.

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

winkyao · 2018-07-24T10:05:54Z

/run-all-tests

zimulala · 2018-07-24T10:56:49Z

ddl/index.go

@@ -678,7 +684,19 @@ func (w *addIndexWorker) handleBackfillTask(d *ddlCtx, task *reorgIndexTask) *ad
 		}

 		handleRange.startHandle = nextHandle
-		if handleRange.startHandle >= handleRange.endHandle {
+		finishTask := false
+		// if nextHandle is math.Int64, we can only use handleOutOfRange to indicates the nexHandle is handled.


s/if/If
s/to indicates/to indicate
s/nexHandle/nextHandle

alivxxx · 2018-07-24T11:00:45Z

ddl/index.go

@@ -678,7 +684,19 @@ func (w *addIndexWorker) handleBackfillTask(d *ddlCtx, task *reorgIndexTask) *ad
 		}

 		handleRange.startHandle = nextHandle
-		if handleRange.startHandle >= handleRange.endHandle {
+		finishTask := false


We can move it to line 692, where it is used.

alivxxx · 2018-07-24T11:03:37Z

ddl/index.go

-		if handleRange.startHandle >= handleRange.endHandle {
+		finishTask := false
+		// if nextHandle is math.Int64, we can only use handleOutOfRange to indicates the nexHandle is handled.
+		if handleOutOfRange && nextHandle == handleRange.endHandle {


Is handleOutOfRange implies nextHandle == handleRange.endHandle?

no. handleOutOfRange just mean current batch is handled out of range.

…add_index_bug

winkyao · 2018-07-24T11:31:23Z

@zimulala @lamxTyler PTAL

crazycs520

LGTM

tiancaiamao · 2018-07-24T13:54:12Z

ddl/index.go

@@ -637,6 +642,7 @@ func (w *addIndexWorker) backfillIndexInTxn(handleRange reorgIndexTask) (nextHan

 		if handleOutOfRange || len(idxRecords) == 0 {
 			nextHandle = handleRange.endHandle
+			handleOutOfRange = true


I'm confused why handleOutOfRange = true when len(idxRecords) == 0

And nextHandle should not be handleRange.endHandle when handleRange.exclude is false

winkyao · 2018-07-24T15:02:20Z

/run-all-tests

…add_index_bug

winkyao · 2018-07-25T00:27:58Z

/run-all-tests

winkyao · 2018-07-25T01:35:17Z

/run-integration-common-test -tidb-test=pr/592

winkyao · 2018-07-25T01:35:31Z

/run-common-test -tidb-test=pr/592

tiancaiamao · 2018-07-25T02:08:57Z

PTAL @shenli

crazycs520

LGTM

shenli · 2018-07-25T08:13:03Z

ddl/index.go

 	// TODO: use tableScan to prune columns.
 	w.idxRecords = w.idxRecords[:0]
 	startTime := time.Now()
+
+	// handleOutOfRange means that the added handle is out of taskRange.endHandle.


Can we rename the handleOutOfRange to taskDone ?

shenli · 2018-07-25T08:17:33Z

ddl/index.go

@@ -549,10 +551,26 @@ func (w *addIndexWorker) getIndexRecord(handle int64, recordKey []byte, rawRecor
 	return idxRecord, nil
 }

-func (w *addIndexWorker) fetchRowColVals(txn kv.Transaction, taskRange reorgIndexTask) ([]*indexRecord, bool, error) {
+func (w *addIndexWorker) getNextHandle(taskRange reorgIndexTask, handleOutOfRange bool) (nextHandle int64) {
+	nextHandle = taskRange.startHandle


This line is useless.

if !taskDone {
// The task is not done. So we need to pick the last processed entry's handle and plus one.
return w.idxRecords[len(w.idxRecords)-1].handle + 1
}

// The task is done. So we need to choose a handle out side this range. There are some corner cases should be considered:
// - The end of task range is MaxInt64.
// - The end of the task is excluded in the range.
if taskRange.endHandle == math.MaxInt64 || !taskRange.endIncluded {
return taskRange.endHandle
}
return taskRange.endHandle + 1

shenli · 2018-07-25T08:26:31Z

ddl/index.go

+	return nextHandle
+}
+
+func (w *addIndexWorker) fetchRowColVals(txn kv.Transaction, taskRange reorgIndexTask) ([]*indexRecord, int64, bool, error) {


Add comment for the newly added return value.

zimulala

LGTM

winkyao · 2018-07-25T08:54:32Z

@shenli PTAL

shenli · 2018-07-25T09:03:45Z

LGTM

CaitinChen · 2018-07-25T09:23:29Z

Here is a suggestion for your PR description:

This PR fixes a bug that when the last remained index count is just defaultTaskHandleCnt+1, the last index may not be added because the handleRange.endIncluded attribute is not judged in the handleBackfillTask function (the caller of fetchRowColVals).

In this case, fetchRowColVals is broken by len(w.idxRecords) >= w.batchCnt, and then handleOutOfRange will be false and nextHandle = idxRecords[len(idxRecords)-1].handle + 1. The nextHandle value will be exactly equal to the endHandle value. Thus in handleBackfillTask, the loop is mistakenly broken by finishTask = handleRange.startHandle >= handleRange.endHandle.

The newly added test case will fail in the master branch:

db_integration_test.go:135:
    tk.MustExec("admin check index t b")
/Users/wink/go_dir/src/github.com/pingcap/tidb/util/testkit/testkit.go:160:
    tk.c.Assert(err, check.IsNil, check.Commentf("sql:%s, %v, error stack %v", sql, args, errors.ErrorStack(err)))
... value *errors.Err = &errors.Err{message:"", cause:(*errors.Err)(0xc420877130), previous:(*errors.Err)(0xc4208773b0), file:"/Users/wink/go_dir/src/github.com/pingcap/tidb/util/testkit/testkit.go", line:134} ("table count 129 != index(b) count 128")
... sql:admin check index t b, [], error stack /Users/wink/go_dir/src/github.com/pingcap/tidb/util/admin/admin.go:204: table count 129 != index(b) count 128
/Users/wink/go_dir/src/github.com/pingcap/tidb/executor/executor.go:452:
/Users/wink/go_dir/src/github.com/pingcap/tidb/executor/adapter.go:269:
/Users/wink/go_dir/src/github.com/pingcap/tidb/session/tidb.go:189:
/Users/wink/go_dir/src/github.com/pingcap/tidb/session/session.go:745:
/Users/wink/go_dir/src/github.com/pingcap/tidb/session/session.go:808:
/Users/wink/go_dir/src/github.com/pingcap/tidb/session/session.go:757:
/Users/wink/go_dir/src/github.com/pingcap/tidb/util/testkit/testkit.go:134:

winkyao · 2018-07-25T09:28:55Z

/run-all-tests

winkyao · 2018-07-25T09:30:03Z

@CaitinChen Thanks for your valuable suggestions.

CaitinChen · 2018-07-25T09:26:11Z

ddl/index.go

+// getNextHandle gets next handle of entry that we are going to process.
+func (w *addIndexWorker) getNextHandle(taskRange reorgIndexTask, taskDone bool) (nextHandle int64) {
+	if !taskDone {
+		// The task is not done. So we need to pick the last processed entry's handle and plus one.


The task is not done. So we need to pick the last processed entry's handle and add one.

Note: "plus" is a prep, not a verb.

CaitinChen · 2018-07-25T09:26:32Z

ddl/index.go

+		return w.idxRecords[len(w.idxRecords)-1].handle + 1
+	}
+
+	// The task is done. So we need to choose a handle out side this range.


The task is done. So we need to choose a handle outside this range.

CaitinChen · 2018-07-25T09:27:15Z

ddl/index.go

+	}
+
+	// The task is done. So we need to choose a handle out side this range.
+	// There are some corner cases should be considered:


Some corner cases should be considered:

You can also use "There are some corner cases which should be considered:"

CaitinChen · 2018-07-25T09:33:02Z

@winkyao My pleasure. ^_^

…add_index_bug

…7142)

…7142) (#7156)

ddl: fix a bug that we miss adding last handle index in some case.

700c5df

Merge branch 'master' into fix_add_index_bug

e2c40c8

winkyao added the status/all tests passed label Jul 24, 2018

zimulala reviewed Jul 24, 2018

View reviewed changes

alivxxx reviewed Jul 24, 2018

View reviewed changes

winkyao added 2 commits July 24, 2018 19:26

addresss comments

e7623bf

Merge branch 'fix_add_index_bug' of github.com:winkyao/tidb into fix_…

cc6ee90

…add_index_bug

crazycs520 reviewed Jul 24, 2018

View reviewed changes

tiancaiamao reviewed Jul 24, 2018

View reviewed changes

winkyao added 2 commits July 24, 2018 22:53

refactor to make code clear

ccc7008

Merge branch 'master' into fix_add_index_bug

55347cd

winkyao removed the status/all tests passed label Jul 24, 2018

winkyao added 2 commits July 24, 2018 23:29

make code cleaner

55b62a3

Merge branch 'fix_add_index_bug' of github.com:winkyao/tidb into fix_…

cb66601

…add_index_bug

winkyao added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 25, 2018

winkyao added the status/all tests passed label Jul 25, 2018

refactor

4db7a68

crazycs520 reviewed Jul 25, 2018

View reviewed changes

shenli reviewed Jul 25, 2018

View reviewed changes

zimulala previously approved these changes Jul 25, 2018

View reviewed changes

address comment

268c3f5

winkyao dismissed zimulala’s stale review via 268c3f5 July 25, 2018 08:53

winkyao added 2 commits July 25, 2018 16:56

add comment

57e4a83

Merge branch 'master' into fix_add_index_bug

c8a9410

winkyao added status/LGT2 Indicates that a PR has LGTM 2. status/LGT3 The PR has already had 3 LGTM. and removed status/LGT1 Indicates that a PR has LGTM 1. status/LGT2 Indicates that a PR has LGTM 2. labels Jul 25, 2018

shenli previously approved these changes Jul 25, 2018

View reviewed changes

CaitinChen reviewed Jul 25, 2018

View reviewed changes

winkyao added 2 commits July 25, 2018 17:46

address comment

192aad7

Merge branch 'fix_add_index_bug' of github.com:winkyao/tidb into fix_…

314bf93

…add_index_bug

winkyao dismissed shenli’s stale review via 314bf93 July 25, 2018 09:48

zimulala approved these changes Jul 25, 2018

View reviewed changes

Merge branch 'master' into fix_add_index_bug

d2f17d5

ciscoxll approved these changes Jul 25, 2018

View reviewed changes

winkyao merged commit 85cd246 into pingcap:master Jul 25, 2018

winkyao deleted the fix_add_index_bug branch July 25, 2018 10:52

winkyao added a commit that referenced this pull request Jul 25, 2018

ddl: fix a bug that we miss adding last handle index in some case. (#…

d0125d3

…7142)

winkyao mentioned this pull request Jul 25, 2018

ddl: fix a bug that we miss adding last handle index in some case. (#7142) #7156

Merged

winkyao added a commit that referenced this pull request Jul 25, 2018

ddl: fix a bug that we miss adding last handle index in some case. (#…

183671b

…7142) (#7156)

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: fix a bug that we miss adding last handle index in some case. #7142

ddl: fix a bug that we miss adding last handle index in some case. #7142

winkyao commented Jul 24, 2018 •

edited

Loading

winkyao commented Jul 24, 2018

zimulala Jul 24, 2018

alivxxx Jul 24, 2018

alivxxx Jul 24, 2018

winkyao Jul 24, 2018

winkyao commented Jul 24, 2018

crazycs520 left a comment

tiancaiamao Jul 24, 2018

tiancaiamao Jul 24, 2018

winkyao commented Jul 24, 2018

winkyao commented Jul 25, 2018

winkyao commented Jul 25, 2018

winkyao commented Jul 25, 2018

tiancaiamao commented Jul 25, 2018

crazycs520 left a comment

shenli Jul 25, 2018

shenli Jul 25, 2018

shenli Jul 25, 2018

shenli Jul 25, 2018

zimulala left a comment

winkyao commented Jul 25, 2018

shenli commented Jul 25, 2018

CaitinChen commented Jul 25, 2018

winkyao commented Jul 25, 2018

winkyao commented Jul 25, 2018

CaitinChen Jul 25, 2018

CaitinChen Jul 25, 2018

CaitinChen Jul 25, 2018

CaitinChen Jul 25, 2018

CaitinChen commented Jul 25, 2018

ddl: fix a bug that we miss adding last handle index in some case. #7142

ddl: fix a bug that we miss adding last handle index in some case. #7142

Conversation

winkyao commented Jul 24, 2018 • edited Loading

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

winkyao commented Jul 24, 2018

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

winkyao commented Jul 24, 2018

crazycs520 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao commented Jul 24, 2018

winkyao commented Jul 25, 2018

winkyao commented Jul 25, 2018

winkyao commented Jul 25, 2018

tiancaiamao commented Jul 25, 2018

crazycs520 left a 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 left a comment

Choose a reason for hiding this comment

winkyao commented Jul 25, 2018

shenli commented Jul 25, 2018

CaitinChen commented Jul 25, 2018

winkyao commented Jul 25, 2018

winkyao commented Jul 25, 2018

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

CaitinChen commented Jul 25, 2018

winkyao commented Jul 24, 2018 •

edited

Loading