Hello Sanja,
On Wed, Apr 09, 2008 at 03:21:02PM +0300, sanja@stripped wrote:
> ChangeSet@stripped, 2008-04-09 15:20:58+03:00, bell@stripped +2 -0
> Problems of partially freed waiting quque fixed (BUG#35040)
>
> mysys/wqueue.c@stripped, 2008-04-09 15:20:55+03:00, bell@stripped +14
> -15
> Problems of partially freed waiting quque fixed.
>
> storage/maria/unittest/ma_pagecache_rwconsist.c@stripped, 2008-04-09 15:20:55+03:00,
> bell@stripped +5 -5
> Explicitly assigned initial value for increasing readability.
> Dbug file flush after each line for better debugging.
> Fixed code style.
>
> --- a/mysys/wqueue.c 2008-02-25 23:31:56 +02:00
> +++ b/mysys/wqueue.c 2008-04-09 15:20:55 +03:00
> @@ -147,9 +147,8 @@ void wqueue_release_one_locktype_from_qu
I'll comment on the total function, it will be easier than only on the
diff.
> void wqueue_release_one_locktype_from_queue(WQUEUE *wqueue)
> {
> struct st_my_thread_var *last= wqueue->last_thread;
> struct st_my_thread_var *next= last->next;
> struct st_my_thread_var *thread;
> struct st_my_thread_var *new_list= NULL;
> uint first_type= next->lock_type;
> if (first_type == MY_PTHREAD_LOCK_WRITE)
> {
> /* release first waiting for write lock */
> thread= next;
We do not really need to do 'thread= next', we could just remove it
and replace 'thread' by 'next' below until we reach the return.
Or is there a reason to keep it?
> pthread_cond_signal(&thread->suspend);
> if (thread == last)
> {
> DBUG_ASSERT(thread->next == thread);
If we come into this if(), thread == last is true, which implies
thread->next == last->next,
and last->next is 'next' by definition, and as we did 'thread= next',
it is sure that thread->next == thread, so you can remove the
assertion.
> wqueue->last_thread= NULL;
> }
> else
> last->next= thread->next;
Above you don't modify thread->next->prev, which should be done (like
in wqueue_unlink_from_queue()), IF the wqueue is used as double-linked
list. Fortunately, ma_pagecache.c uses
wqueue_release_one_locktype_from_queue() only on COND_WRLOCK queue,
which is always used as single-linked list. Please add into the
function's comment, that it operates on single-linked list.
An idea: a WQUEUE can be double-linked or single-linked, there is the
theoretical risk that someone uses one WQUEUE as single-linked and
then calls double-linked-specific functions on it; to better catch
such bad cases, we could add:
#ifndef DBUG_OFF
thread->prev= NULL; /* force segfault if used */
#endif
to wqueue_add_to_queue(). It would force a segfault if
wqueue_unlink_from_queue() is called.
It is just an idea and is not related to your patch, feel free to
ignore it.
> --- a/storage/maria/unittest/ma_pagecache_rwconsist.c 2008-03-04 18:07:52 +02:00
> +++ b/storage/maria/unittest/ma_pagecache_rwconsist.c 2008-04-09 15:20:55 +03:00
> @@ -212,9 +212,9 @@ int main(int argc __attribute__((unused)
>
> #ifndef DBUG_OFF
> #if defined(__WIN__)
> - default_dbug_option= "d:t:i:O,\\test_pagecache_consist.trace";
> + default_dbug_option= "d:f:i:O,\\test_pagecache_consist.trace";
This "f" sounds strange: dbug/user.t says:
f[,functions]
Limit debugger actions to the specified list of functions.
An empty list of functions implies that all functions are selected.
Every function in the list may optionally be followed by a '/' -
this will implicitly select all the functions down the call stack.
Is this really what you wanted?
In the commit comment you wrote:
Dbug file flush after each line for better debugging.
but this is already achieved by the O flag in default_dbug_option.
> #else
> - default_dbug_option= "d:t:i:o,/tmp/test_pagecache_consist.trace";
> + default_dbug_option= "d:t:i:O,/tmp/test_pagecache_consist.trace";
ok for this one.
Ok to push after fixing the little problems.
Thanks.
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Mr. Guilhem Bichot <guilhem@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Sun Microsystems, Lead Software Engineer
/_/ /_/\_, /___/\___\_\___/ Bordeaux, France
<___/ www.mysql.com
