executor: break innerTable fetcher is error happend during fetching outer table #7554
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zz-jason
reviewed
Aug 30, 2018
executor/join.go
Outdated
| chk := e.innerResult.GetChunk(i) | ||
| for j := 0; j < chk.NumRows(); j++ { | ||
| if e.finished.Load().(bool) { |
There was a problem hiding this comment.
This check may slow down the hash table building performance.
There was a problem hiding this comment.
maybe we only need to check e.finished in the granularity of Chunk?
alivxxx
reviewed
Aug 30, 2018
| @@ -264,6 +264,9 @@ func (e *HashJoinExec) fetchInnerRows(ctx context.Context) (err error) { | |||
| e.innerResult.GetMemTracker().AttachTo(e.memTracker) | |||
| e.innerResult.GetMemTracker().SetLabel("innerResult") | |||
| for { | |||
| if e.finished.Load().(bool) { | |||
There was a problem hiding this comment.
If the outer table error happened after this and before Next, there is still race.
There was a problem hiding this comment.
we need a wait group or mutex to block the close operation on child until this goroutine finishes.
zz-jason
reviewed
Aug 30, 2018
| @@ -533,6 +536,9 @@ func (e *HashJoinExec) buildHashTableForList() error { | |||
| valBuf = make([]byte, 8) | |||
| ) | |||
| for i := 0; i < e.innerResult.NumChunks(); i++ { | |||
| if e.finished.Load().(bool) { | |||
There was a problem hiding this comment.
this check can be removed, because we only need to guarantee not calling the next function of the child.
zz-jason
approved these changes
Aug 30, 2018
|
/run-all-tests |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Break fetchInnerAndBuildHashTable if error happened during fetchOuterAndProbeHashTable.
What is changed and how it works?
Before this PR, if any error happens during fetchOuterAndProbeHashTable,
fetchInnerAndBuildHashTable will continue running, this will cause some
unexpected situations such as DataRace which would may by closing HashJoinExec
while fetchOuterAndProbeHashTable is still running.
This PR checks HashJoinExec.finished evecy time in the loop of
fetchInnerAndBuildHashTable to exists in time if any error happens.
Check List
Tests
Code changes
Side effects
Related changes