Re: [Maria-developers] NEW Percona QC patch for review

Hi!

...

...

...

...

"Oleksandr" == Oleksandr Byelkin 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