New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
executor: fix a bug when join exists in subquery. #2106
Conversation
@@ -484,6 +486,9 @@ type hashJoinCtx struct { | |||
|
|||
// Close implements the Executor Close interface. | |||
func (e *HashJoinExec) Close() error { | |||
e.finished = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean
e.closeMu.Lock()
e.finished = true
e.closeMu.Unlock()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, The e.finished should be put front to notify the goroutines to stop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so what's the following two line for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add Lock for waiting waitJoinWorkersAndCloseResultChan unlock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using a channel to notify is better. @hanfei1991 @tiancaiamao
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to use a channel to implement it ? @zimulala
8fd61dc
to
f83fb69
Compare
@tiancaiamao @zimulala PTAL |
e.finished.Store(true) | ||
select { | ||
case _ = <-e.closeLock: | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to select and use <-e.closeLock
directly.
wg sync.WaitGroup | ||
// closeMu add a lock for closing executor. | ||
closeLock chan bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using struct {}
type is better.
d264bb7
to
ab91907
Compare
LGTM |
47afaff
to
84b2109
Compare
LGTM |
When a join exitsts in subquery, the Close will be called before all rows are processed. In this case, we must ensure all channels have been closed.
fix #2009
@shenli @coocood @zimulala @winoros @tiancaiamao @XuHuaiyu PTAL