Code review comment for lp:~dingqi-lxb/percona-server/5.5_log_queries_in_memory

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://bazaar.launchpad.net/~tsarev/percona-server/5.5_log_queries_in_memory/revision/202
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.patch. Right now their check a slow query log by "log_grep.inc". You should add similar checks of plugin to every test.

About the implementation:

6) Incorrect works with query exec time:
 ulonglong utime_of_query= thd->current_utime() - thd->utime_after_lock;
 * Possible scenario: query doesn't pass to slow query log, but pass to INFORMATION_SCHEMA.SLOW_LOG memory. Or opposite.
 * 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.patch. thd->server_status variable (SERVER_QUERY_WAS_SLOW flag) should setup status of every query (and also should be used both in PS and plugin)

7) Separatelly about this:

+ ulonglong utime_of_query= thd->current_utime() - thd->utime_after_lock;
+
+#ifndef DBUG_OFF
+ if (thd->variables.query_exec_time != 0)
+ utime_of_query= thd->lex->sql_command != SQLCOM_SET_OPTION ? thd->variables.query_exec_time : 0;
+#endif
+
+ if (utime_of_query <= thd->variables.long_query_time)
+ 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-to-interface .

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

review: Needs Fixing

« Back to merge proposal