Percona Server moved to https://jira.percona.com/projects/PS

Merge lp:~sergei.glushchenko/percona-server/5.6-ps-bug1159743 into lp:percona-server/5.6

  1. 5.6-ps-bug1159743
  2. Merge into 5.6
Proposed by Sergei Glushchenko
Status: Merged
Approved by: Laurynas Biveinis
Approved revision: no longer in the source branch.
Merged at revision: 502
Proposed branch: lp:~sergei.glushchenko/percona-server/5.6-ps-bug1159743
Merge into: lp:percona-server/5.6
Diff against target: 208 lines (+64/-12)
8 files modified
Percona-Server/include/mysql/plugin_audit.h.pp (+2/-1)
Percona-Server/include/mysql/plugin_auth.h.pp (+2/-1)
Percona-Server/include/mysql/plugin_ftparser.h.pp (+2/-1)
Percona-Server/include/mysql/service_thd_wait.h (+2/-1)
Percona-Server/include/violite.h (+3/-1)
Percona-Server/mysql-test/t/pool_of_threads.test (+2/-2)
Percona-Server/sql/scheduler.cc (+13/-0)
Percona-Server/vio/viosocket.c (+38/-5)
To merge this branch: bzr merge lp:~sergei.glushchenko/percona-server/5.6-ps-bug1159743
Reviewer Review Type Date Requested Status
Laurynas Biveinis (community) Approve
Vlad Lesin Pending
Review via email: mp+196762@code.launchpad.net

This proposal supersedes a proposal from 2013-07-16.

Description of the change

I've addressed Laurynas's comments
http://jenkins.percona.com/job/percona-server-5.6-param/418/

To post a comment you must log in.
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote : Posted in a previous version of this proposal

https://mariadb.atlassian.net/browse/MDEV-4566
probably should also be ported

Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote : Posted in a previous version of this proposal

http://lists.askmonty.org/pipermail/commits/2013-May/004743.html

Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote : Posted in a previous version of this proposal

Both MDEV-156 and MDEV-4566 are ported.

Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote : Posted in a previous version of this proposal

http://jenkins.percona.com/view/PS%205.6/job/percona-server-5.6-param/218/

Revision history for this message
Vlad Lesin (vlad-lesin) wrote : Posted in a previous version of this proposal

Here is fresh testing http://jenkins.percona.com/view/PS%205.6/job/percona-server-5.6-param/402/ .

review: Approve
Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote : Posted in a previous version of this proposal

Sergei -

    - Why are the changes in 79-80 and 86-87 required?
    - 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?
    - 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?

review: Needs Fixing
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote : Posted in a previous version of this proposal

Sergei -

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

OK, so the testcase is still potentially unstable, depending on the
host load. A DEBUG_SYNC would be a stable replacement, but then it's
a trade-off between a potentially unstable testcase that tests release
builds only and a stable debug-only testcase. IMHO let's proceed with
sleeps and wait for any actual instabilities.

> > - 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:
...
> So, these are my arguments for current approach.

OK. Let's add a note to upstream merge checklist when this is merged.

To sum up, the only change needed is 176-180, 188-190 from the 1st
review.

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Percona-Server/include/mysql/plugin_audit.h.pp'
2--- Percona-Server/include/mysql/plugin_audit.h.pp 2013-10-15 11:37:57 +0000
3+++ Percona-Server/include/mysql/plugin_audit.h.pp 2013-11-26 18:06:32 +0000
4@@ -44,7 +44,8 @@
5 THD_WAIT_BINLOG= 8,
6 THD_WAIT_GROUP_COMMIT= 9,
7 THD_WAIT_SYNC= 10,
8- THD_WAIT_LAST= 11
9+ THD_WAIT_NET= 11,
10+ THD_WAIT_LAST= 12
11 } thd_wait_type;
12 extern struct thd_wait_service_st {
13 void (*thd_wait_begin_func)(void*, int);
14
15=== modified file 'Percona-Server/include/mysql/plugin_auth.h.pp'
16--- Percona-Server/include/mysql/plugin_auth.h.pp 2013-09-09 15:24:45 +0000
17+++ Percona-Server/include/mysql/plugin_auth.h.pp 2013-11-26 18:06:32 +0000
18@@ -44,7 +44,8 @@
19 THD_WAIT_BINLOG= 8,
20 THD_WAIT_GROUP_COMMIT= 9,
21 THD_WAIT_SYNC= 10,
22- THD_WAIT_LAST= 11
23+ THD_WAIT_NET= 11,
24+ THD_WAIT_LAST= 12
25 } thd_wait_type;
26 extern struct thd_wait_service_st {
27 void (*thd_wait_begin_func)(void*, int);
28
29=== modified file 'Percona-Server/include/mysql/plugin_ftparser.h.pp'
30--- Percona-Server/include/mysql/plugin_ftparser.h.pp 2013-09-09 15:24:45 +0000
31+++ Percona-Server/include/mysql/plugin_ftparser.h.pp 2013-11-26 18:06:32 +0000
32@@ -44,7 +44,8 @@
33 THD_WAIT_BINLOG= 8,
34 THD_WAIT_GROUP_COMMIT= 9,
35 THD_WAIT_SYNC= 10,
36- THD_WAIT_LAST= 11
37+ THD_WAIT_NET= 11,
38+ THD_WAIT_LAST= 12
39 } thd_wait_type;
40 extern struct thd_wait_service_st {
41 void (*thd_wait_begin_func)(void*, int);
42
43=== modified file 'Percona-Server/include/mysql/service_thd_wait.h'
44--- Percona-Server/include/mysql/service_thd_wait.h 2011-06-30 15:46:53 +0000
45+++ Percona-Server/include/mysql/service_thd_wait.h 2013-11-26 18:06:32 +0000
46@@ -74,7 +74,8 @@
47 THD_WAIT_BINLOG= 8,
48 THD_WAIT_GROUP_COMMIT= 9,
49 THD_WAIT_SYNC= 10,
50- THD_WAIT_LAST= 11
51+ THD_WAIT_NET= 11,
52+ THD_WAIT_LAST= 12
53 } thd_wait_type;
54
55 extern struct thd_wait_service_st {
56
57=== modified file 'Percona-Server/include/violite.h'
58--- Percona-Server/include/violite.h 2013-09-29 16:08:42 +0000
59+++ Percona-Server/include/violite.h 2013-11-26 18:06:32 +0000
60@@ -104,7 +104,9 @@
61 ssize_t vio_pending(Vio *vio);
62 #endif
63 /* Set timeout for a network operation. */
64-int vio_timeout(Vio *vio, uint which, int timeout_sec);
65+extern int vio_timeout(Vio *vio, uint which, int timeout_sec);
66+extern void vio_set_wait_callback(void (*before_wait)(void),
67+ void (*after_wait)(void));
68 /* Connect to a peer. */
69 my_bool vio_socket_connect(Vio *vio, struct sockaddr *addr, socklen_t len,
70 int timeout);
71
72=== modified file 'Percona-Server/mysql-test/t/pool_of_threads.test'
73--- Percona-Server/mysql-test/t/pool_of_threads.test 2013-01-30 09:55:26 +0000
74+++ Percona-Server/mysql-test/t/pool_of_threads.test 2013-11-26 18:06:32 +0000
75@@ -13,13 +13,13 @@
76 connection default;
77 let $default_id = `select connection_id()`;
78 send SELECT sleep(50000);
79---sleep 1
80+--sleep 2.5
81
82 connect(con2,localhost,root,,);
83 connection con2;
84 let $con2_id = `select connection_id()`;
85 send SELECT sleep(50000);
86---sleep 0.5
87+--sleep 2.5
88
89 --disable_abort_on_error
90 --disable_result_log
91
92=== modified file 'Percona-Server/sql/scheduler.cc'
93--- Percona-Server/sql/scheduler.cc 2013-08-14 03:57:21 +0000
94+++ Percona-Server/sql/scheduler.cc 2013-11-26 18:06:32 +0000
95@@ -25,6 +25,7 @@
96 #include "sql_callback.h"
97 #include "global_threads.h"
98 #include "mysql/thread_pool_priv.h"
99+#include <violite.h>
100
101 /*
102 End connection, in case when we are using 'no-threads'
103@@ -71,6 +72,15 @@
104 static void scheduler_wait_sync_end(void) {
105 thd_wait_end(NULL);
106 }
107+
108+static void scheduler_wait_net_begin(void) {
109+ thd_wait_begin(NULL, THD_WAIT_NET);
110+}
111+
112+static void scheduler_wait_net_end(void) {
113+ thd_wait_end(NULL);
114+}
115+
116 };
117 /**@}*/
118
119@@ -86,6 +96,9 @@
120 scheduler_wait_lock_end);
121 thr_set_sync_wait_callback(scheduler_wait_sync_begin,
122 scheduler_wait_sync_end);
123+
124+ vio_set_wait_callback(scheduler_wait_net_begin,
125+ scheduler_wait_net_end);
126 }
127
128 /*
129
130=== modified file 'Percona-Server/vio/viosocket.c'
131--- Percona-Server/vio/viosocket.c 2013-09-29 16:08:42 +0000
132+++ Percona-Server/vio/viosocket.c 2013-11-26 18:06:32 +0000
133@@ -29,6 +29,38 @@
134 # include <sys/filio.h>
135 #endif
136
137+/* Network io wait callbacks for threadpool */
138+static void (*before_io_wait)(void)= 0;
139+static void (*after_io_wait)(void)= 0;
140+
141+/* Wait callback macros (both performance schema and threadpool */
142+#define START_SOCKET_WAIT(locker, state_ptr, sock, which, timeout) \
143+do \
144+{ \
145+ MYSQL_START_SOCKET_WAIT(locker, state_ptr, sock, \
146+ which, 0); \
147+ if (timeout && before_io_wait) \
148+ before_io_wait(); \
149+} while(0)
150+
151+
152+#define END_SOCKET_WAIT(locker, timeout) \
153+do \
154+{ \
155+ MYSQL_END_SOCKET_WAIT(locker, 0); \
156+ if (timeout && after_io_wait) \
157+ after_io_wait(); \
158+} while(0)
159+
160+
161+
162+void vio_set_wait_callback(void (*before_wait)(void),
163+ void (*after_wait)(void))
164+{
165+ before_io_wait= before_wait;
166+ after_io_wait= after_wait;
167+}
168+
169 int vio_errno(Vio *vio __attribute__((unused)))
170 {
171 /* These transport types are not Winsock based. */
172@@ -826,8 +858,8 @@
173 break;
174 }
175
176- MYSQL_START_SOCKET_WAIT(locker, &state, vio->mysql_socket, PSI_SOCKET_SELECT, 0);
177-
178+ START_SOCKET_WAIT(locker, &state, vio->mysql_socket,
179+ PSI_SOCKET_SELECT, timeout);
180 /*
181 Wait for the I/O event and return early in case of
182 error or timeout.
183@@ -850,7 +882,7 @@
184 break;
185 }
186
187- MYSQL_END_SOCKET_WAIT(locker, 0);
188+ END_SOCKET_WAIT(locker, timeout);
189 DBUG_RETURN(ret);
190 }
191
192@@ -892,13 +924,14 @@
193 break;
194 }
195
196- MYSQL_START_SOCKET_WAIT(locker, &state, vio->mysql_socket, PSI_SOCKET_SELECT, 0);
197+ START_SOCKET_WAIT(locker, &state, vio->mysql_socket,
198+ PSI_SOCKET_SELECT, timeout);
199
200 /* The first argument is ignored on Windows. */
201 ret= select(fd + 1, &readfds, &writefds, &exceptfds,
202 (timeout >= 0) ? &tm : NULL);
203
204- MYSQL_END_SOCKET_WAIT(locker, 0);
205+ END_SOCKET_WAIT(locker, timeout);
206
207 /* Set error code to indicate a timeout error. */
208 if (ret == 0)

Subscribers

People subscribed via source and target branches