List:Commits« Previous MessageNext Message »
From:Marc Alff Date:May 13 2011 1:44pm
Subject:bzr commit into mysql-trunk branch (marc.alff:3083) Bug#11766111
View as plain text  
#At file:///Users/malff/BZR_TREE/mysql-trunk-bug11766111/ based on revid:bjorn.munch@stripped

 3083 Marc Alff	2011-05-13
      Bug#11766111 59150: PERFSCHEMA.SETUP_OBJECTS FAILS SPORADICALLY IN RELEASE BUILD
      
      Before this fix, some table that, according to performance_schema.setup_objects,
      should be instrumented, were in fact not instrumented by the performance schema.
      
      Changing ENABLED from 'YES' to 'NO' was working properly,
      but changing ENABLED from 'NO' to 'YES' failed to instrument tables in some cases.
      
      The root cause (found by analysis) is that get_table_share_v1() was returning NULL
      and giving up any instrumentation, making it impossible to instrument a table later.
      
      The fix is to always provide an instrumented PSI_table_share.
      
      This behavior is actually the original design intent, see comments in
      find_or_create_table_share().
      
      This fix:
      
      1)
      Moves the checks for the ENABLED and TIMED flags from get_table_share_v1() to open_table_v1()
      
      2)
      Implements a cache in PFS_table_share to maintain:
        PFS_table_share::m_io_enabled
      as a flag derived from:
        table performance_schema.setup_objects, column ENABLED
        table setup_instrument, row wait/io/table/sql/handler, ENABLED column
      and likewise for similar flags.
      
      3)
      Simplifies the implementation of table setup_instruments,
      and added code to maintain the PFS_table_share cached values.
      
      4)
      Refresh the cache at every table open only,
      not at every table io or table lock operation, for performances.
      
      Part 1 fixes the bug found, while parts 2-4 are related performance optimizations.

    modified:
      storage/perfschema/pfs.cc
      storage/perfschema/pfs_instr_class.cc
      storage/perfschema/pfs_instr_class.h
      storage/perfschema/pfs_setup_object.h
      storage/perfschema/table_setup_instruments.cc
      storage/perfschema/table_setup_instruments.h
=== modified file 'storage/perfschema/pfs.cc'
--- a/storage/perfschema/pfs.cc	2011-02-14 14:23:55 +0000
+++ b/storage/perfschema/pfs.cc	2011-05-13 13:44:31 +0000
@@ -1402,9 +1402,6 @@ static void destroy_cond_v1(PSI_cond* co
 static PSI_table_share*
 get_table_share_v1(my_bool temporary, TABLE_SHARE *share)
 {
-  /* Do not instrument this table is all table instruments are disabled. */
-  if (! global_table_io_class.m_enabled && ! global_table_lock_class.m_enabled)
-    return NULL;
   /* An instrumented thread is required, for LF_PINS. */
   PFS_thread *pfs_thread= my_pthread_getspecific_ptr(PFS_thread*, THR_PFS);
   if (unlikely(pfs_thread == NULL))
@@ -1454,6 +1451,16 @@ open_table_v1(PSI_table_share *share, co
   PFS_thread *thread= my_pthread_getspecific_ptr(PFS_thread*, THR_PFS);
   if (unlikely(thread == NULL))
     return NULL;
+
+  if (unlikely(setup_objects_version != pfs_table_share->m_setup_objects_version))
+  {
+    pfs_table_share->refresh_setup_object_flags(thread);
+  }
+
+  /* Do not instrument this table is all table instruments are disabled. */
+  if (! pfs_table_share->m_io_enabled && ! pfs_table_share->m_lock_enabled)
+    return NULL;
+
   PFS_table *pfs_table= create_table(pfs_table_share, thread, identity);
   return reinterpret_cast<PSI_table *> (pfs_table);
 }
@@ -2212,27 +2219,8 @@ get_thread_table_io_locker_v1(PSI_table_
   if (! flag_global_instrumentation)
     return NULL;
 
-  if (! global_table_io_class.m_enabled)
-    return NULL;
-
   PFS_table_share *share= pfs_table->m_share;
-  if (unlikely(setup_objects_version != share->m_setup_objects_version))
-  {
-    PFS_thread *pfs_thread= my_pthread_getspecific_ptr(PFS_thread*, THR_PFS);
-    if (unlikely(pfs_thread == NULL))
-      return NULL;
-    /* Refresh the enabled and timed flags from SETUP_OBJECTS */
-    share->m_setup_objects_version= setup_objects_version;
-    lookup_setup_object(pfs_thread,
-                        OBJECT_TYPE_TABLE, /* even for temporary tables */
-                        share->m_schema_name,
-                        share->m_schema_name_length,
-                        share->m_table_name,
-                        share->m_table_name_length,
-                        & share->m_enabled,
-                        & share->m_timed);
-  }
-  if (! share->m_enabled)
+  if (! share->m_io_enabled)
     return NULL;
 
   PFS_instr_class *klass;
@@ -2250,7 +2238,7 @@ get_thread_table_io_locker_v1(PSI_table_
     state->m_thread= reinterpret_cast<PSI_thread *> (pfs_thread);
     flags= STATE_FLAG_THREAD;
 
-    if (klass->m_timed && share->m_timed)
+    if (share->m_io_timed)
       flags|= STATE_FLAG_TIMED;
 
     if (flag_events_waits_current)
@@ -2284,7 +2272,7 @@ get_thread_table_io_locker_v1(PSI_table_
   }
   else
   {
-    if (klass->m_timed && share->m_timed)
+    if (share->m_io_timed)
     {
       flags= STATE_FLAG_TIMED;
     }
@@ -2320,27 +2308,8 @@ get_thread_table_lock_locker_v1(PSI_tabl
   if (! flag_global_instrumentation)
     return NULL;
 
-  if (! global_table_lock_class.m_enabled)
-    return NULL;
-
   PFS_table_share *share= pfs_table->m_share;
-  if (unlikely(setup_objects_version != share->m_setup_objects_version))
-  {
-    PFS_thread *pfs_thread= my_pthread_getspecific_ptr(PFS_thread*, THR_PFS);
-    if (unlikely(pfs_thread == NULL))
-      return NULL;
-    /* Refresh the enabled and timed flags from SETUP_OBJECTS */
-    share->m_setup_objects_version= setup_objects_version;
-    lookup_setup_object(pfs_thread,
-                        OBJECT_TYPE_TABLE, /* even for temporary tables */
-                        share->m_schema_name,
-                        share->m_schema_name_length,
-                        share->m_table_name,
-                        share->m_table_name_length,
-                        & share->m_enabled,
-                        & share->m_timed);
-  }
-  if (! share->m_enabled)
+  if (! share->m_lock_enabled)
     return NULL;
 
   PFS_instr_class *klass;
@@ -2380,7 +2349,7 @@ get_thread_table_lock_locker_v1(PSI_tabl
     state->m_thread= reinterpret_cast<PSI_thread *> (pfs_thread);
     flags= STATE_FLAG_THREAD;
 
-    if (klass->m_timed && share->m_timed)
+    if (share->m_lock_timed)
       flags|= STATE_FLAG_TIMED;
 
     if (flag_events_waits_current)
@@ -2414,7 +2383,7 @@ get_thread_table_lock_locker_v1(PSI_tabl
   }
   else
   {
-    if (klass->m_timed && share->m_timed)
+    if (share->m_lock_timed)
     {
       flags= STATE_FLAG_TIMED;
     }

=== modified file 'storage/perfschema/pfs_instr_class.cc'
--- a/storage/perfschema/pfs_instr_class.cc	2011-02-14 14:23:55 +0000
+++ b/storage/perfschema/pfs_instr_class.cc	2011-05-13 13:44:31 +0000
@@ -118,6 +118,36 @@ PFS_table_share *table_share_array= NULL
 PFS_instr_class global_table_io_class;
 PFS_instr_class global_table_lock_class;
 
+void PFS_instr_class::set_enabled(PFS_instr_class *pfs, bool enabled)
+{
+  pfs->m_enabled= enabled;
+
+  /*
+    When the table instruments are changed,
+    the cache on top of SETUP_OBJECTS is invalidated.
+  */
+  if ((pfs == & global_table_io_class) ||
+      (pfs == & global_table_lock_class))
+  {
+    setup_objects_version++;
+  }
+}
+
+void PFS_instr_class::set_timed(PFS_instr_class *pfs, bool timed)
+{
+  pfs->m_timed= timed;
+
+  /*
+    When the table instruments are changed,
+    the cache on top of SETUP_OBJECTS is invalidated.
+  */
+  if ((pfs == & global_table_io_class) ||
+      (pfs == & global_table_lock_class))
+  {
+    setup_objects_version++;
+  }
+}
+
 /**
   Hash index for instrumented table shares.
   This index is searched by table fully qualified name (@c PFS_table_share_key),
@@ -409,6 +439,24 @@ static void set_table_share_key(PFS_tabl
   }
 }
 
+void PFS_table_share::refresh_setup_object_flags(PFS_thread *thread)
+{
+  bool enabled;
+  bool timed;
+  m_setup_objects_version= setup_objects_version;
+  lookup_setup_object(thread,
+                      OBJECT_TYPE_TABLE,
+                      m_schema_name, m_schema_name_length,
+                      m_table_name, m_table_name_length,
+                      &enabled, &timed);
+
+  m_io_enabled= enabled && global_table_io_class.m_enabled;
+  m_io_timed= timed && global_table_io_class.m_timed;
+
+  m_lock_enabled= enabled && global_table_lock_class.m_enabled;
+  m_lock_timed= timed && global_table_lock_class.m_timed;
+}
+
 /**
   Initialize the file class buffer.
   @param file_class_sizing            max number of file class
@@ -1041,8 +1089,10 @@ PFS_table_share* find_or_create_table_sh
   uint retry_count= 0;
   uint version= 0;
   const uint retry_max= 3;
-  bool enabled= true;
-  bool timed= true;
+  bool io_enabled= true;
+  bool lock_enabled= true;
+  bool io_timed= true;
+  bool lock_timed= true;
 
 search:
   entry= reinterpret_cast<PFS_table_share**>
@@ -1065,6 +1115,8 @@ search:
 
   if (retry_count == 0)
   {
+    bool enabled;
+    bool timed;
     version= setup_objects_version;
     lookup_setup_object(thread,
                         OBJECT_TYPE_TABLE,
@@ -1072,6 +1124,12 @@ search:
                         table_name, table_name_length,
                         &enabled, &timed);
 
+    io_enabled= enabled && global_table_io_class.m_enabled;
+    io_timed= timed && global_table_io_class.m_timed;
+
+    lock_enabled= enabled && global_table_lock_class.m_enabled;
+    lock_timed= timed && global_table_lock_class.m_timed;
+
     /*
       Even when enabled is false, a record is added in the dictionary:
       - It makes enabling a table already in the table cache possible,
@@ -1101,8 +1159,10 @@ search:
           pfs->m_schema_name_length= schema_name_length;
           pfs->m_table_name= &pfs->m_key.m_hash_key[schema_name_length + 2];
           pfs->m_table_name_length= table_name_length;
-          pfs->m_enabled= enabled;
-          pfs->m_timed= timed;
+          pfs->m_io_enabled= io_enabled;
+          pfs->m_lock_enabled= lock_enabled;
+          pfs->m_io_timed= io_timed;
+          pfs->m_lock_timed= lock_timed;
           pfs->init_refcount();
           pfs->m_table_stat.reset();
           set_keys(pfs, share);

=== modified file 'storage/perfschema/pfs_instr_class.h'
--- a/storage/perfschema/pfs_instr_class.h	2011-02-14 14:23:55 +0000
+++ b/storage/perfschema/pfs_instr_class.h	2011-05-13 13:44:31 +0000
@@ -99,6 +99,9 @@ struct PFS_instr_class
   {
     return m_flags & PSI_FLAG_GLOBAL;
   }
+
+  static void set_enabled(PFS_instr_class *pfs, bool enabled);
+  static void set_timed(PFS_instr_class *pfs, bool timed);
 };
 
 struct PFS_mutex;
@@ -231,7 +234,17 @@ public:
     PFS_atomic::add_32(& m_refcount, -1);
   }
 
-  /** Setup object refresh version. */
+  void refresh_setup_object_flags(PFS_thread *thread);
+
+  /**
+    Setup object refresh version.
+    Cache version used when computing the enabled / timed flags.
+    @sa setup_objects_version
+    @sa m_io_enabled
+    @sa m_lock_enabled
+    @sa m_io_timed
+    @sa m_lock_timed
+  */
   uint m_setup_objects_version;
   /** Internal lock. */
   pfs_lock m_lock;
@@ -245,11 +258,14 @@ public:
   const char *m_table_name;
   /** Length in bytes of @c m_table_name. */
   uint m_table_name_length;
-  /** True if this table instrument is enabled. */
-  bool m_enabled;
-  /** True if this table instrument is timed. */
-  bool m_timed;
-  bool m_purge;
+  /** True if table io instrumentation is enabled. */
+  bool m_io_enabled;
+  /** True if table lock instrumentation is enabled. */
+  bool m_lock_enabled;
+  /** True if table io instrumentation is timed. */
+  bool m_io_timed;
+  /** True if table lock instrumentation is timed. */
+  bool m_lock_timed;
   /** Table statistics. */
   PFS_table_stat m_table_stat;
   /** Number of indexes. */

=== modified file 'storage/perfschema/pfs_setup_object.h'
--- a/storage/perfschema/pfs_setup_object.h	2011-02-04 11:55:17 +0000
+++ b/storage/perfschema/pfs_setup_object.h	2011-05-13 13:44:31 +0000
@@ -70,6 +70,20 @@ struct PFS_setup_object
   bool m_timed;
 };
 
+/**
+  Version number of the SETUP_OBJECTS cache.
+  The content of the SETUP_OBJECTS table,
+  and the content of SETUP_INSTRUMENTS table for instruments that apply to objects :
+  - wait/io/table/sql/handler
+  - wait/lock/table/sql/handler
+  is cached once for each object,
+  to avoid evaluating the object ENABLED and TIMED flags too frequently.
+  Incrementing @c setup_objects_version invalidates the cache.
+  @sa global_table_io_class
+  @sa global_table_lock_class
+  @sa PFS_table_share::refresh_setup_objects_flags
+  @sa PFS_table_share::m_setup_objects_version
+*/
 extern uint setup_objects_version;
 
 int init_setup_object(const PFS_global_param *param);

=== modified file 'storage/perfschema/table_setup_instruments.cc'
--- a/storage/perfschema/table_setup_instruments.cc	2011-02-14 14:23:55 +0000
+++ b/storage/perfschema/table_setup_instruments.cc	2011-05-13 13:44:31 +0000
@@ -1,4 +1,4 @@
-/* Copyright (c) 2008, 2010, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2008, 2011, Oracle and/or its affiliates. All rights reserved.
 
   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
@@ -25,6 +25,7 @@
 #include "pfs_column_values.h"
 #include "table_setup_instruments.h"
 #include "pfs_global.h"
+#include "pfs_setup_object.h"
 
 THR_LOCK table_setup_instruments::m_table_lock;
 
@@ -173,10 +174,7 @@ int table_setup_instruments::rnd_pos(con
 
 void table_setup_instruments::make_row(PFS_instr_class *klass)
 {
-  m_row.m_name= &klass->m_name[0];
-  m_row.m_name_length= klass->m_name_length;
-  m_row.m_enabled_ptr= &klass->m_enabled;
-  m_row.m_timed_ptr= &klass->m_timed;
+  m_row.m_instr_class= klass;
 }
 
 int table_setup_instruments::read_row_values(TABLE *table,
@@ -200,16 +198,13 @@ int table_setup_instruments::read_row_va
       switch(f->field_index)
       {
       case 0: /* NAME */
-        set_field_varchar_utf8(f, m_row.m_name, m_row.m_name_length);
+        set_field_varchar_utf8(f, m_row.m_instr_class->m_name, m_row.m_instr_class->m_name_length);
         break;
       case 1: /* ENABLED */
-        set_field_enum(f, (*m_row.m_enabled_ptr) ? ENUM_YES : ENUM_NO);
+        set_field_enum(f, m_row.m_instr_class->m_enabled ? ENUM_YES : ENUM_NO);
         break;
       case 2: /* TIMED */
-        if (m_row.m_timed_ptr)
-          set_field_enum(f, (*m_row.m_timed_ptr) ? ENUM_YES : ENUM_NO);
-        else
-          set_field_enum(f, ENUM_NO);
+        set_field_enum(f, m_row.m_instr_class->m_timed ? ENUM_YES : ENUM_NO);
         break;
       default:
         DBUG_ASSERT(false);
@@ -238,14 +233,11 @@ int table_setup_instruments::update_row_
         return HA_ERR_WRONG_COMMAND;
       case 1: /* ENABLED */
         value= (enum_yes_no) get_field_enum(f);
-        *m_row.m_enabled_ptr= (value == ENUM_YES) ? true : false;
+        PFS_instr_class::set_enabled(m_row.m_instr_class, (value == ENUM_YES) ? true : false);
         break;
       case 2: /* TIMED */
-        if (m_row.m_timed_ptr)
-        {
-          value= (enum_yes_no) get_field_enum(f);
-          *m_row.m_timed_ptr= (value == ENUM_YES) ? true : false;
-        }
+        value= (enum_yes_no) get_field_enum(f);
+        PFS_instr_class::set_timed(m_row.m_instr_class, (value == ENUM_YES) ? true : false);
         break;
       default:
         DBUG_ASSERT(false);

=== modified file 'storage/perfschema/table_setup_instruments.h'
--- a/storage/perfschema/table_setup_instruments.h	2011-02-14 14:23:55 +0000
+++ b/storage/perfschema/table_setup_instruments.h	2011-05-13 13:44:31 +0000
@@ -32,14 +32,8 @@
 /** A row of PERFORMANCE_SCHEMA.SETUP_INSTRUMENTS. */
 struct row_setup_instruments
 {
-  /** Column NAME. */
-  const char *m_name;
-  /** Length in bytes of @c m_name. */
-  uint m_name_length;
-  /** Column ENABLED. */
-  bool *m_enabled_ptr;
-  /** Column TIMED. */
-  bool *m_timed_ptr;
+  /** Columns NAME, ENABLED, TIMED. */
+  PFS_instr_class *m_instr_class;
 };
 
 /** Position of a cursor on PERFORMANCE_SCHEMA.SETUP_INSTRUMENTS. */


Attachment: [text/bzr-bundle] bzr/marc.alff@oracle.com-20110513134431-v3y2t88tzgsrgn0g.bundle
Thread
bzr commit into mysql-trunk branch (marc.alff:3083) Bug#11766111Marc Alff13 May