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

Proposed by Stewart Smith
Status: Superseded
Proposed branch: lp:~stewart/drizzle/bug492391-i_s-concurrency
Merge into: lp:~drizzle-trunk/drizzle/development
Diff against target: 172 lines (+57/-14)
3 files modified
drizzled/plugin/info_schema_table.h (+21/-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
Jay Pipes (community) Needs Fixing
Review via email: mp+16935@code.launchpad.net

This proposal has been superseded by a proposal from 2010-01-11.

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

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 :

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 :

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 :

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

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

1264. By Stewart Smith

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

1265. By Stewart Smith

pthread.hneeds to be included in info_schema_table.h

1266. By Stewart Smith

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

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
1=== modified file 'drizzled/plugin/info_schema_table.h'
2--- drizzled/plugin/info_schema_table.h 2009-12-28 19:35:53 +0000
3+++ drizzled/plugin/info_schema_table.h 2010-01-11 04:36:14 +0000
4@@ -301,11 +301,23 @@
5 InfoSchemaTable();
6 InfoSchemaTable(const InfoSchemaTable &);
7 InfoSchemaTable& operator=(const InfoSchemaTable &);
8+
9+private:
10+ void init()
11+ {
12+ pthread_mutexattr_init(&mutex_attr);
13+ pthread_mutexattr_settype(&mutex_attr, PTHREAD_MUTEX_RECURSIVE_NP);
14+ pthread_mutex_init(&mutex, &mutex_attr);
15+ }
16+
17 public:
18
19 typedef std::vector<const ColumnInfo *> Columns;
20 typedef std::vector<InfoSchemaRecord *> Rows;
21-
22+
23+ pthread_mutex_t mutex;
24+ pthread_mutexattr_t mutex_attr;
25+
26 InfoSchemaTable(const std::string& tab_name,
27 Columns& in_column_info,
28 int idx_col1,
29@@ -324,7 +336,9 @@
30 column_info(in_column_info),
31 rows(),
32 i_s_methods(in_methods)
33- {}
34+ {
35+ init();
36+ }
37
38 explicit InfoSchemaTable(const std::string& tab_name)
39 :
40@@ -337,7 +351,9 @@
41 column_info(),
42 rows(),
43 i_s_methods(NULL)
44- {}
45+ {
46+ init();
47+ }
48
49 virtual ~InfoSchemaTable()
50 {
51@@ -345,6 +361,8 @@
52 rows.end(),
53 DeleteRows());
54 rows.clear();
55+ pthread_mutexattr_destroy(&mutex_attr);
56+ pthread_mutex_destroy(&mutex);
57 }
58
59 /**
60
61=== modified file 'plugin/information_engine/information_cursor.cc'
62--- plugin/information_engine/information_cursor.cc 2009-12-21 10:00:33 +0000
63+++ plugin/information_engine/information_cursor.cc 2010-01-11 04:36:14 +0000
64@@ -61,6 +61,8 @@
65 int InformationCursor::rnd_init(bool)
66 {
67 plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();
68+ pthread_mutex_lock(&sch_table->mutex);
69+
70 if (sch_table)
71 {
72 if (! sch_table->getRows().empty())
73@@ -69,8 +71,24 @@
74 }
75 sch_table->fillTable(ha_session(),
76 table);
77- iter= sch_table->getRows().begin();
78+
79+ std::for_each(rows.begin(),
80+ rows.end(),
81+ plugin::DeleteRows());
82+ rows.clear();
83+
84+ plugin::InfoSchemaTable::Rows::iterator copy_iter= sch_table->getRows().begin();
85+
86+ while(copy_iter != sch_table->getRows().end())
87+ {
88+ rows.push_back(new plugin::InfoSchemaRecord(*(*copy_iter)));
89+ copy_iter++;
90+ }
91+
92+ iter= rows.begin();
93 }
94+ pthread_mutex_unlock(&sch_table->mutex);
95+
96 return 0;
97 }
98
99@@ -78,10 +96,9 @@
100 int InformationCursor::rnd_next(unsigned char *buf)
101 {
102 ha_statistic_increment(&SSV::ha_read_rnd_next_count);
103- plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();
104
105- if (iter != sch_table->getRows().end() &&
106- ! sch_table->getRows().empty())
107+ if (iter != rows.end() &&
108+ ! rows.empty())
109 {
110 (*iter)->copyRecordInto(buf);
111 ++iter;
112@@ -122,11 +139,10 @@
113 memcpy(&cs, pos, sizeof(uint32_t));
114
115 /* search for that data */
116- plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();
117- plugin::InfoSchemaTable::Rows::iterator it= find_if(sch_table->getRows().begin(),
118- sch_table->getRows().end(),
119+ plugin::InfoSchemaTable::Rows::iterator it= find_if(rows.begin(),
120+ rows.end(),
121 FindRowByChecksum(cs));
122- if (it != sch_table->getRows().end())
123+ if (it != rows.end())
124 {
125 (*it)->copyRecordInto(buf);
126 }
127
128=== modified file 'plugin/information_engine/information_cursor.h'
129--- plugin/information_engine/information_cursor.h 2009-12-22 08:05:32 +0000
130+++ plugin/information_engine/information_cursor.h 2010-01-11 04:36:14 +0000
131@@ -33,10 +33,16 @@
132 THR_LOCK_DATA lock; /* MySQL lock */
133 InformationEngine::Share *share;
134 drizzled::plugin::InfoSchemaTable::Rows::iterator iter;
135+ drizzled::plugin::InfoSchemaTable::Rows rows;
136
137 public:
138 InformationCursor(drizzled::plugin::StorageEngine &engine, TableShare &table_arg);
139- ~InformationCursor() {}
140+ ~InformationCursor() {
141+ std::for_each(rows.begin(),
142+ rows.end(),
143+ drizzled::plugin::DeleteRows());
144+ rows.clear();
145+}
146
147 int open(const char *name, int mode, uint32_t test_if_locked);
148
149@@ -60,19 +66,22 @@
150 */
151 ha_rows estimate_rows_upper_bound()
152 {
153+ ha_rows retval= HA_POS_ERROR;
154 if (share)
155 {
156 drizzled::plugin::InfoSchemaTable *sch_table= share->getInfoSchemaTable();
157 if (sch_table)
158 {
159+ pthread_mutex_lock(&sch_table->mutex);
160 if (! sch_table->getRows().empty())
161 {
162 /* we multiply by 3 here to ensure enough memory is allocated for sort buffers */
163- return (3 * sch_table->getRows().size());
164+ retval= 3 * sch_table->getRows().size();
165 }
166+ pthread_mutex_unlock(&sch_table->mutex);
167 }
168 }
169- return HA_POS_ERROR;
170+ return retval;
171 }
172 };
173