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

Proposed by Sergei Glushchenko
Status: Merged
Approved by: Laurynas Biveinis
Approved revision: 596
Merged at revision: 612
Proposed branch: lp:~sergei.glushchenko/percona-server/5.6-ps-bug1309026
Merge into: lp:percona-server/5.6
Diff against target: 25 lines (+8/-0)
1 file modified
sql/binlog.cc (+8/-0)
To merge this branch: bzr merge lp:~sergei.glushchenko/percona-server/5.6-ps-bug1309026
Reviewer Review Type Date Requested Status
Laurynas Biveinis (community) Approve
Review via email: mp+220879@code.launchpad.net

Description of the change

    Bug 1309026: server crashing, potentially thread pool related

    Cause is race condition in group commit code which leads to
    concurrent updates in PFS instrumentation while it is not designed
    to be updated concurrently. Workaround is to set thread-local
    value of PSI_thread to NULL for the follower thread while it is
    waiting for leader thread to complete commit.

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

Debug builds failed because of https://bugs.launchpad.net/percona-server/+bug/1323014

To post a comment you must log in.
Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

I wonder about the PFS instrumentation implications of the fix.

Before, PFS tracked the follower thread twice for the same time span: once for the follower thread itself, the waits on m_lock_done and m_cond_done; and at the same time, for the leader thread in the follower context, whatever is done in ha_commit_low.

With the fix, we lose m_lock_done and m_cond_done accounting?

review: Needs Information
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

Yes, we'll loose it. Another option is to make PFS instrumentation thread-safe, but will have impact on performance, I think.

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

I think it's OK to lose the m_lock_done/m_cond_done accounting until the upstream fix.

Please move down PSI_thread *psi_thread declaration to the first #ifdef HAVE_PSI_THREAD_INTERFACE block (now we'd have a new build warning if someone builds without PFS)

review: Needs Fixing
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

Repushed

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 'sql/binlog.cc'
2--- sql/binlog.cc 2014-05-16 05:48:50 +0000
3+++ sql/binlog.cc 2014-06-11 10:22:53 +0000
4@@ -1549,6 +1549,11 @@
5 */
6 if (!leader)
7 {
8+#ifdef HAVE_PSI_THREAD_INTERFACE
9+ PSI_thread *psi_thread;
10+ psi_thread= PSI_THREAD_CALL(get_thread)();
11+ PSI_THREAD_CALL(set_thread)(NULL);
12+#endif
13 mysql_mutex_lock(&m_lock_done);
14 #ifndef DBUG_OFF
15 /*
16@@ -1564,6 +1569,9 @@
17 mysql_cond_wait(&m_cond_done, &m_lock_done);
18 }
19 mysql_mutex_unlock(&m_lock_done);
20+#ifdef HAVE_PSI_THREAD_INTERFACE
21+ PSI_THREAD_CALL(set_thread)(psi_thread);
22+#endif
23 }
24 return leader;
25 }

Subscribers

People subscribed via source and target branches