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?

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

  - 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.

  - 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.

  - include my_global.h and use DBUG_ASSERT() instead of assert()? Also
    don't include assert.h, errno.h and string.h?

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

  - 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.

  - 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.

  - innobase_kill_connection() needs comments