Skip to content
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.

Sign up for GitHub

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

Jump to bottom

plan/statistics: concurrent build columns #2713

Merged
merged 14 commits into from
Mar 22, 2017
Merged

plan/statistics: concurrent build columns #2713

merged 14 commits into from
Mar 22, 2017

Conversation

alivxxx
Copy link
Contributor

@alivxxx alivxxx commented Feb 23, 2017

@hanfei1991
Copy link
Member

Any bench result to prove the improvement?

@alivxxx
Copy link
Contributor Author

alivxxx commented Feb 24, 2017 •
edited
Loading

Bench on a table of 9 coulmns,4 single column index,3 double column index,100000 random generated rows using my laptop.
Before the change,the analyze time is 12.47s, after the change, analyze time is 6.36s.
I find that if concurrent build column, the time for single index scan is 4 times slower than sequentially build. So while the concurrency is 8, there is only 2 times faster.
@hanfei1991

@coocood
Copy link
Member

coocood commented Feb 28, 2017

The execution time is not important for analyze table, we need it to have minimum impact on the system.
So I think we should not make analyze concurrent.

@alivxxx alivxxx closed this Mar 1, 2017
@alivxxx alivxxx deleted the xhb/concurrent-build-stats branch March 1, 2017 14:23
@hanfei1991 hanfei1991 restored the xhb/concurrent-build-stats branch March 7, 2017 07:58
@hanfei1991 hanfei1991 reopened this Mar 7, 2017
@hanfei1991
Copy link
Member

Execution time is very important for user @coocood

hanfei1991
hanfei1991 reviewed Mar 7, 2017
View reviewed changes
plan/statistics/statistics.go Outdated
@@ -395,12 +396,13 @@ func (t *Table) build4SortedColumn(sc *variable.StatementContext, offset int, re
}
var valuesPerBucket, lastNumber, bucketIdx int64 = 1, 0, 0
knowCount := true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need know count in concurrent enviroument

@coocood
Copy link
Member

coocood commented Mar 7, 2017

@hanfei1991
At least we should keep the original none-concurrent execution and make it configurable.

@ngaut
Copy link
Member

ngaut commented Mar 7, 2017

Can we use a session variable to control the concurrency ?

@zimulala
Copy link
Contributor

zimulala commented Mar 7, 2017

PTAL @lamxTyler

hanfei1991
hanfei1991 reviewed Mar 7, 2017
View reviewed changes
plan/statistics/statistics_test.go
@@ -190,21 +190,21 @@ func (s *testStatisticsSuite) TestTable(c *C) {
c.Check(count, Equals, int64(1))
count, err = col.LessRowCount(sc, types.NewIntDatum(20000))
c.Check(err, IsNil)
c.Check(count, Equals, int64(19980))
c.Check(count, Equals, int64(19984))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the test result be changed ?

Copy link
Contributor Author

@alivxxx alivxxx Mar 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there was a bug when caculate the bucketIdx after merge buckets.

@hanfei1991
Copy link
Member

Well, we can add a concurrency argument for analyze, just like what we do for Join concurrency. If you don't think it will add the complexity. Now the question is , what should the default value be ?
@coocood @ngaut

tiancaiamao
tiancaiamao reviewed Mar 8, 2017
View reviewed changes
Copy link
Contributor

@tiancaiamao tiancaiamao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @coocood , concurrent introduce too much complexity, which is our dangerous enemy. It bring than less benefit than gain.

plan/statistics/statistics.go Outdated
go b.buildMultiColumns(t, offsets, i*groupSize, isSorted, doneCh)
}
for range splittedOffsets {
err := <-doneCh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If error happened, then this function return, leave doneCh alone.
worker goroutine still waiting for write to doneCh, and would block forever, then goroutine leak .....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following the logic at https://github.com/pingcap/tidb/blob/master/domain/domain.go#L103.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiancaiamao The length of channel is len(splittedOffsets). It will never be blocked. PTAL

coocood
coocood reviewed Mar 13, 2017
View reviewed changes
sessionctx/variable/sysvar.go Outdated
@@ -598,6 +599,7 @@ var defaultSysVars = []*SysVar{
{ScopeSession, TiDBSkipConstraintCheck, "0"},
{ScopeSession, TiDBSkipDDLWait, "0"},
{ScopeSession, TiDBOptAggPushDown, "ON"},
{ScopeSession, BuildStatsConcurrencyVar, "4"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default should be 1.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To minmum performance impact, reduce the risk.

@hanfei1991
Copy link
Member

LGTM

@hanfei1991
Copy link
Member

@coocood @tiancaiamao PTAL

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 20, 2017
@shenli
Copy link
Member

shenli commented Mar 22, 2017

@lamxTyler Please resolve the conflicts.

@shenli
Copy link
Member

shenli commented Mar 22, 2017

Rest LGTM

shenli
shenli approved these changes Mar 22, 2017
View reviewed changes
Copy link
Member

@shenli shenli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shenli shenli merged commit 60bcd98 into master Mar 22, 2017
@shenli shenli deleted the xhb/concurrent-build-stats branch March 22, 2017 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanfei1991 hanfei1991 hanfei1991 left review comments

@tiancaiamao tiancaiamao tiancaiamao left review comments

@coocood coocood coocood left review comments

@shenli shenli shenli approved these changes

Assignees
No one assigned
Labels
status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@alivxxx @hanfei1991 @coocood @ngaut @zimulala @shenli @tiancaiamao