executor: fix IndexMerge worker panic because of closed channel #29803

guo-shaoge · 2021-11-15T10:20:53Z

Signed-off-by: guo-shaoge shaoge1994@163.com

What problem does this PR solve?

Issue Number: close #29610

Problem Summary:
When partial index worker got error, it will write give err to main worker using resultCh. But resultCh may already be closed(Maybe because process worker finished and close it).

What is changed and how it works?

Add a log to indicate why we got error when fetching handles.
partial index worker wirte its err msg using fetchCh instead of using resultCh directly.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below): this is covered by QA team.
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

executor: fix IndexMerge worker panic because of closed channel

Signed-off-by: guo-shaoge <shaoge1994@163.com>

ti-chi-bot · 2021-11-15T10:20:55Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: guo-shaoge <shaoge1994@163.com>

guo-shaoge · 2021-11-16T06:47:20Z

/run-common-test
/run-integration-copr-test
/run-integration-common-test

Signed-off-by: guo-shaoge <shaoge1994@163.com>

…to fix_index_merge_panic

guo-shaoge · 2021-11-24T02:02:53Z

/run-check_dev_2

guo-shaoge · 2022-01-06T09:08:12Z

/run-mysql-test

Signed-off-by: guo-shaoge <shaoge1994@163.com>

guo-shaoge · 2022-01-06T14:06:38Z

/cc @XuHuaiyu @wshwsh12

Signed-off-by: guo-shaoge <shaoge1994@163.com>

…ex_merge_panic Signed-off-by: guo-shaoge <shaoge1994@163.com>

Signed-off-by: guo-shaoge <shaoge1994@163.com>

sre-bot · 2022-01-08T08:39:38Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/1431aeca8ad90d773658ee99ecc2c07eaacdf47c

executor/distsql.go

executor/index_merge_reader.go

guo-shaoge · 2022-01-17T03:45:11Z

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

guo-shaoge · 2022-01-17T05:03:34Z

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

Signed-off-by: guo-shaoge <shaoge1994@163.com>

…to fix_index_merge_panic

XuHuaiyu

We'd better to add more failpoint test cases

XuHuaiyu · 2022-01-17T07:07:38Z

executor/index_merge_reader.go

-	resultCh <- &lookupTableTask{
-		doneCh: doneCh,
+func (w *partialIndexWorker) syncErr(ctx context.Context, fetchCh chan<- *lookupTableTask, err error) {
+	logutil.Logger(ctx).Error("got error when read index handles", zap.Error(err))


what's index handles?

XuHuaiyu · 2022-01-17T07:08:09Z

executor/index_merge_reader_test.go

+	sql := "select /*+ use_index_merge(t1) */ * from t1 where c1 < 10 or c2 < 10;"
+
+	c.Assert(failpoint.Enable("github.com/pingcap/tidb/executor/partialIndexWorkerError", "return"), IsNil)
+	err := tk.QueryToErr(sql)


check the error message here

XuHuaiyu · 2022-01-17T07:09:35Z

executor/index_merge_reader.go

@@ -684,7 +688,7 @@ func (e *IndexMergeReaderExecutor) handleHandlesFetcherPanic(ctx context.Context
 		logutil.Logger(ctx).Error(err4Panic.Error())
 		doneCh := make(chan error, 1)
 		doneCh <- err4Panic
-		resultCh <- &lookupTableTask{
+		ch <- &lookupTableTask{


Add a failpoint test for this panic

ti-chi-bot · 2022-02-07T21:55:14Z

@guo-shaoge: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

guo-shaoge · 2023-03-10T07:54:11Z

#41633
#41610
#41036

executor: add log when index/table worker panic for IndexMergeReader

62d6d52

Signed-off-by: guo-shaoge <shaoge1994@163.com>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 15, 2021

trivial change

7e7ce63

Signed-off-by: guo-shaoge <shaoge1994@163.com>

guo-shaoge added 3 commits November 22, 2021 02:32

try to fix this better

43cc4fe

Signed-off-by: guo-shaoge <shaoge1994@163.com>

fix fmt

aaff798

Signed-off-by: guo-shaoge <shaoge1994@163.com>

fix bug

8f269e6

Signed-off-by: guo-shaoge <shaoge1994@163.com>

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 23, 2021

guo-shaoge added 2 commits November 23, 2021 22:24

Merge branch 'master' into fix_index_merge_panic

be7abf2

Merge branch 'fix_index_merge_panic' of github.com:guo-shaoge/tidb in…

1d43b7a

…to fix_index_merge_panic

XuHuaiyu added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Nov 30, 2021

guo-shaoge changed the title ~~executor: add log when index/table worker panic for IndexMergeReader~~ executor: fix IndexMerge worker panic because of closed channel Jan 6, 2022

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 6, 2022

executor: fix IndexMerge worker panic because of closed channel

a248ed4

Signed-off-by: guo-shaoge <shaoge1994@163.com>

ti-chi-bot requested review from wshwsh12 and XuHuaiyu January 6, 2022 14:06

add failpoint case

651d74e

Signed-off-by: guo-shaoge <shaoge1994@163.com>

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2022

guo-shaoge added 2 commits January 7, 2022 11:33

Merge branch 'master' of https://github.com/pingcap/tidb into fix_ind…

8303a7f

…ex_merge_panic Signed-off-by: guo-shaoge <shaoge1994@163.com>

fix fmt

e8d716b

Signed-off-by: guo-shaoge <shaoge1994@163.com>

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2022

fix compile error

c45e064

Signed-off-by: guo-shaoge <shaoge1994@163.com>

Merge branch 'master' into fix_index_merge_panic

4bb720b

wshwsh12 reviewed Jan 11, 2022

View reviewed changes

executor/distsql.go Outdated Show resolved Hide resolved

executor/index_merge_reader.go Outdated Show resolved Hide resolved

Merge branch 'master' into fix_index_merge_panic

b87b0db

guo-shaoge and others added 3 commits January 17, 2022 13:57

fix

f145958

Signed-off-by: guo-shaoge <shaoge1994@163.com>

Merge branch 'fix_index_merge_panic' of github.com:guo-shaoge/tidb in…

aa7fa57

…to fix_index_merge_panic

Merge branch 'master' into fix_index_merge_panic

1431aec

XuHuaiyu reviewed Jan 17, 2022

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2022

guo-shaoge closed this Mar 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: fix IndexMerge worker panic because of closed channel #29803

executor: fix IndexMerge worker panic because of closed channel #29803

guo-shaoge commented Nov 15, 2021 •

edited

Loading

ti-chi-bot commented Nov 15, 2021

guo-shaoge commented Nov 16, 2021

guo-shaoge commented Nov 24, 2021

guo-shaoge commented Jan 6, 2022

guo-shaoge commented Jan 6, 2022

sre-bot commented Jan 8, 2022 •

edited

Loading

guo-shaoge commented Jan 17, 2022

guo-shaoge commented Jan 17, 2022

XuHuaiyu left a comment

XuHuaiyu Jan 17, 2022

XuHuaiyu Jan 17, 2022

XuHuaiyu Jan 17, 2022

ti-chi-bot commented Feb 7, 2022

guo-shaoge commented Mar 10, 2023

executor: fix IndexMerge worker panic because of closed channel #29803

executor: fix IndexMerge worker panic because of closed channel #29803

Conversation

guo-shaoge commented Nov 15, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 15, 2021

guo-shaoge commented Nov 16, 2021

guo-shaoge commented Nov 24, 2021

guo-shaoge commented Jan 6, 2022

guo-shaoge commented Jan 6, 2022

sre-bot commented Jan 8, 2022 • edited Loading

guo-shaoge commented Jan 17, 2022

guo-shaoge commented Jan 17, 2022

XuHuaiyu left a comment

Choose a reason for hiding this comment

XuHuaiyu Jan 17, 2022

Choose a reason for hiding this comment

XuHuaiyu Jan 17, 2022

Choose a reason for hiding this comment

XuHuaiyu Jan 17, 2022

Choose a reason for hiding this comment

ti-chi-bot commented Feb 7, 2022

guo-shaoge commented Mar 10, 2023

guo-shaoge commented Nov 15, 2021 •

edited

Loading

sre-bot commented Jan 8, 2022 •

edited

Loading