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 order by bug #4470

Merged
merged 7 commits into from Sep 8, 2017

Conversation

Projects
None yet
3 participants
@tiancaiamao
Contributor

tiancaiamao commented Sep 7, 2017

tiancaiamao added some commits Sep 7, 2017

@@ -353,64 +358,45 @@ func (ih *indexWorker) fetchHandles(e *IndexLookUpExecutor, result distsql.Selec
case <-finished:
return
case workCh <- task:
e.resultCh <- task

This comment has been minimized.

@coocood

coocood Sep 7, 2017

Member

I think there need another select, if resultCh is full, and IndexLookupExecutor stopped reading then call Close, indexWorker may be blocked here forever.

But it only need to select finished, ctx.Done can be handled in the next loop.

The old implementation before refactor have this issue, it's very hard to trigger though.

@coocood

coocood Sep 7, 2017

Member

I think there need another select, if resultCh is full, and IndexLookupExecutor stopped reading then call Close, indexWorker may be blocked here forever.

But it only need to select finished, ctx.Done can be handled in the next loop.

The old implementation before refactor have this issue, it's very hard to trigger though.

This comment has been minimized.

@tiancaiamao

tiancaiamao Sep 8, 2017

Contributor

TestIndexDoubleReadClose actually cover the case and that's why my first commit fail to pass CI.
I've fix it by drain the resultCh in IndexLookUpExecutor.Close https://github.com/pingcap/tidb/pull/4470/files#diff-f77833966b29c503e5221562eb37f86fR552

@tiancaiamao

tiancaiamao Sep 8, 2017

Contributor

TestIndexDoubleReadClose actually cover the case and that's why my first commit fail to pass CI.
I've fix it by drain the resultCh in IndexLookUpExecutor.Close https://github.com/pingcap/tidb/pull/4470/files#diff-f77833966b29c503e5221562eb37f86fR552

tasksErr: errors.Trace(err),
doneCh := make(chan error, 1)
doneCh <- errors.Trace(err)
e.resultCh <- &lookupTableTask{

This comment has been minimized.

@coocood

coocood Sep 7, 2017

Member

Need to select finished channel.

@coocood

coocood Sep 7, 2017

Member

Need to select finished channel.

This comment has been minimized.

@tiancaiamao

tiancaiamao Sep 8, 2017

Contributor

If finished is closed, means we've close the IndexLookUpExecutor and don't care the result any more.
IndexLookUpExecutor.Close() would drain the remain lookupTableTask and discard them.
Both the same result, so select finished here or not doesn't make any difference.

@tiancaiamao

tiancaiamao Sep 8, 2017

Contributor

If finished is closed, means we've close the IndexLookUpExecutor and don't care the result any more.
IndexLookUpExecutor.Close() would drain the remain lookupTableTask and discard them.
Both the same result, so select finished here or not doesn't make any difference.

@hanfei1991

This comment has been minimized.

Show comment
Hide comment
@hanfei1991

hanfei1991 Sep 8, 2017

Member

LGTM

Member

hanfei1991 commented Sep 8, 2017

LGTM

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 8, 2017

Contributor

/run-all-test

Contributor

tiancaiamao commented Sep 8, 2017

/run-all-test

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 8, 2017

Member

LGTM

Member

coocood commented Sep 8, 2017

LGTM

@coocood

coocood approved these changes Sep 8, 2017

@tiancaiamao tiancaiamao merged commit 7cdbceb into master Sep 8, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@tiancaiamao tiancaiamao deleted the tiancaiamao/fix-orderby branch Sep 8, 2017

@coocood coocood referenced this pull request Oct 16, 2017

Closed

sql query exception #4788

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment