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

Proposed by Stewart Smith
Status: Rejected
Rejected by: Stewart Smith
Proposed branch: lp:~stewart/drizzle/bug492391-i_s-concurrency
Merge into: lp:~drizzle-trunk/drizzle/development
Diff against target: 183 lines (+61/-14)
3 files modified
drizzled/plugin/info_schema_table.h (+25/-3)
plugin/information_engine/information_cursor.cc (+24/-8)
plugin/information_engine/information_cursor.h (+12/-3)
To merge this branch: bzr merge lp:~stewart/drizzle/bug492391-i_s-concurrency
Reviewer Review Type Date Requested Status
Brian Aker Needs Fixing
Monty Taylor Pending
Jay Pipes Pending
Review via email: mp+17340@code.launchpad.net

This proposal supersedes a proposal from 2010-01-11.

To post a comment you must log in.
Revision history for this message
Stewart Smith (stewart) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Jay Pipes (jaypipes) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Stewart Smith (stewart) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Jay Pipes (jaypipes) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Jay Pipes (jaypipes) wrote : Posted in a previous version of this proposal

sweet. :)

review: Approve
Revision history for this message
Brian Aker (brianaker) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Stewart Smith (stewart) wrote :

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

Revision history for this message
Stewart Smith (stewart) wrote :

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?

Revision history for this message
Stewart Smith (stewart) wrote :

Ignore the above. Comment is on wrong branch.

Revision history for this message
Brian Aker (brianaker) wrote :

Another failure, from Gaz ->

main.information_schema [ fail ] timeout

Merge is also still failing.

review: Needs Fixing
Revision history for this message
Stewart Smith (stewart) wrote :

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

Unmerged revisions

1266. By Stewart Smith

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

1265. By Stewart Smith

pthread.hneeds to be included in info_schema_table.h

1264. By Stewart Smith

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

1263. By Stewart Smith

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

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
29#include <string>29#include <string>
30#include <set>30#include <set>
31#include <algorithm>31#include <algorithm>
32#ifndef _XOPEN_SOURCE
33#define _XOPEN_SOURCE 500 // for portable recursive mutex definition on linux
34#endif
35#include <pthread.h>
3236
33class Session;37class Session;
34class TableList;38class TableList;
@@ -301,11 +305,23 @@
301 InfoSchemaTable();305 InfoSchemaTable();
302 InfoSchemaTable(const InfoSchemaTable &);306 InfoSchemaTable(const InfoSchemaTable &);
303 InfoSchemaTable& operator=(const InfoSchemaTable &);307 InfoSchemaTable& operator=(const InfoSchemaTable &);
308
309private:
310 void init()
311 {
312 pthread_mutexattr_init(&mutex_attr);
313 pthread_mutexattr_settype(&mutex_attr, PTHREAD_MUTEX_RECURSIVE);
314 pthread_mutex_init(&mutex, &mutex_attr);
315 }
316
304public:317public:
305318
306 typedef std::vector<const ColumnInfo *> Columns;319 typedef std::vector<const ColumnInfo *> Columns;
307 typedef std::vector<InfoSchemaRecord *> Rows;320 typedef std::vector<InfoSchemaRecord *> Rows;
308 321
322 pthread_mutex_t mutex;
323 pthread_mutexattr_t mutex_attr;
324
309 InfoSchemaTable(const std::string& tab_name,325 InfoSchemaTable(const std::string& tab_name,
310 Columns& in_column_info,326 Columns& in_column_info,
311 int idx_col1,327 int idx_col1,
@@ -324,7 +340,9 @@
324 column_info(in_column_info),340 column_info(in_column_info),
325 rows(),341 rows(),
326 i_s_methods(in_methods)342 i_s_methods(in_methods)
327 {}343 {
344 init();
345 }
328346
329 explicit InfoSchemaTable(const std::string& tab_name)347 explicit InfoSchemaTable(const std::string& tab_name)
330 :348 :
@@ -337,7 +355,9 @@
337 column_info(),355 column_info(),
338 rows(),356 rows(),
339 i_s_methods(NULL)357 i_s_methods(NULL)
340 {}358 {
359 init();
360 }
341361
342 virtual ~InfoSchemaTable()362 virtual ~InfoSchemaTable()
343 {363 {
@@ -345,6 +365,8 @@
345 rows.end(),365 rows.end(),
346 DeleteRows());366 DeleteRows());
347 rows.clear();367 rows.clear();
368 pthread_mutexattr_destroy(&mutex_attr);
369 pthread_mutex_destroy(&mutex);
348 }370 }
349371
350 /**372 /**
351373
=== 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 @@
61int InformationCursor::rnd_init(bool)61int InformationCursor::rnd_init(bool)
62{62{
63 plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();63 plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();
64 pthread_mutex_lock(&sch_table->mutex);
65
64 if (sch_table)66 if (sch_table)
65 {67 {
66 if (! sch_table->getRows().empty())68 if (! sch_table->getRows().empty())
@@ -69,8 +71,24 @@
69 }71 }
70 sch_table->fillTable(ha_session(),72 sch_table->fillTable(ha_session(),
71 table);73 table);
72 iter= sch_table->getRows().begin();74
75 std::for_each(rows.begin(),
76 rows.end(),
77 plugin::DeleteRows());
78 rows.clear();
79
80 plugin::InfoSchemaTable::Rows::iterator copy_iter= sch_table->getRows().begin();
81
82 while(copy_iter != sch_table->getRows().end())
83 {
84 rows.push_back(new plugin::InfoSchemaRecord(*(*copy_iter)));
85 copy_iter++;
86 }
87
88 iter= rows.begin();
73 }89 }
90 pthread_mutex_unlock(&sch_table->mutex);
91
74 return 0;92 return 0;
75}93}
7694
@@ -78,10 +96,9 @@
78int InformationCursor::rnd_next(unsigned char *buf)96int InformationCursor::rnd_next(unsigned char *buf)
79{97{
80 ha_statistic_increment(&SSV::ha_read_rnd_next_count);98 ha_statistic_increment(&SSV::ha_read_rnd_next_count);
81 plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();
8299
83 if (iter != sch_table->getRows().end() &&100 if (iter != rows.end() &&
84 ! sch_table->getRows().empty())101 ! rows.empty())
85 {102 {
86 (*iter)->copyRecordInto(buf);103 (*iter)->copyRecordInto(buf);
87 ++iter;104 ++iter;
@@ -122,11 +139,10 @@
122 memcpy(&cs, pos, sizeof(uint32_t));139 memcpy(&cs, pos, sizeof(uint32_t));
123140
124 /* search for that data */141 /* search for that data */
125 plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();142 plugin::InfoSchemaTable::Rows::iterator it= find_if(rows.begin(),
126 plugin::InfoSchemaTable::Rows::iterator it= find_if(sch_table->getRows().begin(),143 rows.end(),
127 sch_table->getRows().end(),
128 FindRowByChecksum(cs));144 FindRowByChecksum(cs));
129 if (it != sch_table->getRows().end())145 if (it != rows.end())
130 {146 {
131 (*it)->copyRecordInto(buf);147 (*it)->copyRecordInto(buf);
132 }148 }
133149
=== 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 @@
33 THR_LOCK_DATA lock; /* MySQL lock */33 THR_LOCK_DATA lock; /* MySQL lock */
34 InformationEngine::Share *share;34 InformationEngine::Share *share;
35 drizzled::plugin::InfoSchemaTable::Rows::iterator iter;35 drizzled::plugin::InfoSchemaTable::Rows::iterator iter;
36 drizzled::plugin::InfoSchemaTable::Rows rows;
3637
37public:38public:
38 InformationCursor(drizzled::plugin::StorageEngine &engine, TableShare &table_arg);39 InformationCursor(drizzled::plugin::StorageEngine &engine, TableShare &table_arg);
39 ~InformationCursor() {}40 ~InformationCursor() {
41 std::for_each(rows.begin(),
42 rows.end(),
43 drizzled::plugin::DeleteRows());
44 rows.clear();
45}
4046
41 int open(const char *name, int mode, uint32_t test_if_locked);47 int open(const char *name, int mode, uint32_t test_if_locked);
4248
@@ -60,19 +66,22 @@
60 */66 */
61 ha_rows estimate_rows_upper_bound()67 ha_rows estimate_rows_upper_bound()
62 {68 {
69 ha_rows retval= HA_POS_ERROR;
63 if (share)70 if (share)
64 {71 {
65 drizzled::plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();72 drizzled::plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();
66 if (sch_table)73 if (sch_table)
67 {74 {
75 pthread_mutex_lock(&sch_table->mutex);
68 if (! sch_table->getRows().empty())76 if (! sch_table->getRows().empty())
69 {77 {
70 /* we multiply by 3 here to ensure enough memory is allocated for sort buffers */78 /* we multiply by 3 here to ensure enough memory is allocated for sort buffers */
71 return (3 * sch_table->getRows().size());79 retval= 3 * sch_table->getRows().size();
72 }80 }
81 pthread_mutex_unlock(&sch_table->mutex);
73 }82 }
74 }83 }
75 return HA_POS_ERROR;84 return retval;
76 }85 }
77};86};
7887