maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

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