Merge lp:~dingqi-lxb/percona-server/5.5_log_queries_in_memory into lp:percona-server/5.5
Status: | Work in progress |
---|---|
Proposed branch: | lp:~dingqi-lxb/percona-server/5.5_log_queries_in_memory |
Merge into: | lp:percona-server/5.5 |
Diff against target: |
896 lines (+879/-0) 3 files modified
doc/source/diagnostics/slow_log_in_memory.rst (+74/-0) patches/series (+1/-0) patches/slowlog_in_memory.patch (+804/-0) |
To merge this branch: | bzr merge lp:~dingqi-lxb/percona-server/5.5_log_queries_in_memory |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Oleg Tsarev (community) | 2012-01-02 | Needs Fixing on 2012-01-11 | |
Review via email:
|
This proposal supersedes a proposal from 2011-12-29.
Description of the change
http://
patch for https:/
add a variable 'slowlog_
If the current_memory grows larger than max_memory, the eldest ones will be wash out.
Same action happens when max_mamory are set to a new value that is less than current_memory.
Oleg Tsarev (tsarev) wrote : | # |
8.
+ ulonglong utime_of_query= thd->current_
+ if (utime_of_query <= thd->variables.
+ return;
Please look into the slow_extended.
static inline ulonglong get_query_
...
thd->server_
Please better use this status variable, instead of calculate query execution time directly.
林晓斌 (dingqi-lxb) wrote : | # |
Thank you. Oleg, I have some items to discuss.
2. About innodb_trx_id. Please look to two following cases
In these two cases, the variable thd->innodb_trx_id is set to 0. It seems that I have to show it in the result table as 0, is there other result even use innodb_was_used ?
8.About calculating query execution time directly, it is because that I have to get the query_time, the status is not enough.
* Why plugin doesn't uninstall in test?
You mean that it shows " Plugin is busy and will be uninstalled on shutdown" in result-file? It is the same with the demo for audit plugin "null_audit", the strategy decided by mysqld
Oleg Tsarev (tsarev) wrote : | # |
On Mon, Dec 12, 2011 at 12:05 PM, 林晓斌 <email address hidden> wrote:
> Thank you. Oleg, I have some items to discuss.
>
> 2. About innodb_trx_id. Please look to two following cases
> In these two cases, the variable thd->innodb_trx_id is set to 0. It seems that I have to show it in the result table as 0, is there other result even use innodb_was_used ?
I think right decision is "NULL" value instead of 0.
>
> 8.About calculating query execution time directly, it is because that I have to get the query_time, the status is not enough.
>
> * Why plugin doesn't uninstall in test?
> You mean that it shows " Plugin is busy and will be uninstalled on shutdown" in result-file? It is the same with the demo for audit plugin "null_audit", the strategy decided by mysqld
Ok
> --
> https:/
> You are reviewing the proposed merge of lp:~dingqi-lxb/percona-server/5.5_log_queries_in_memory into lp:percona-server.
--
Oleg Tsarev, Software Engineer, Percona Inc.
林晓斌 (dingqi-lxb) wrote : | # |
2. About innodb_trx_id. Generally, when the thd->innodb_trx_id is 0, the value of "INNODB_TRX_ID" should show NULL, so the thd->innodb_
7. Please add tests to sys_vars suite for the all added variables.
The variable add in this plugin will not be seen in other test-case, it can only be seen the plugin is installed. So, is test-case in sys_vars suits necessary?
Stewart Smith (stewart) wrote : | # |
On Wed, 21 Dec 2011 02:50:35 -0000, 林晓斌 <email address hidden> wrote:
> +There are two plugins in the feature. They can be installed by
> command ``"install plugin SLOWLOG_
> 'slowlog_
> 'slowlog_
I think we should be consistent in naming. SLOW_LOG instead of SLOWLOG
and slow_log instead of slowlog.
--
Stewart Smith
Baron Schwartz (baron-xaprb) wrote : | # |
I'm sorry that I haven't noticed this discussion before, but I believe that it would be best to avoid a query time in milliseconds:
1. It should have microsecond resolution. Milliseconds is not enough.
2. It should be named EXEC_TIME and should be a floating-point value in seconds so the user does not have to convert the value to seconds.
Oleg Tsarev (tsarev) wrote : | # |
About documentation:
1) Please link to page page "Slow Log In Memory" to the index of documentation (to "Diagnostic" article)
2) "MS_QUERY_TIME – The query_exec_time of the query, in second."
But looks like time in the milliseconds
About the patch:
3) You avoid the "quiltrc" config file from the root of branch. As result your patch has incorrect format.
Please, use "quilt --quiltrc=quiltrc ..." while you touch the patches
About the feature:
4) I think, we should add at least following:
* User
* Query_time (microseconds)
About the tests:
5) Here is little example of incorrect work (JUST EXAMPLE):
http://
So, your implementation avoid all variables related to slow query log, except long_query_time.
* How to right test: modify all tests provided by slow_extended.
About the implementation:
6) Incorrect works with query exec time:
ulonglong utime_of_query= thd->current_
* Possible scenario: query doesn't pass to slow query log, but pass to INFORMATION_
* How to fix: you shoud use thd->server_status variable for detect insert or don't insert event to SLOW_LOG table. This allows to support all related options from PS.
* What requires the modify: slow_extended.
7) Separatelly about this:
+ ulonglong utime_of_query= thd->current_
+
+#ifndef DBUG_OFF
+ if (thd->variables
+ utime_of_query= thd->lex-
+#endif
+
+ if (utime_of_query <= thd->variables.
+ return;
Copy-paste - not a very good idea.
8) Class slow_log_list. Some methods lock/unlock mutexes, some not (pop_front). It think this is a bad contract-
9) Event Id can be missordered (different from order of events in the list)
10) Order of event can be missordered (different from slow query log)
11) Class slow_log_list. Why "first" and "last" are public members?
12) Please also note: subunit has a #bug 911237 - according to 5.5 testing not a work right now. Please merge fix then it will be merged to lp:percona-server
Oleg Tsarev (tsarev) wrote : | # |
To 12) - fix already in lp:percona-server, just need a merge before future tests
林晓斌 (dingqi-lxb) wrote : | # |
4)There is Query_time already, will be shown as 1.1234, in senconds, what does this Query_time mean?
6) ok, before recored it, I will check thd->server_status before. But the result of utime_of_query has to be re-calculated here all the same.
7) what does this mean?
9) Yes, the procedure of assigning event_id and putting into the list is not locked as a whole, for performance consideration. Ok, I will lock them as a whole.
10) This can not be confirmed. Because the procedre of logging into slow log and loging into memory are different plugins. Is there any proposal?
11) Because the "first" and "last" needs to be accessed at i_s_fill_
Oleg Tsarev (tsarev) wrote : | # |
4) This is Ok. Also please ask Baron about Lock_time
6) Not a "before", but "instead". You should not use direct utime_of_query calculation. Please replace any calculation by check of flag thd->server_status
7) This is mean you should not use copy-paste. Never.
9) Any perfomance consideration should not break the correct work
10) You can modify the slow_extended.patch and create event id on server-side. But this is requires consideration
11) Why you didn't add public interface for iteration instead of access to members?
林晓斌 (dingqi-lxb) wrote : | # |
6) Check can use thd->server_status, but I need to calculate the utime_of_query if the thd->server_status & SERVER_
10) It will make the plugin depart from the concept that it can be used in both Percona-server and Oracle-mysql directly. I propose that it remains the current strategy, if the misorder between memory and slow log can be tolerant.
Oleg Tsarev (tsarev) wrote : | # |
6) right
10) ok
1. Please add patch name to the "patches/series" file
2. About innodb_trx_id. Please look to two following cases:
* log_slow_verbosity doesn't set "innodb"
* query doesn't use the InnoDB
In first case slow query log will has no information about InnoDB
In second case slow query will have line "# No InnoDB statistics available for this query\n"
In this two cases innodb_trx_id will be incorrect, and you can't use and setup it.
Please handle variable innodb_was_used correctly (at least for innodb_trx_id)
3. Following is very, very bad: query= "NULL"; query_len= 5; SCHEMA. I_S_SLOW_ LOG WHERE QUERY IS NULL
+ if (general_query)
+ {
....
+ }
+ else
+ {
+ this->general_
+ this->general_
+ }
You are break SQL semantic. For example query:
SELECT * FROM INSFORMATION_
or any join will not work correctly.
You are break SQL language semantic by string "NULL" instead of NULL
Same for Schema.
4. I_S_SLOW_LOG not very good name. Why SLOW_LOG bad?
5. Please add documentation to the doc/
6. Sleep in the test file - bad idea. Please replace this by debug variable query_exec_time.
7. Please add tests to sys_vars suite for the all added variables.
Please fix this, and I will review again.