*: avoid special cases DATA RACE #38918

keeplearning20221 · 2022-11-05T09:59:11Z

What problem does this PR solve?

Issue Number: ref #38914

Problem Summary:

What is changed and how it works?

Replacing alloc when sql executes an error. Prevent unfinished Goroutines from continuing to use session alloc

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
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.

None

ti-chi-bot · 2022-11-05T09:59:13Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

hawkingrei
tiancaiamao

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.

tiancaiamao · 2022-11-07T02:02:17Z

util/chunk/alloc.go

+		return col.elemBuf == nil
+	}
+
+	return id == cap(col.elemBuf)


s/id/sizeClass?

When this may differ? caused by exchange column?

(c *Column) Reset can change column type

modified: executor/builder.go modified: executor/distsql.go modified: executor/executor.go modified: executor/index_lookup_hash_join.go modified: executor/index_lookup_join.go modified: executor/index_lookup_merge_join.go modified: executor/index_merge_reader.go modified: executor/join.go modified: executor/joiner.go modified: executor/pipelined_window.go modified: executor/window.go modified: server/conn.go modified: server/conn_stmt.go modified: sessionctx/variable/session.go modified: sessionctx/variable/session_test.go modified: testkit/testkit.go

sessionctx/variable/session.go

modified: sessionctx/variable/session.go

…tidb into fix_reuse_bug

modified: sessionctx/variable/session.go modified: sessionctx/variable/session_test.go

keeplearning20221 · 2022-11-08T01:00:28Z

/run-check_dev_2

bob34007 · 2022-11-08T07:26:23Z

sessionctx/variable/session.go

@@ -1354,8 +1341,20 @@ func (s *SessionVars) SetAlloc(alloc chunk.Allocator) {
 }

 // ClearAlloc indicates stop reuse chunk
-func (s *SessionVars) ClearAlloc() {
+func (s *SessionVars) ClearAlloc(cc *chunk.Allocator, err bool) {


err is usually an abbreviation for error

hawkingrei · 2022-11-08T07:27:42Z

sessionctx/variable/session.go

@@ -96,6 +96,12 @@ type RetryInfo struct {
 	LastRcReadTS           uint64
 }

+// ReuseChunkPool save Alloc object
+type ReuseChunkPool struct {
+	Lock  sync.Mutex


In general. Mutex's name is mu.

XuHuaiyu · 2022-11-08T11:53:45Z

/merge

ti-chi-bot · 2022-11-08T11:53:49Z

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

Commit hash: 1e8259b

keeplearning20221 · 2022-11-08T12:09:05Z

/run-unit-test

ti-chi-bot · 2022-11-08T12:14:31Z

In response to a cherrypick label: new pull request created: #38981.

sre-bot · 2022-11-08T13:04:21Z

TiDB MergeCI notify

🔴 Bad News! New failing [1] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/common-test	🟥 failed 1, success 10, total 11	9 min 43 sec	New failing
idc-jenkins-ci-tidb/integration-ddl-test	🔴 failed 1, success 5, total 6	43 min	Existing failure
idc-jenkins-ci-tidb/mybatis-test	🔴 failed 1, success 0, total 1	11 min	Existing failure
idc-jenkins-ci/integration-cdc-test	✅ all 39 tests passed	21 min	Fixed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 17 tests passed	13 min	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	6 min 47 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	5 min 54 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	5 min 31 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 43 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

TonsnakeLin · 2022-11-08T14:03:07Z

Did you test the performance for transfer workload by this PR? I found that you replace the session.getNewChunk by Chunk.new and add a mutex to protect the pool. I worry about the final performance improvement.

keeplearning20221 · 2022-11-09T02:12:31Z

Did you test the performance for transfer workload by this PR? I found that you replace the session.getNewChunk by Chunk.new and add a mutex to protect the pool. I worry about the final performance improvement.

Tested, the performance impact is relatively small, and safety is the priority

add type verification

c7bcbb6

keeplearning20221 requested a review from a team as a code owner November 5, 2022 09:59

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 5, 2022

keeplearning20221 and others added 2 commits November 5, 2022 21:23

modified: ../../util/chunk/alloc.go

dc70678

Merge branch 'master' into fix_reuse_bug

c4c67af

tiancaiamao reviewed Nov 7, 2022

View reviewed changes

modified: util/chunk/alloc_test.go

4de7ea7

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 7, 2022

modified: util/chunk/alloc.go

cf03c32

tiancaiamao approved these changes Nov 7, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 7, 2022

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/needs-triage-completed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 7, 2022

keeplearning20221 changed the title ~~chunk: add type verification~~ *: avoid special cases DATA RACE Nov 7, 2022

Merge branch 'master' into fix_reuse_bug

81f3e52

hawkingrei reviewed Nov 7, 2022

View reviewed changes

sessionctx/variable/session.go Outdated Show resolved Hide resolved

keeplearning20221 added 2 commits November 7, 2022 23:01

modified: server/conn.go

1945f13

modified: sessionctx/variable/session.go

Merge branch 'fix_reuse_bug' of https://github.com/keeplearning20221/…

d90a1d2

…tidb into fix_reuse_bug

hawkingrei requested review from XuHuaiyu and bb7133 November 7, 2022 15:46

keeplearning20221 and others added 2 commits November 8, 2022 00:47

modified: executor/executor.go

8465cd3

modified: sessionctx/variable/session.go modified: sessionctx/variable/session_test.go

Merge branch 'master' into fix_reuse_bug

bb8e264

tiancaiamao approved these changes Nov 8, 2022

View reviewed changes

bob34007 reviewed Nov 8, 2022

View reviewed changes

hawkingrei reviewed Nov 8, 2022

View reviewed changes

modified: sessionctx/variable/session.go

07effdf

hawkingrei approved these changes Nov 8, 2022

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 Nov 8, 2022

keeplearning20221 added 3 commits November 8, 2022 15:57

Merge branch 'master' into fix_reuse_bug

d3c2f2a

Merge branch 'master' into fix_reuse_bug

78a7f9f

Merge branch 'master' into fix_reuse_bug

1e8259b

XuHuaiyu approved these changes Nov 8, 2022

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 8, 2022

Merge branch 'master' into fix_reuse_bug

d0856bf

ti-chi-bot added the needs-cherry-pick-release-6.4 label Nov 8, 2022

ti-chi-bot merged commit ea26284 into pingcap:master Nov 8, 2022

ti-chi-bot mentioned this pull request Nov 8, 2022

*: avoid special cases DATA RACE (#38918) #38981

Closed

12 tasks

keeplearning20221 deleted the fix_reuse_bug branch November 8, 2022 12:14

keeplearning20221 restored the fix_reuse_bug branch November 8, 2022 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: avoid special cases DATA RACE #38918

*: avoid special cases DATA RACE #38918

keeplearning20221 commented Nov 5, 2022 •

edited

Loading

ti-chi-bot commented Nov 5, 2022 •

edited

Loading

tiancaiamao Nov 7, 2022

keeplearning20221 Nov 7, 2022

keeplearning20221 commented Nov 8, 2022

bob34007 Nov 8, 2022

keeplearning20221 Nov 8, 2022

hawkingrei Nov 8, 2022

keeplearning20221 Nov 8, 2022

keeplearning20221 Nov 8, 2022

XuHuaiyu commented Nov 8, 2022

ti-chi-bot commented Nov 8, 2022

keeplearning20221 commented Nov 8, 2022

ti-chi-bot commented Nov 8, 2022

sre-bot commented Nov 8, 2022

TonsnakeLin commented Nov 8, 2022

keeplearning20221 commented Nov 9, 2022

*: avoid special cases DATA RACE #38918

*: avoid special cases DATA RACE #38918

Conversation

keeplearning20221 commented Nov 5, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 5, 2022 • edited Loading

tiancaiamao Nov 7, 2022

Choose a reason for hiding this comment

keeplearning20221 Nov 7, 2022

Choose a reason for hiding this comment

keeplearning20221 commented Nov 8, 2022

bob34007 Nov 8, 2022

Choose a reason for hiding this comment

keeplearning20221 Nov 8, 2022

Choose a reason for hiding this comment

hawkingrei Nov 8, 2022

Choose a reason for hiding this comment

keeplearning20221 Nov 8, 2022

Choose a reason for hiding this comment

keeplearning20221 Nov 8, 2022

Choose a reason for hiding this comment

XuHuaiyu commented Nov 8, 2022

ti-chi-bot commented Nov 8, 2022

keeplearning20221 commented Nov 8, 2022

ti-chi-bot commented Nov 8, 2022

sre-bot commented Nov 8, 2022

TiDB MergeCI notify

TonsnakeLin commented Nov 8, 2022

keeplearning20221 commented Nov 9, 2022

keeplearning20221 commented Nov 5, 2022 •

edited

Loading

ti-chi-bot commented Nov 5, 2022 •

edited

Loading