*: update stats using query feedback #6197
Merged
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
|
/run-all-tests |
shenli
added
priority/P1
shenli
reviewed
Apr 2, 2018
domain/domain.go
Outdated
| continue | ||
| } | ||
| err = statsHandle.HandleUpdateStats() | ||
| if err != nil { |
There was a problem hiding this comment.
I think it is better to add metrics in a separate pr, it is already too large now.
| } | ||
| case <-dumpFeedbackTicker.C: | ||
| err = statsHandle.DumpStatsFeedbackToKV() | ||
| if err != nil { |
There was a problem hiding this comment.
I will do it in a separate pr.
statistics/cmsketch.go
Outdated
| @@ -54,6 +54,17 @@ func (c *CMSketch) InsertBytes(bytes []byte) { | |||
| } | |||
| } | |||
|
|
|||
| func (c *CMSketch) setValue(h1, h2 uint64, count uint32) { | |||
| return buf.Bytes(), nil | ||
| } | ||
|
|
||
| func decodeFeedback(val []byte, q *QueryFeedback, c *CMSketch) error { |
There was a problem hiding this comment.
Add comments for the following code logic.
| } | ||
|
|
||
| for _, t := range tests { |
| @@ -190,6 +191,100 @@ func (h *Handle) dumpTableStatDeltaToKV(id int64, delta variable.TableDelta) (bo | |||
| return updated, errors.Trace(err) | |||
| } | |||
|
|
|||
| // DumpStatsFeedbackToKV dumps the stats feedback to KV. | |||
There was a problem hiding this comment.
Do we need to add a few metrics for the following operations?
There was a problem hiding this comment.
Yes, I will do it in a separate pr.
zz-jason
reviewed
Apr 3, 2018
statistics/handle.go
Outdated
| // For now, we do not use the query feedback, so just set it to 1. | ||
| const maxQueryFeedBackCount = 1 | ||
| // MaxQueryFeedbackCount is the max number of feedback that cache in memory. | ||
| var MaxQueryFeedbackCount = 1000 |
coocood
reviewed
Apr 3, 2018
statistics/update.go
Outdated
| if err != nil { | ||
| return errors.Trace(err) | ||
| } | ||
| sql := fmt.Sprintf("delete from mysql.stats_feedback where table_id = %d and hist_id = %d and is_index = %d", tableID, hist.ID, isIndex) |
There was a problem hiding this comment.
May the delete exceeds transaction limit?
coocood
reviewed
Apr 3, 2018
statistics/feedback.go
Outdated
| @@ -493,3 +493,127 @@ func buildNewHistogram(h *Histogram, buckets []bucket) *Histogram { | |||
| } | |||
| return hist | |||
| } | |||
|
|
|||
| // QueryFeedbackPB is used to serialize the QueryFeedback. | |||
| type QueryFeedbackPB struct { | |||
There was a problem hiding this comment.
The name has PB but It's not protobuf.
Do you plan to change it to protobuf in the future?
winoros
reviewed
Apr 4, 2018
| @@ -243,7 +243,7 @@ func (s *testSuite) TestAggregation(c *C) { | |||
|
|
|||
| result = tk.MustQuery("select count(*) from information_schema.columns") | |||
| // When adding new memory columns in information_schema, please update this variable. | |||
| columnCountOfAllInformationSchemaTables := "737" | |||
| columnCountOfAllInformationSchemaTables := "741" | |||
There was a problem hiding this comment.
I think it is 4 column added.
|
LGTM |
shenli
reviewed
Apr 4, 2018
| @@ -54,6 +54,18 @@ func (c *CMSketch) InsertBytes(bytes []byte) { | |||
| } | |||
| } | |||
|
|
|||
| // setValue sets the count for value that hashed into (h1, h2). | |||
There was a problem hiding this comment.
No, it means the hash value pair.
|
PTAL @winoros |
winoros
reviewed
Apr 9, 2018
statistics/feedback.go
Outdated
| func encodeFeedback(q *QueryFeedback) ([]byte, error) { | ||
| var pb *queryFeedback | ||
| var err error | ||
| if q.hist.tp.Tp == mysql.TypeLong { |
There was a problem hiding this comment.
Can we make sure that the Tp there is only TypeLong? Not possible to be TypeLongLong?
zz-jason
reviewed
Apr 9, 2018
statistics/update.go
Outdated
|
|
||
| // HandleUpdateStats update the stats using feedback. | ||
| func (h *Handle) HandleUpdateStats(is infoschema.InfoSchema) error { | ||
| sql := fmt.Sprintf("select table_id, hist_id, is_index, feedback from mysql.stats_feedback order by table_id, hist_id, is_index") |
added 2 commits
April 9, 2018 15:52
zz-jason
reviewed
Apr 9, 2018
statistics/update.go
Outdated
| } | ||
| sql := fmt.Sprintf("insert into mysql.stats_feedback (table_id, hist_id, is_index, feedback) values "+ | ||
| "(%d, %d, %d, X'%X')", fb.tableID, fb.hist.ID, isIndex, vals) | ||
| _, err = h.ctx.(sqlexec.SQLExecutor).Execute(context.TODO(), sql) |
There was a problem hiding this comment.
why not using RestrictedSQLExecutor ?
winoros
added
conflicting
status/LGT2
|
PTAL @winoros |
winoros
approved these changes
Apr 10, 2018
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.
This pr dumps the collected feedback into kv and use these feedback to update the stats, including the histogram and CM Sketch.
PTAL @coocood @winoros @shenli