Merge lp:~dingqi-lxb/percona-server/5.5_log_queries_in_memory into lp:percona-server/5.5

[林晓斌]

http://jenkins.percona.com/view/Percona%20Server%205.5/job/percona-server-5.5-param/251/

patch for https://blueprints.launchpad.net/percona-server/+spec/log-queries-in-memory.

add a variable 'slowlog_in_memory_audit_max_memory' and global status 'slow_log_current_memory'

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)]

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:
+ if (general_query)
+ {
....
+ }
+ else
+ {
+ this->general_query= "NULL";
+ this->general_query_len= 5;
+ }
You are break SQL semantic. For example query:
SELECT * FROM INSFORMATION_SCHEMA.I_S_SLOW_LOG WHERE QUERY IS NULL
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.

review: Needs Fixing

[Oleg Tsarev (tsarev)]

8.
+ ulonglong utime_of_query= thd->current_utime() - thd->utime_after_lock;
+ if (utime_of_query <= thd->variables.long_query_time)
+ return;

Please look into the slow_extended.patch:

static inline ulonglong get_query_exec_time(THD *thd, ulonglong cur_utime)
...
thd->server_status|= SERVER_QUERY_WAS_SLOW;

Please better use this status variable, instead of calculate query execution time directly.

[林晓斌 (dingqi-lxb)]

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)]

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://code.launchpad.net/~dingqi-lxb/percona-server/5.5_log_queries_in_memory/+merge/85251
> 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)]

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_was_used will not be imported here.

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)]

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_IN_MEMORY_AUDIT soname
> 'slowlog_in_memory.so'"`` and ``"install plugin SLOW_LOG soname
> 'slowlog_in_memory.so'"``, they work by keeping one list in memory.

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)]

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.

[lp:~dingqi-lxb/percona-server/5.5_log_queries_in_memory]

199. By 林晓斌 on 2012-01-01

merge from trunk

200. By 林晓斌 on 2012-01-01

add slowlog_in_memory.patch

Unmerged revisions

200. By 林晓斌 on 2012-01-01

add slowlog_in_memory.patch

199. By 林晓斌 on 2012-01-01

merge from trunk