exeuctor: fix coprocessor goroutine leak for IndexMerge #41610
Conversation
Signed-off-by: guo-shaoge <shaoge1994@163.com>
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Looks like no need to check goroutines explicitly, because ci will fail if found leaking. Check #41631 |
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
executor/index_merge_reader.go
Outdated
| @@ -361,6 +361,13 @@ func (e *IndexMergeReaderExecutor) startPartialIndexWorker(ctx context.Context, | |||
| SetFromInfoSchema(e.ctx.GetInfoSchema()). | |||
| SetClosestReplicaReadAdjuster(newClosestReadAdjuster(e.ctx, &builder.Request, e.partialNetDataSizes[workID])) | |||
|
|
|||
| selectResults := make([]distsql.SelectResult, 0, len(keyRanges)) | |||
There was a problem hiding this comment.
Why use a slice? only one element?
executor/index_merge_reader.go
Outdated
| @@ -510,6 +535,7 @@ func (e *IndexMergeReaderExecutor) startPartialTableWorker(ctx context.Context, | |||
| if err = worker.tableReader.Close(); err != nil { | |||
| logutil.Logger(ctx).Error("close Select result failed:", zap.Error(err)) | |||
| } | |||
| tableReaderClosed = true | |||
There was a problem hiding this comment.
If worker.tableReader.Close() return a error, it will be called again in defer()?
There was a problem hiding this comment.
Will fix in #41647
Signed-off-by: guo-shaoge <shaoge1994@163.com>
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: a8445b7
|
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 94066e4
|
| @@ -903,12 +903,12 @@ func TestIndexMergeCoprGoroutinesLeak(t *testing.T) { | |||
| require.Contains(t, res[1][0], "IndexMerge") | |||
|
|
|||
| // If got goroutines leak in coprocessor, ci will fail. | |||
| require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/executor/testIndexMergePartialTableWorkerCoprLeak", "return(3)")) | |||
| require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/executor/testIndexMergePartialTableWorkerCoprLeak", `panic("testIndexMergePartialTableWorkerCoprLeak")`)) | |||
|
In response to a cherrypick label: new pull request created to branch |
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
What problem does this PR solve?
Issue Number: close #41605
Problem Summary: As issue said, for partialIndexWorker and partialTableWorker of IndexMerge, we may failed to call
SelectResult.Closewhen got panic(oom) inIndexMerge.fetchHandles(). Therefore goroutines in coprocessor that tries to send copResp will stuck, because receiver already panic andcopIterator.finishChis never closed.What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.