executor: fix IndexMerge handle panic logic (#41036) #41063

ti-chi-bot · 2023-02-05T07:42:45Z

This is an automated cherry-pick of #41036

What problem does this PR solve?

Issue Number: close #41047

Problem Summary:
There are five kinds of workers in IndexMergeReaderExecutor:

partialIndexWorker/partialTableWorker: get handles from rows, then send handles to processWorker.
processWorker: do Union or Intersection of handles., then send handles to tableScanWorker. Also it send task to main goroutine using resultCh.
tableScanWorker: fetch row from tikv using handles, then send rows to main goroutine
partTblIntersectionWorker: For dynamic partition table, for each partition, start a goroutine to do intersection. This is to speed up intersection processing.
main goroutine: wait for tableScanWorker to get rows.

Why panic:

processWorker finish, so it close resultCh. code
partialIndexWorker got error, then try to send error using resultCh. code
got write to closed channel panic.

What is changed and how it works?

partialIndexWorker/partialTableWorker should not use resultCh to send error. Instead should use fetchCh, which is used to send task to processWorker.
processWorker should check err in task, which is received from fetchCh. If got error, just stop processing.
add handleIntersectionProcessWorkerPanic(), which is similar to handleLoopFetcherPanic. The latter will close(resultCh) when return.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
1. load data using lighning, too large to upload
2. run sql as follows, and tidb doesn't crash when SQL is canceled because oom.

use s11_indexmerge_test;

set global tidb_server_memory_limit="500MB";
explain analyze select /*+ use_index_merge(tp2) */ * from tp2 where a < 40000000 or b < 40000000;
explain analyze select /*+ use_index_merge(tp2, ia, ib) */ * from tp2 where a < 40000000 and b < 40000000;

set tidb_mem_quota_query = 1000;
explain analyze select /*+ use_index_merge(tp2) */ * from tp2 where a < 40000000 or b < 40000000;
explain analyze select /*+ use_index_merge(tp2, ia, ib) */ * from tp2 where a < 40000000 and b < 40000000;

set tidb_mem_quota_query = 10<<20;
explain analyze select /*+ use_index_merge(tp2) */ * from tp2 where a < 40000000 or b < 40000000;
explain analyze select /*+ use_index_merge(tp2, ia, ib) */ * from tp2 where a < 40000000 and b < 40000000;

set tidb_mem_quota_query = 500<<20;
explain analyze select /*+ use_index_merge(tp2) */ * from tp2 where a < 40000000 or b < 40000000;
explain analyze select /*+ use_index_merge(tp2, ia, ib) */ * from tp2 where a < 40000000 and b < 40000000;
                                                                                                                                                                                                                                                            set tidb_mem_quota_query = 1<<30;
explain analyze select /*+ use_index_merge(tp2) */ * from tp2 where a < 40000000 or b < 40000000;
explain analyze select /*+ use_index_merge(tp2, ia, ib) */ * from tp2 where a < 40000000 and b < 40000000;

No code

Side effects

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

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

executor: fix IndexMerge handle panic logic

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2023-02-05T07:42:47Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

XuHuaiyu
windtalker

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 <[email protected]>

guo-shaoge · 2023-02-06T09:46:25Z

/merge

ti-chi-bot · 2023-02-06T09:46:29Z

@guo-shaoge: /merge in this pull request requires 2 approval(s).

In response to this:

/merge

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 ti-community-infra/tichi repository.

…k-41036-to-release-6.5 Signed-off-by: guo-shaoge <[email protected]>

Signed-off-by: guo-shaoge <[email protected]>

guo-shaoge · 2023-02-06T11:55:36Z

/test build

windtalker

LGTM

guo-shaoge · 2023-02-09T02:36:58Z

/merge

ti-chi-bot · 2023-02-09T02:37:01Z

This pull request has been accepted and is ready to merge.

Commit hash: 6574662

ti-chi-bot · 2023-02-09T04:48:11Z

@ti-chi-bot: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

purelind · 2023-02-09T05:29:41Z

/retest

guo-shaoge · 2023-02-10T02:05:08Z

/retest

guo-shaoge · 2023-02-10T02:26:01Z

/test unit-test

guo-shaoge · 2023-02-10T03:28:56Z

/test unit-test

guo-shaoge · 2023-02-10T09:34:10Z

/test unit-test

hawkingrei · 2023-02-10T09:40:14Z

/retest

purelind · 2023-02-10T10:20:33Z

/retest

guo-shaoge · 2023-02-10T15:08:36Z

No idea why this case fail

   testing.go:1319: race detected during execution of test
--- FAIL: TestCaptureUserFilter (80.73s)

WARNING: DATA RACE
Write at 0x00c00dd59d10 by goroutine 22028:
  runtime.mapassign_faststr()
      GOROOT/src/runtime/map_faststr.go:203 +0x0
  github.com/pingcap/tidb/util/stmtsummary.(*stmtSummaryByDigestElement).add()
      util/stmtsummary/statement_summary.go:708 +0x166
  github.com/pingcap/tidb/util/stmtsummary.(*stmtSummaryByDigest).add()
      util/stmtsummary/statement_summary.go:616 +0xb2
  github.com/pingcap/tidb/util/stmtsummary.(*stmtSummaryByDigestMap).AddStatement()
      util/stmtsummary/statement_summary.go:340 +0x590
  github.com/pingcap/tidb/executor.(*ExecStmt).SummaryStmt()
      executor/adapter.go:1819 +0x1bf1
  github.com/pingcap/tidb/executor.(*ExecStmt).FinishExecuteStmt()
      executor/adapter.go:1388 +0x7de
  github.com/pingcap/tidb/executor.(*ExecStmt).CloseRecordSet()
      executor/adapter.go:1418 +0x64
  github.com/pingcap/tidb/executor.(*recordSet).Close()
      executor/adapter.go:183 +0xcb
  github.com/pingcap/tidb/session.(*execStmtResult).Close()
      session/session.go:2405 +0x7e
  github.com/pingcap/tidb/testkit.(*TestKit).MustExecWithContext()
      testkit/testkit.go:132 +0x28f
  github.com/pingcap/tidb/testkit.(*TestKit).MustExec()
      testkit/testkit.go:122 +0xf7
  github.com/pingcap/tidb/bindinfo_test.TestCaptureUserFilter()
      bindinfo/capture_test.go:645 +0xb5a
  github.com/pingcap/tidb/session.runStmt()
      session/session.go:2350 +0x85c
  github.com/pingcap/tidb/session.(*session).ExecuteStmt()
      session/session.go:2214 +0x1266
  github.com/pingcap/tidb/testkit.(*TestKit).ExecWithContext()
      testkit/testkit.go:318 +0x84e
  github.com/pingcap/tidb/testkit.(*TestKit).MustExecWithContext()
      testkit/testkit.go:127 +0xb7
  github.com/pingcap/tidb/testkit.(*TestKit).MustExec()
      testkit/testkit.go:122 +0xf7
  github.com/pingcap/tidb/bindinfo_test.TestCaptureUserFilter()
      bindinfo/capture_test.go:613 +0xd0
  github.com/pingcap/tidb/domain.(*Domain).rebuildSysVarCache()
      domain/sysvar_cache.go:146 +0x73c
  github.com/pingcap/tidb/domain.(*Domain).LoadSysVarCacheLoop()
      domain/domain.go:1370 +0xb9
  github.com/pingcap/tidb/session.BootstrapSession()
      session/session.go:3315 +0x78c
  github.com/pingcap/tidb/domain.(*Domain).GetSessionCache()
      domain/sysvar_cache.go:62 +0x6a
  github.com/pingcap/tidb/session.(*session).loadCommonGlobalVariablesIfNeeded()
      session/session.go:3567 +0x11e
  github.com/pingcap/tidb/session.(*session).ExecuteStmt()
      session/session.go:2129 +0x2fb
  github.com/pingcap/tidb/session.(*session).ExecuteInternal()
      session/session.go:1675 +0x4eb
  github.com/pingcap/tidb/domain.(*Domain).LoadPrivilegeLoop()
      domain/domain.go:1312 +0x143
  github.com/pingcap/tidb/session.BootstrapSession()
      session/session.go:3308 +0x72c
  github.com/pingcap/tidb/testkit.bootstrap()
      testkit/mockstore.go:84 +0x84
  github.com/pingcap/tidb/testkit.CreateMockStoreAndDomain()
      testkit/mockstore.go:69 +0xd0
  github.com/pingcap/tidb/bindinfo_test.TestCaptureUserFilter()
      bindinfo/capture_test.go:609 +0x58
  testing.tRunner()
      GOROOT/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      GOROOT/src/testing/testing.go:1493 +0x47

Previous read at 0x00c00dd59d10 by goroutine 38466:
  runtime.mapiternext()
      GOROOT/src/runtime/map.go:866 +0x0
  github.com/pingcap/tidb/bindinfo.(*BindHandle).CaptureBaselines()
      bindinfo/handle.go:906 +0x1053
  github.com/pingcap/tidb/domain.(*Domain).globalBindHandleWorkerLoop.func1()
      domain/domain.go:1487 +0x519

Goroutine 22028 (running) created at:
  testing.(*T).Run()
      GOROOT/src/testing/testing.go:1493 +0x75d
  testing.runTests.func1()
      GOROOT/src/testing/testing.go:1846 +0x99
  testing.tRunner()
      GOROOT/src/testing/testing.go:1446 +0x216
  testing.runTests()
      GOROOT/src/testing/testing.go:1844 +0x7ec
  testing.(*M).Run()
      GOROOT/src/testing/testing.go:1726 +0xa84
  go.uber.org/goleak.VerifyTestMain()
      external/org_uber_go_goleak/testmain.go:53 +0x70
  github.com/pingcap/tidb/bindinfo_test.TestMain()
      bindinfo/main_test.go:32 +0x43c
  main.main()
      bazel-out/k8-fastbuild/bin/bindinfo/bindinfo_test_/testmain.go:276 +0x7c9

Goroutine 38466 (running) created at:
  github.com/pingcap/tidb/domain.(*Domain).globalBindHandleWorkerLoop()
      domain/domain.go:1461 +0x10a
  github.com/pingcap/tidb/domain.(*Domain).LoadBindInfoLoop()
      domain/domain.go:1454 +0x1f5
  github.com/pingcap/tidb/session.BootstrapSession()
      session/session.go:3302 +0x688
  github.com/pingcap/tidb/testkit.bootstrap()
      testkit/mockstore.go:84 +0x84
  github.com/pingcap/tidb/testkit.CreateMockStoreAndDomain()
      testkit/mockstore.go:69 +0xd0
  github.com/pingcap/tidb/bindinfo_test.TestCaptureUserFilter()
      bindinfo/capture_test.go:609 +0x58
  testing.tRunner()
      GOROOT/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      GOROOT/src/testing/testing.go:1493 +0x47
==================

hawkingrei · 2023-02-11T18:01:28Z

/retest

This is an automated cherry-pick of pingcap#41036

ca88fc2

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Feb 5, 2023

executor: fix IndexMerge handle panic logic #41036

Merged

12 tasks

ti-chi-bot assigned guo-shaoge Feb 5, 2023

guo-shaoge requested review from XuHuaiyu and windtalker February 5, 2023 08:08

fix conflicts

090a571

Signed-off-by: guo-shaoge <[email protected]>

XuHuaiyu approved these changes Feb 6, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 6, 2023

VelocityLight added cherry-pick-approved Cherry pick PR approved by release team. and removed do-not-merge/cherry-pick-not-approved labels Feb 6, 2023

fix const var

4a0fcc4

Signed-off-by: guo-shaoge <[email protected]>

guo-shaoge added 2 commits February 6, 2023 17:46

Merge branch 'release-6.5' of github.com:pingcap/tidb into cherry-pic…

6bdd527

…k-41036-to-release-6.5 Signed-off-by: guo-shaoge <[email protected]>

fix conflicts

6574662

Signed-off-by: guo-shaoge <[email protected]>

windtalker approved these changes Feb 9, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 9, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 9, 2023

ti-chi-bot added 2 commits February 9, 2023 10:37

Merge branch 'release-6.5' into cherry-pick-41036-to-release-6.5

7114142

Merge branch 'release-6.5' into cherry-pick-41036-to-release-6.5

46c228f

ti-chi-bot merged commit 99f338d into pingcap:release-6.5 Feb 11, 2023

windtalker mentioned this pull request Mar 7, 2023

add v6.5.1 release notes pingcap/docs-cn#13279

Merged

16 tasks

VelocityLight added do-not-merge/cherry-pick-not-approved cherry-pick-approved Cherry pick PR approved by release team. and removed cherry-pick-approved Cherry pick PR approved by release team. do-not-merge/cherry-pick-not-approved labels Mar 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: fix IndexMerge handle panic logic (#41036) #41063

executor: fix IndexMerge handle panic logic (#41036) #41063

ti-chi-bot commented Feb 5, 2023

ti-chi-bot commented Feb 5, 2023 •

edited

Loading

guo-shaoge commented Feb 6, 2023

ti-chi-bot commented Feb 6, 2023

guo-shaoge commented Feb 6, 2023

windtalker left a comment

guo-shaoge commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

purelind commented Feb 9, 2023

guo-shaoge commented Feb 10, 2023

guo-shaoge commented Feb 10, 2023

guo-shaoge commented Feb 10, 2023

guo-shaoge commented Feb 10, 2023

hawkingrei commented Feb 10, 2023

purelind commented Feb 10, 2023

guo-shaoge commented Feb 10, 2023

hawkingrei commented Feb 11, 2023

executor: fix IndexMerge handle panic logic (#41036) #41063

executor: fix IndexMerge handle panic logic (#41036) #41063

Conversation

ti-chi-bot commented Feb 5, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Feb 5, 2023 • edited Loading

guo-shaoge commented Feb 6, 2023

ti-chi-bot commented Feb 6, 2023

guo-shaoge commented Feb 6, 2023

windtalker left a comment

Choose a reason for hiding this comment

guo-shaoge commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

purelind commented Feb 9, 2023

guo-shaoge commented Feb 10, 2023

guo-shaoge commented Feb 10, 2023

guo-shaoge commented Feb 10, 2023

guo-shaoge commented Feb 10, 2023

hawkingrei commented Feb 10, 2023

purelind commented Feb 10, 2023

guo-shaoge commented Feb 10, 2023

hawkingrei commented Feb 11, 2023

ti-chi-bot commented Feb 5, 2023 •

edited

Loading