maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "Oleksandr" == Oleksandr Byelkin <sanja@xxxxxxxxxxxx> writes:

Oleksandr> Hi!
Oleksandr> Here is new patch. I hope I understood you.

I assume that since your last patch, you have only changed the things
I commented on in my last review.

<cut>

> === modified file 'sql/sql_cache.cc'
> --- sql/sql_cache.cc	2010-11-30 21:11:03 +0000
> +++ sql/sql_cache.cc	2011-05-11 05:44:27 +0000

<cut>

> -bool Query_cache::try_lock(bool use_timeout)
> +bool Query_cache::try_lock(THD *thd, Cache_try_lock_mode mode)
>  {
>    bool interrupt= FALSE;
> +  const char* old_proc_info;
>    DBUG_ENTER("Query_cache::try_lock");
> +  if (!thd)
> +    thd = current_thd;

Why testing and setting thd ?
Please fix this in the calling functions (as caller should always know
if thd is set or not).
If it can't be easily fixed, add at least a comment in which context
thd could not be set.

> +  old_proc_info= thd->proc_info;
> +  thd_proc_info(thd,"Waiting on query cache mutex");
>  
>    pthread_mutex_lock(&structure_guard_mutex);
> +  DBUG_EXECUTE_IF("status_wait_query_cache_mutex_sleep", {
> +      sleep(5);
> +    });
> +  if (m_cache_status == DISABLED)
> +  {
> +    pthread_mutex_unlock(&structure_guard_mutex);

You need to add:
thd->proc_info = old_proc_info;

> +    return TRUE;
> +  }

> @@ -488,11 +641,16 @@
>        }
>        else
>        {
> -        pthread_cond_wait(&COND_cache_status_changed, &structure_guard_mutex);
> +        DBUG_ASSERT(mode == TRY);
> +        interrupt= TRUE;
> +        break;

The following above the assert:

/**
  If we are here, then mode is == TRY and there was someone else using
  the query cache. (m_cache_lock_status != Query_cache::UNLOCKED).
  Signal that we didn't get a lock.
*/

DBUG_ASSERT(m_requests_in_progress > 1);

...

In this function, it will also be less code if you set interrupt to
TRUE at start of function.  Another options is they you use
LINT_INIT(interrupt) and set the variable also when
m_cache_lock_status == Query_cache::UNLOCKED.

<cut>

> @@ -866,14 +1034,18 @@
>  {
>    DBUG_ENTER("query_cache_insert");
>  
> -  /* See the comment on double-check locking usage above. */
>    if (net->query_cache_query == 0)
>      DBUG_VOID_RETURN;
>  
> +  DBUG_ASSERT(current_thd);
> +
>    DBUG_EXECUTE_IF("wait_in_query_cache_insert",
>                    debug_wait_for_kill("wait_in_query_cache_insert"); );
>  

Add a comment here:

/* First we check if query cache is disable without doing a mutex lock */

> -  if (query_cache.try_lock())
> +  if(query_cache.is_disabled())
> +    DBUG_VOID_RETURN;

Add a comment here:
  /*
    Lock the cache with try_lock(). try_lock() will fail if
    cache was disabled between the above test and lock.
  */

> +
> +  if (query_cache.try_lock(NULL, Query_cache::WAIT))
>      DBUG_VOID_RETURN;
>  
>    Query_cache_block *query_block= (Query_cache_block*)net->query_cache_query;
> @@ -931,7 +1103,10 @@
>    if (net->query_cache_query == 0)
>      DBUG_VOID_RETURN;
>  
> -  if (query_cache.try_lock())
> +  if(query_cache.is_disabled())
> +    DBUG_VOID_RETURN;

You can remove the above test, as we know the query cache was enabled
when the query started. So there is two cases:

1) Either it's still enabled (which is probably the case) and we don't
   need the test.
2) It's not enabled and the below try_lock() will catch it.

The extra test for is_disabled() is only relevant for
query_cache.insert() to ensure that in normal operations we don't
need to take any mutex when the cache is disabled.

> @@ -1096,6 +1272,12 @@
>  			query_cache_size_arg));
>    DBUG_ASSERT(initialized);
>  
> +  if (global_system_variables.query_cache_type == 0)
> +  {
> +    my_error(ER_QUERY_CACHE_IS_DISABLED, MYF(0));
> +    DBUG_RETURN(0);
> +  }
> +
>    lock_and_suspend();
>  

Shouldn't we do lock_and_suspend() BEFORE testing for
(query_cache_type == 0)?

For example the above may fail if we get two concurrent resize or
disable commands ?

<cut>

> @@ -1985,11 +2268,14 @@
>  {
>    DBUG_ENTER("Query_cache::pack");
>  
> +  if (is_disabled())
> +    DBUG_VOID_RETURN;
> +

We don't need the above. Same argument as for abort()

>    /*
>      If the entire qc is being invalidated we can bail out early
>      instead of waiting for the lock.
>    */
> -  if (try_lock())
> +  if (try_lock(NULL, Query_cache::WAIT))
>      DBUG_VOID_RETURN;
>  
>    if (query_cache_size == 0)

<cut>

Regards,
Monty