Hi Kristofer,
On 10/16/08 6:03 PM, Kristofer Pettersson wrote:
> #At file:///home/thek/Development/cpp/mysqlbzr/mysql-5.1-bug38551/
Bug target version is 6.0+
> 2666 Kristofer Pettersson 2008-10-16
> Bug#38551 query cache can still consume [very little] cpu time even when it is
> off.
>
> When the query cache is disabled, the server shouldn't attempt to take the
> query cache mutex.
>
> By using the command line option --query_cache_type=0, the user can disable
> the query cache permanently and avoid taking the query cache mutex.
> modified:
> sql/mysqld.cc
> sql/sql_cache.cc
> sql/sql_cache.h
>
> per-file messages:
> sql/sql_cache.cc
> * Added checks to see if query cache is disabled to avoid taking the
> structure_guard_mutex.
> * Shortened execution path while invalidating if the query cache size is 0.
> sql/sql_cache.h
> * Introduced new member and method for checking if the cache is disabled
> === modified file 'sql/mysqld.cc'
> --- a/sql/mysqld.cc 2008-10-13 10:22:36 +0000
> +++ b/sql/mysqld.cc 2008-10-16 20:11:04 +0000
> @@ -3458,6 +3458,19 @@ You should consider changing lower_case_
> files_charset_info :
> &my_charset_bin);
>
> +
> + /*
> + If we explicitly turn off query cache from the command line query cache will
> + be disabled for the reminder of the server life time. This is because we
> + want to avoid locking the QC specific mutex if query cache isn't going to
> + be used.
> + */
> + if (global_system_variables.query_cache_type == 0)
> + {
> + have_query_cache= SHOW_OPTION_NO;
This changes the semantic of the variable as its value is YES even if
query caching is disabled.
> + query_cache.disable_query_cache();
> + }
> +
Could you move this to Query_cache::init? This will cause compilation to
fail if server is being built without query cache.
> return 0;
> }
>
> @@ -6691,12 +6704,10 @@ The minimum value for this variable is 4
> (uchar**)&query_cache_min_res_unit, (uchar**)&query_cache_min_res_unit,
> 0, GET_ULONG, REQUIRED_ARG, QUERY_CACHE_MIN_RESULT_DATA_SIZE,
> 0, ULONG_MAX, 0, 1, 0},
> -#endif /*HAVE_QUERY_CACHE*/
> {"query_cache_size", OPT_QUERY_CACHE_SIZE,
> "The memory allocated to store results from old queries.",
> (uchar**)&query_cache_size, (uchar**)&query_cache_size, 0, GET_ULONG,
> REQUIRED_ARG, 0, 0, (longlong) ULONG_MAX, 0, 1024, 0},
> -#ifdef HAVE_QUERY_CACHE
> {"query_cache_type", OPT_QUERY_CACHE_TYPE,
> "0 = OFF = Don't cache or retrieve results. 1 = ON = Cache all results except
> SELECT SQL_NO_CACHE ... queries. 2 = DEMAND = Cache only SELECT SQL_CACHE ... queries.",
> (uchar**)&global_system_variables.query_cache_type,
>
> === modified file 'sql/sql_cache.cc'
> --- a/sql/sql_cache.cc 2008-09-30 13:47:01 +0000
> +++ b/sql/sql_cache.cc 2008-10-16 20:11:04 +0000
> @@ -670,7 +670,7 @@ void query_cache_insert(NET *net, const
> DBUG_ENTER("query_cache_insert");
>
> /* See the comment on double-check locking usage above. */
> - if (net->query_cache_query == 0)
> + if (query_cache.is_disabled() || net->query_cache_query == 0)
> DBUG_VOID_RETURN;
The check is needed here?
> DBUG_EXECUTE_IF("wait_in_query_cache_insert",
> @@ -778,7 +778,7 @@ void query_cache_end_of_result(THD *thd)
> DBUG_ENTER("query_cache_end_of_result");
>
> /* See the comment on double-check locking usage above. */
> - if (thd->net.query_cache_query == 0)
> + if (query_cache.is_disabled() || thd->net.query_cache_query == 0)
> DBUG_VOID_RETURN;
Here too? If query cache is disabled, query_cache_query will always be
NULL...
> if (thd->killed)
> @@ -891,7 +891,7 @@ Query_cache::Query_cache(ulong query_cac
> min_result_data_size(ALIGN_SIZE(min_result_data_size_arg)),
> def_query_hash_size(ALIGN_SIZE(def_query_hash_size_arg)),
> def_table_hash_size(ALIGN_SIZE(def_table_hash_size_arg)),
> - initialized(0)
> + initialized(0),m_query_cache_is_disabled(FALSE)
Add space after the comma.
> {
> ulong min_needed= (ALIGN_SIZE(sizeof(Query_cache_block)) +
> ALIGN_SIZE(sizeof(Query_cache_block_table)) +
> @@ -978,7 +978,7 @@ void Query_cache::store_query(THD *thd,
>
> See also a note on double-check locking usage above.
> */
> - if (thd->locked_tables || query_cache_size == 0)
> + if (m_query_cache_is_disabled || thd->locked_tables || query_cache_size == 0)
> DBUG_VOID_RETURN;
OK
> uint8 tables_type= 0;
>
> @@ -1161,14 +1161,18 @@ end:
> Check if the query is in the cache. If it was cached, send it
> to the user.
>
> - RESULTS
> - 1 Query was not cached.
> - 0 The query was cached and user was sent the result.
> - -1 The query was cached but we didn't have rights to use it.
> - No error is sent to the client yet.
> + @param thd Pointer to the thread handler
> + @param sql A pointer to the sql statement *
> + @param query_length Length of the statement in characters
> +
> + @return status code
> + @retval 1 Query was not cached.
> + @retval 0 The query was cached and user was sent the result.
> + @retval -1 The query was cached but we didn't have rights to use it.
> +
> + In case of -1, no error is sent to the client.
@note
>
> - NOTE
> - This method requires that sql points to allocated memory of size:
> + *) The buffer must be allocated memory of size:
> tot_length= query_length + thd->db_length + 1 + QUERY_CACHE_FLAGS_SIZE;
> */
>
> @@ -1183,6 +1187,9 @@ Query_cache::send_result_to_client(THD *
> Query_cache_query_flags flags;
> DBUG_ENTER("Query_cache::send_result_to_client");
>
> + if (m_query_cache_is_disabled)
> + DBUG_RETURN(0);
Necessary? the query_cache_type check below this one should be enough.
> /*
> Testing 'query_cache_size' without a lock here is safe: the thing
> we may loose is that the query won't be served from cache, but we
> @@ -2529,7 +2536,17 @@ void Query_cache::invalidate_table(THD *
> void Query_cache::invalidate_table(THD *thd, uchar * key, uint32 key_length)
> {
> bool interrupt;
> +
Trailing white space..
Please add comments.
> + if (m_query_cache_is_disabled)
> + return;
> +
> STRUCT_LOCK(&structure_guard_mutex);
> + if (query_cache_size == 0)
> + {
> + STRUCT_UNLOCK(&structure_guard_mutex);
> + return;
> + }
> +
Trailing white space.. otherwise OK.
> wait_while_table_flush_is_in_progress(&interrupt);
> if (interrupt)
> {
>
> === modified file 'sql/sql_cache.h'
> --- a/sql/sql_cache.h 2008-07-24 13:41:55 +0000
> +++ b/sql/sql_cache.h 2008-10-16 20:11:04 +0000
> @@ -279,6 +279,8 @@ private:
>
> Cache_status m_cache_status;
>
> + bool m_query_cache_is_disabled;
> +
Trailing white space..
> void free_query_internal(Query_cache_block *point);
> void invalidate_table_internal(THD *thd, uchar *key, uint32 key_length);
>
> @@ -437,6 +439,14 @@ protected:
> /* register query in cache */
> void store_query(THD *thd, TABLE_LIST *used_tables);
>
> + /**
> + At startup the user has an option to disable the query cache
> + to avoid locking the structure_guard_mutex.
> + This option is enabled by explicitly setting query_cache_type=OFF
> + in the command line.
> + */
> + void disable_query_cache(void) { m_query_cache_is_disabled= TRUE; }
> +
OK.
> /*
> Check if the query is in the cache and if this is true send the
> data to client.
> @@ -469,6 +479,8 @@ protected:
> friend void query_cache_end_of_result(THD *thd);
> friend void query_cache_abort(NET *net);
>
> + bool is_disabled(void) { return m_query_cache_is_disabled; }
> +
> bool is_flushing(void)
> {
> return (m_cache_status != Query_cache::NO_FLUSH_IN_PROGRESS);
>
We also need to prevent query_cache_type from being set if the query
cache was disabled at start up, otherwise setting query_cache_type will
succeed and the user will have no idea what is going on.
Regards,
-- Davi
