Merge lp:~stewart/drizzle/bug492391-i_s-concurrency into lp:~drizzle-trunk/drizzle/development

[Stewart Smith]

[Stewart Smith (stewart)]

Not the most ideal fix, but a minimal one that does actually make the code work.

[Jay Pipes (jaypipes)]

Hi!

Some little things, one bigger thing. :)

The bigger thing, and I'm sure you've already thought about this, is the fact that for each call to an I_S table, the underlying table/view rows are copied into the InformationCursor::rows memory. This is clearly a suboptimal strategy, but I'm going to assume you've already discussed this with Brian?

Tiny style fixups:

77 + while(copy_iter != sch_table->getRows().end())

One space after while, please :)

There is some space/indent weirdness going on in the constructors (see below on diff). Indents should be 2 spaces (there is one area they are 4 spaces below.)

Unless there's a good reason not to do so, the general trend I've seen in the code is to have constructors and other method definitions be in the .cc files and not the .h files where they are implicitly inlined. You have the setup of the mutex attr and the mutex itself duplicated in both constructors and they are in the .h file. I'd prefer to see the mutex and mutex attr initialization put into an init() private method and see the constructors defined in the .cc file.

Also, the following lines should be reversed IMHO:

46 + pthread_mutex_destroy(&mutex);
47 + pthread_mutexattr_destroy(&mutex_attr);

The reason is that pthread_mutex_destroy() may invalidate the data at address &mutex_attr, and pthread_mutexattr_destroy() may return EINVAL. Not sure of the importance, but you may want to place (void) before both the calls...

Finally, instead of calling pthread_mutex_lock() directly, like in InformationCursor::rnd_init():

59 + pthread_mutex_lock(&sch_table->mutex);

it would be better to encapsulate the locking in a InfoSchemaTable::lock() method. This would make it much easier to change (for instance if you wanted to switch to a rwlock instead of a mutex...)

Cheers!

Jay

review: Needs Fixing

[Stewart Smith (stewart)]

On Thu, Jan 07, 2010 at 04:05:19PM -0000, Jay Pipes wrote:
> Some little things, one bigger thing. :)
>
> The bigger thing, and I'm sure you've already thought about this, is
> the fact that for each call to an I_S table, the underlying table/view
> rows are copied into the InformationCursor::rows memory. This is
> clearly a suboptimal strategy, but I'm going to assume you've already
> discussed this with Brian?

This strategy is complete poo :)

It is, however, the simplest patch to get to working from where we can
fix things to actually not suck (that patch is... well.. huge).

> Tiny style fixups:
>
> 77 + while(copy_iter != sch_table->getRows().end())
>
> One space after while, please :)

i want a commit hook :)

> There is some space/indent weirdness going on in the constructors
> (see below on diff). Indents should be 2 spaces (there is one area
> they are 4 spaces below.)

hrrm... maybe emacs mode fail there.

> Unless there's a good reason not to do so, the general trend I've
> seen in the code is to have constructors and other method definitions
> be in the .cc files and not the .h files where they are implicitly
> inlined. You have the setup of the mutex attr and the mutex itself
> duplicated in both constructors and they are in the .h file. I'd
> prefer to see the mutex and mutex attr initialization put into an
> init() private method and see the constructors defined in the .cc file.

okay. not that much of a deal either way, but sure.

> Also, the following lines should be reversed IMHO:
>
> 46 + pthread_mutex_destroy(&mutex);
> 47 + pthread_mutexattr_destroy(&mutex_attr);
>
> The reason is that pthread_mutex_destroy() may invalidate the data
> at address &mutex_attr, and pthread_mutexattr_destroy() may return
> EINVAL. Not sure of the importance, but you may want to place (void)
> before both the calls...

interesting... where you get this info from?

> Finally, instead of calling pthread_mutex_lock() directly, like in InformationCursor::rnd_init():
>
> 59 + pthread_mutex_lock(&sch_table->mutex);
>
> it would be better to encapsulate the locking in a
> InfoSchemaTable::lock() method. This would make it much easier to
> change (for instance if you wanted to switch to a rwlock instead of a
> mutex...)

I don't want it to be misunderstood as a lock on the
InfoSchemaTable... just on the rows...

but i also don't want the code lasting more than a trivial amount of
time :)

--
Stewart Smith

[Jay Pipes (jaypipes)]

Stewart Smith wrote:
> On Thu, Jan 07, 2010 at 04:05:19PM -0000, Jay Pipes wrote:
>> Some little things, one bigger thing. :)
>>
>> The bigger thing, and I'm sure you've already thought about this, is
>> the fact that for each call to an I_S table, the underlying table/view
>> rows are copied into the InformationCursor::rows memory. This is
>> clearly a suboptimal strategy, but I'm going to assume you've already
>> discussed this with Brian?
>
> This strategy is complete poo :)
>
> It is, however, the simplest patch to get to working from where we can
> fix things to actually not suck (that patch is... well.. huge).

OK, I figured as much :)

>> Also, the following lines should be reversed IMHO:
>>
>> 46 + pthread_mutex_destroy(&mutex);
>> 47 + pthread_mutexattr_destroy(&mutex_attr);
>>
>> The reason is that pthread_mutex_destroy() may invalidate the data
>> at address &mutex_attr, and pthread_mutexattr_destroy() may return
>> EINVAL. Not sure of the importance, but you may want to place (void)
>> before both the calls...
>
> interesting... where you get this info from?

Per this link:

http://docs.sun.com/app/docs/doc/802-1949/6i5ur8qam?a=view

"If a mutex is dynamically allocated and was initialized with an
attribute object, its attribute object should be freed with
pthread_mutexattr_destroy() before the mutex itself is freed."

>> Finally, instead of calling pthread_mutex_lock() directly, like in InformationCursor::rnd_init():
>>
>> 59 + pthread_mutex_lock(&sch_table->mutex);
>>
>> it would be better to encapsulate the locking in a
>> InfoSchemaTable::lock() method. This would make it much easier to
>> change (for instance if you wanted to switch to a rwlock instead of a
>> mutex...)
>
> I don't want it to be misunderstood as a lock on the
> InfoSchemaTable... just on the rows...

But it *is* essentially a lock on the whole object :)

> but i also don't want the code lasting more than a trivial amount of
> time :)

ack. :)

-jay

[Jay Pipes (jaypipes)]

sweet. :)

review: Approve

[Brian Aker (brianaker)]

In file included from plugin/innobase/handler/ha_innodb.cc:85:
./drizzled/plugin/info_schema_table.h: In member function 'void drizzled::plugin::InfoSchemaTable::init()':
./drizzled/plugin/info_schema_table.h:309: error: 'PTHREAD_MUTEX_RECURSIVE_NP' was not declared in this scope
gmake[2]: *** [plugin/innobase/handler/plugin_libinnobase_plugin_la-ha_innodb.lo] Error 1
gmake[2]: Leaving directory `/home/hudson/hudson/workspace/drizzle-build-freebsd-8.0'

Can you run your code through param build before submitting for inclusion?

Thanks!

review: Needs Fixing

[Stewart Smith (stewart)]

now ran through param build. seems to be okay everywhere.

[Stewart Smith (stewart)]

http://hudson.drizzle.org/job/Drizzle-param-ubuntu904-64bit/68/consoleFull

shows a func_gconcat failure.

I have logged into the exact same machine, built and run the tests (both debug and non-debug) and cannot at all reproduce this.

I currently theorize something to do with a dirty tree that is being used to build it through hudson.

Is someone able to blast away the dir for hudson and re-run the build?

[Stewart Smith (stewart)]

Ignore the above. Comment is on wrong branch.

[Brian Aker (brianaker)]

Another failure, from Gaz ->

main.information_schema [ fail ] timeout

Merge is also still failing.

review: Needs Fixing

[Stewart Smith (stewart)]

Going to mark this as rejected due to Brian's DATA_DICTIONARY work.

Unmerged revisions

1266. By Stewart Smith on 2010-01-14

need _XOPEN_SOURCE defined on Linux to get portable recursive mutex definition.

1265. By Stewart Smith on 2010-01-14

pthread.hneeds to be included in info_schema_table.h

1264. By Stewart Smith on 2010-01-11

move mutex init in InfoSchemaTable to private init() method and call from both constructors

1263. By Stewart Smith on 2010-01-11

free mutex attr before mutex. as per:

http://docs.sun.com/app/docs/doc/802-1949/6i5ur8qam?a=view

"If a mutex is dynamically allocated and was initialized with an
attribute object, its attribute object should be freed with
pthread_mutexattr_destroy() before the mutex itself is freed."

1262. By Stewart Smith on 2010-01-06

Rows in the I_S plugin object make it not thread safe. Due to the way we do subselects, it's also recursive (fillTable can call fillTable).... so we have a recursive mutex.

=== modified file 'drizzled/plugin/info_schema_table.h'
--- drizzled/plugin/info_schema_table.h	2010-01-06 02:20:42 +0000
+++ drizzled/plugin/info_schema_table.h	2010-01-14 02:20:23 +0000
@@ -29,6 +29,10 @@
 #include <string>
 #include <set>
 #include <algorithm>
+#ifndef _XOPEN_SOURCE
+#define _XOPEN_SOURCE 500  // for portable recursive mutex definition on linux
+#endif
+#include <pthread.h>
 
 class Session;
 class TableList;
@@ -301,11 +305,23 @@
   InfoSchemaTable();
   InfoSchemaTable(const InfoSchemaTable &);
   InfoSchemaTable& operator=(const InfoSchemaTable &);
+
+private:
+  void init()
+  {
+    pthread_mutexattr_init(&mutex_attr);
+    pthread_mutexattr_settype(&mutex_attr, PTHREAD_MUTEX_RECURSIVE);
+    pthread_mutex_init(&mutex, &mutex_attr);
+  }
+
 public:
 
   typedef std::vector<const ColumnInfo *> Columns;
   typedef std::vector<InfoSchemaRecord *> Rows;
-  
+
+  pthread_mutex_t mutex;
+  pthread_mutexattr_t mutex_attr;
+
   InfoSchemaTable(const std::string& tab_name,
                   Columns& in_column_info,
                   int idx_col1,
@@ -324,7 +340,9 @@
       column_info(in_column_info),
       rows(),
       i_s_methods(in_methods)
-  {}
+    {
+      init();
+    }
 
   explicit InfoSchemaTable(const std::string& tab_name)
     :
@@ -337,7 +355,9 @@
       column_info(),
       rows(),
       i_s_methods(NULL)
-  {}
+  {
+    init();
+  }
 
   virtual ~InfoSchemaTable()
   {
@@ -345,6 +365,8 @@
                   rows.end(),
                   DeleteRows());
     rows.clear();
+    pthread_mutexattr_destroy(&mutex_attr);
+    pthread_mutex_destroy(&mutex);
   }
 
   /**

=== modified file 'plugin/information_engine/information_cursor.cc'
--- plugin/information_engine/information_cursor.cc	2010-01-06 02:20:42 +0000
+++ plugin/information_engine/information_cursor.cc	2010-01-14 02:20:23 +0000
@@ -61,6 +61,8 @@
 int InformationCursor::rnd_init(bool)
 {
   plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();
+  pthread_mutex_lock(&sch_table->mutex);
+
   if (sch_table)
   {
     if (! sch_table->getRows().empty())
@@ -69,8 +71,24 @@
     }
     sch_table->fillTable(ha_session(),
                          table);
-    iter= sch_table->getRows().begin();
+
+    std::for_each(rows.begin(),
+                  rows.end(),
+                  plugin::DeleteRows());
+    rows.clear();
+
+    plugin::InfoSchemaTable::Rows::iterator copy_iter= sch_table->getRows().begin();
+
+    while(copy_iter != sch_table->getRows().end())
+    {
+      rows.push_back(new plugin::InfoSchemaRecord(*(*copy_iter)));
+      copy_iter++;
+    }
+
+    iter= rows.begin();
   }
+  pthread_mutex_unlock(&sch_table->mutex);
+
   return 0;
 }
 
@@ -78,10 +96,9 @@
 int InformationCursor::rnd_next(unsigned char *buf)
 {
   ha_statistic_increment(&SSV::ha_read_rnd_next_count);
-  plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();
 
-  if (iter != sch_table->getRows().end() &&
-      ! sch_table->getRows().empty())
+  if (iter != rows.end() &&
+      ! rows.empty())
   {
     (*iter)->copyRecordInto(buf);
     ++iter;
@@ -122,11 +139,10 @@
   memcpy(&cs, pos, sizeof(uint32_t));
 
   /* search for that data */
-  plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();
-  plugin::InfoSchemaTable::Rows::iterator it= find_if(sch_table->getRows().begin(),
-                                                      sch_table->getRows().end(),
+  plugin::InfoSchemaTable::Rows::iterator it= find_if(rows.begin(),
+                                                      rows.end(),
                                                       FindRowByChecksum(cs));
-  if (it != sch_table->getRows().end())
+  if (it != rows.end())
   {
     (*it)->copyRecordInto(buf);
   }

=== modified file 'plugin/information_engine/information_cursor.h'
--- plugin/information_engine/information_cursor.h	2009-12-22 08:05:32 +0000
+++ plugin/information_engine/information_cursor.h	2010-01-14 02:20:23 +0000
@@ -33,10 +33,16 @@
   THR_LOCK_DATA lock;      /* MySQL lock */
   InformationEngine::Share *share;
   drizzled::plugin::InfoSchemaTable::Rows::iterator iter;
+  drizzled::plugin::InfoSchemaTable::Rows rows;
 
 public:
   InformationCursor(drizzled::plugin::StorageEngine &engine, TableShare &table_arg);
-  ~InformationCursor() {}
+  ~InformationCursor() {
+    std::for_each(rows.begin(),
+                  rows.end(),
+                  drizzled::plugin::DeleteRows());
+    rows.clear();
+}
 
   int open(const char *name, int mode, uint32_t test_if_locked);
 
@@ -60,19 +66,22 @@
    */
   ha_rows estimate_rows_upper_bound()
   {
+    ha_rows retval= HA_POS_ERROR;
     if (share)
     {
       drizzled::plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();
       if (sch_table)
       {
+        pthread_mutex_lock(&sch_table->mutex);
         if (! sch_table->getRows().empty())
         {
           /* we multiply by 3 here to ensure enough memory is allocated for sort buffers */
-          return (3 * sch_table->getRows().size());
+          retval= 3 * sch_table->getRows().size();
         }
+        pthread_mutex_unlock(&sch_table->mutex);
       }
     }
-    return HA_POS_ERROR;
+    return retval;
   }
 };