Code review comment for lp:~vlad-lesin/percona-server/5.6-sql_timeout_twitter

> Vlad,
>
> - do we really need the following change?
>
> -#if !defined(__cplusplus) && !defined(bool)
> -#define bool In_C_you_should_use_my_bool_instead()
> -#endif
> +#include <stdbool.h>
>
> The intention was to allow using C99 'bool' in C code (and break
> Windows compatibility), but I don't see any occurrences of such
> usage?
>
Removed.

> - my_os_timer_* to avoid name clash with existing my_timer_init()
>
Done.

> - I'm having second thoughts on per-user MAX_STATEMENT_TIME limit
> after reviewing the code. It feels like it's going to be a pain to
> support and maintain. Let's remove it? It's a "nice to have" thing, but
> it was not a requirement for us originally, and would also
> unproportionally complicate migrations to/from upstream MySQL.
>
Removed.

> - was there any specific reason to implement your own set of
> regression tests instead of porting them from the Twitter tree? Most
> of their tests use the SELECT hint that we chose not to implement,
> but those could be converted to SET ...; SELECT ... form.
All tests are ported from Twitter tree except these two:
mysql-test/suite/randgen/t/max_statement_time.test
mysql-test/t/max_statement_time.test

The first one is not ported because I did not find randgen grammar files for the test
in the tree. The second one repeats the mysql-test/t/max_statement_time_func.test test
but uses statement timeout set instead of set with variable.

>
> - include my_global.h and use DBUG_ASSERT() instead of assert()? Also
> don't include assert.h, errno.h and string.h?
As we do not support BSD-based platforms and do not test code for such platforms with our
regression tests system the BSD-based platform specific files are kept as is.

> - unnecessary change:
>
> @@ -4254,6 +4284,7 @@ static int replace_column_table(GRANT_TA
> goto end;
> }
> }
> +
> }
>
> /*
>
> - remnant from SELECT hints implementation that is also unnecessary:
>
> --- Percona-Server/sql/sql_lex.h 2013-06-27 15:15:35 +0000
> +++ Percona-Server/sql/sql_lex.h 2013-08-06 11:40:36 +0000
> @@ -2533,6 +2533,9 @@ struct LEX: public Query_tables_list
>
> class Explain_format *explain_format;
>
> + /** Maximum execution time for a statement. */
> + ulong max_statement_time;
> +
> LEX();
>
> virtual ~LEX()
>
As per-user MAX_STATEMENT_TIME limit code is removed the above changes are removed too.

> - I would restructure get_max_statement_time() / set_statement_timer()
> a bit for performance reasons. We should first check if we do have
> some timeout defined (i.e. thd->variables.max_statement_time > 0)
> and then check other conditions. So that we do as less work as
> possible when timeouts are not used. likely()/unlikely() and
> explicit "inline" hints can also make a big difference.
>
Done.

> - can you explain changed in ha_innodb.cc? It looks like fixing a
> problem in the wrong place, i.e. implementing a workaround of
> thread_pool_priv.h defining MYSQL_SERVER unconditionally.
Made conditional #define in thread_pool_priv.h

> - innobase_kill_connection() needs comments
Done.

http://jenkins.percona.com/view/PS 5.6/job/percona-server-5.6-param/269