Code review comment for lp:~sergei.glushchenko/percona-server/5.6-ps-bug1159743

Laurynas,

> - Why are the changes in 79-80 and 86-87 required?

This change was made to make test more stable in Jenkins. I saw
failures when connection was successful when it was supposed to fail
with "too many connections". I think server was too slow to start
executing the "select sleep()" query. This change helped to make the
test stable.

> - Now there are two sets of macros to annotate socket waits:
> MYSQL_(START|END)_SOCKET_WAIT and (START|END)_SOCKET_WAIT,
> where the former annotates for PFS alone, and the latter for
> PFS and threadpool. It probably does not have sense to ever use
> the former option, and if upstream introduces new calls, they
> will merge silently and will appear to work. Maybe it's
> better, if possible, to add threadpool callback calls to
> MYSQL_START_SOCKET_WAIT and do not introduce another set of
> macros?

Yes, it was my first thought, but this approach introduces additional
dependencies for performance schema. But on second thought I agreed
with MariaDB's approach because of following:

1. This is general approach for wait callbacks, thr_lock.c follows
   same convention. Provide function to set callbacks which are set in
   scheduler_init. Callbacks are local to thr_lock.c and there is no
   knowledge of threadpool in thr_lock.c. Same done for
   viosocket.c.
2. Modifying MYSQL_(START|END)_SOCKET_WAIT would mix two different
   things. Notify PFS about socket wait and notify threadpool (or
   other scheduler if it happened to provide callback) about socket
   wait. I think it is wrong to mix them. mysql_cond_timedwait is
   instrumented for PFS, but it doesn't report wait event for
   scheduler. You can think of MYSQL_START_SOCKET_WAIT as about
   instrumented poll, don't really know why mysql_poll was not
   introduced. So (START|END)_SOCKET_WAIT are only to avoid code
   duplication.
3. Modifying MYSQL_(START|END)_SOCKET_WAIT would require some
   knowledge about scheduler callbacks by performance schema which I'd
   prefer to avoid.

So, these are my arguments for current approach.

> - 176-180, 188-190: is the if (timeout != 0) intended for "This
> patch also don't report wait event when timeout is 0. This helps
> to avoid reporting of wait event for THD::is_connected, which
> prevents from reporiting wait event when another wait set
> (specifically THD_WAIT_USER_LOCK)."?
> This already should be handled by "if (timeout &&
> before_io_wait)" in the macro itself, and has the effect of
> disabling the PFS callback, which was not intended?

Indeed, this is fixed by
https://mariadb.atlassian.net/browse/MDEV-4566, which I ported later
and is not needed now. I forgot about that change when I ported follow
up fix. Will fix.