Re: [Maria-developers] The query cache patch fixed by me and then Monty (most changes by him)

...

...

...

...

"Oleksandr" == Oleksandr Byelkin writes:

Oleksandr> Hi! Oleksandr> On 29.12.2010 23:29, Oleksandr Byelkin wrote:

...

On 29.12.2010 13:00, Oleksandr Byelkin wrote:

...

Hi!

Here is changes which was done:

I found a bug here, new version will be tomorrow...

Oleksandr> Here is new patch and relative patch against sources with previous patch. Oleksandr> New patch fixes switching ON query cache amd also remove some small bugs Oleksandr> about compilation without query cache. === modified file 'sql/set_var.cc' --- sql/set_var.cc 2010-12-04 14:32:42 +0000 <cut> +static void fix_query_cache_type(THD *thd, enum_var_type type) +{ + if (type == OPT_GLOBAL) + { + if (global_system_variables.query_cache_type != 0 && + query_cache.is_disabled()) + fix_query_cache_size(thd, type); I assume the above code will enable the query cache if it was disabled before? Please add a comment about this, as it's not self evident why we would call fix_query_cache_size() when the cache is disabled. + else if (global_system_variables.query_cache_type == 0) + query_cache.disable_query_cache(); + } +} <cut> +++ sql/sql_cache.cc 2010-12-30 17:33:22 +0000 @@ -286,6 +286,7 @@ if (and only if) this query has a registered result set writer (thd->net.query_cache_query). 4. Query_cache::invalidate + Query_cache::invalidate_locked_for_write - Called from various places to invalidate query cache based on data- base, table and myisam file name. During an on going invalidation the query cache is temporarily disabled. @@ -334,6 +335,8 @@ #include "../storage/myisammrg/ha_myisammrg.h" #include "../storage/myisammrg/myrg_def.h" <cut> +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) + + 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); + }); Didn't we talk about return with an error here if the query cache is disabled? <cut> { 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"); ); - if (query_cache.try_lock()) + if(query_cache.is_disabled()) + DBUG_VOID_RETURN; + + if (query_cache.try_lock(NULL, Query_cache::WAIT)) DBUG_VOID_RETURN; The above code is not safe as the query cache may be disabled between the test and try_lock(). Query_cache_block *query_block= (Query_cache_block*)net->query_cache_query; @@ -931,7 +1098,10 @@ if (net->query_cache_query == 0) DBUG_VOID_RETURN; - if (query_cache.try_lock()) + if(query_cache.is_disabled()) + DBUG_VOID_RETURN; + + if (query_cache.try_lock(NULL, Query_cache::WAIT)) { net->query_cache_query = 0; DBUG_VOID_RETURN; Same above (and in a couple of other cases). <cut> +void Query_cache::disable_query_cache(void) +{ + m_cache_status= DISABLE_REQUEST; + /* + If there is no requests in progress try to free buffer. + try_lock(TRY) will exit immediately if there is lock. + unlock() should free block. + */ + if (m_requests_in_progress == 0 && !try_lock(NULL, TRY)) + unlock(); +} It's not safe to set mf_cache_status to DISABLE_REQUEST directly, we need the mutex here and we should onlt set it if it's not already DISABLED. The reason is if the original warible was m_cache_status= DISABLE (it's possible, as we in set_var.cc test is_disabled() without a mutex and it can change between calls) we will set it to DISABLE_REQUEST forever. Sanja, lets discuss the issue of ::try_lock in #maria to resolve this quickly. Regards, Monty