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

Vlad,

  - generally I would use check_killed() instead of get_killed(). yes,
    that would be a bit inconsistent naming with set_killed(), but that
    would also make porting APC easier, should we need to do so.
  - mysqld--help-win.result also needs an adjustment
  - setting sql_timeout_expire in mysql_parse() prevents the feature
    from working with client API prepared statements. this check should
    be done in dispatch_command()
  - doing it in dispatch_command() will also make statement-based
    replication immune to sql_timeout. which is good.
  - many lines use tabs for indentation. you can get them with the
    following command:

  bzr diff -c-1 | grep "+\t"

  - changes in merge_buffers() in filesort.cc break the logic it used
    previously, i.e. check the killed state on each iteration in the
    "while" loop, unless param->not_killable is set.

    What we do now is: check the killed state before executing the loop,
    reset it if param->not_killable is set, then check the same value
    inside the loop. See how that code has been changed in MariaDB to
    support APC.

  - same problem as above in storage/myisam/sort.c

  - in Interruptible_wait::wait() we check for the killed status, but
    only after waiting for the full timeout value, which means up to 5
    seconds. Why not do a similar check as with the m_abs_timeout? In
    this case you don't even need a call to m_thd->get_killed() as it
    will be checked by the caller.

  - another problem from the above code is that checking get_killed()
    and resetting the error in Interruptible_wait::wait() returns a
    normal status to the user. I.e. SELECT SLEEP(N) should fail "ERROR
    1317 (70100): Query execution was interrupted" when the timeout is
    exceeded, but it doesn't. Same with user-level locks.

  - in get_killed(), I'd rewrite it like this to not rely on how smart
    is the compiler:

    if (!sql_timeout_expire)
      return killed_status;
    return (my_micro_time() >= sql_timeout_expire) ? THD::KILL_QUERY :
                                                     killed_status;

  - I wonder if we really need my_micro_time() for this. my_time() has
    1 second resolution (which is sufficient in this case) and is also
    faster.

  - in MyISAM code, I would use thd_killed() and keep the killed_ptr()
    name in the same way it is implemented in MariaDB.