-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
*: add a PERFORMANCE_SCHEMA.slow_query table #8981
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8981 +/- ##
==========================================
- Coverage 67.6% 67.54% -0.06%
==========================================
Files 363 365 +2
Lines 75309 75426 +117
==========================================
+ Hits 50909 50944 +35
- Misses 19911 19986 +75
- Partials 4489 4496 +7
Continue to review full report at Codecov.
|
May I suggest changing the duration to |
infoschema/tiniub/init.go
Outdated
} | ||
dbInfo := &model.DBInfo{ | ||
ID: autoid.GenLocalSchemaID(), | ||
Name: model.NewCIStr("TiNiuB"), |
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.
Will the server fail to start if there is already a database which name is also 'TiNiuB'?
Maybe we should give some suggestions to the user when this happened in @lysu 's PR: 1. disable this plugin and start again, 2. rename the existed database.
btw: it is better to use an English name, not a Chinese Pinyin as the database name.
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.
re, it's better give tiniub
a English name? or can we direct put new table into PERFORMANCE_SCHEMA
or INFOMATION_SCHEMA
?
In mysql, we can write a information_schema plugin
to add a table to information_schema, https://dev.mysql.com/doc/refman/5.7/en/writing-information-schema-plugins.html
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.
if dbName == "information_schema" { | ||
return true | ||
} | ||
for _, driver := range drivers { |
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.
when do we call tiniub.Init()
? and do we have race-condition to access this drivers
? for current plugin is start time is ok...
but if drivers can be dynamic change it's better to take care race.
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.
tiniub.Init
is not yet used in this PR.
Your concern is right, if TiNiuB
is make into a plugin and load multiple times.
infoschema/tiniub/init.go
Outdated
} | ||
dbInfo := &model.DBInfo{ | ||
ID: autoid.GenLocalSchemaID(), | ||
Name: model.NewCIStr("TiNiuB"), |
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.
re, it's better give tiniub
a English name? or can we direct put new table into PERFORMANCE_SCHEMA
or INFOMATION_SCHEMA
?
In mysql, we can write a information_schema plugin
to add a table to information_schema, https://dev.mysql.com/doc/refman/5.7/en/writing-information-schema-plugins.html
// IterRecords rewrites the IterRecords method of slowQueryTable. | ||
func (s slowQueryTable) IterRecords(ctx sessionctx.Context, startKey kv.Key, cols []*table.Column, fn table.RecordIterFunc) error { | ||
dom := domain.GetDomain(ctx) | ||
result := dom.ShowSlowQuery(&ast.ShowSlow{Tp: 42}) |
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.
42
is a great number maybe added in ShowSlowType
? 😆
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.
A hack here because define a new ShowSlowType
need to update parser...
I think I should update parser and avoid hack
infoschema/tiniub/tables.go
Outdated
row = append(row, types.NewDatum(item.DB)) | ||
row = append(row, types.NewDatum(item.TableIDs)) | ||
row = append(row, types.NewDatum(item.IndexIDs)) | ||
row = append(row, types.NewDatum(item.Internal)) |
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 found it's maybe useful to combine pingcap/parser#32 sql digest into here, then we can agg a group similar sql's time in furture.
I just wonder that whether this pr will be merged into master? |
I had tried to move tables part to https://github.com/lysu/slow_log_table and integration it in #9006 (https://github.com/lysu/tidb/commit/a8b1f3f0ed51121ed795a4758743b3c7fee75d9b) It seems work at first glance - - but needs more refining. also found we can solve "can not work with go module problem" with https://github.com/lysu/slow_log_table/blob/master/go.mod#L8 |
I have some feedback with naming (sorry, not all of it is easily actionable):
Note: If you haven't seen sys before, for every view there is an
|
LGTM |
@tiancaiamao maybe need remove tiniub/tables.go and it will be package as plugin |
Hi @morgo @winoros |
What problem does this PR solve?
Create a virtual table
TiNiuB.slow_query
Compare to the
admin show slow
method, this virtual table if more flexible.Borrow ideas from https://github.com/TiNiuB/tiquery
This could serve as a plugin demo later @lysu
What is changed and how it works?
slow_query
table is similar toperfschema
tables, except that it's not standard, so a plugin maybe more applicable.Note that
tiniub.Init()
is not called in this PR, so theTiNiuB
database is not created by default.Check List
Tests