Hi Kristofer,
OK to push given the below comments are addressed.
On 10/29/08 10:23 AM, Kristofer Pettersson wrote:
> #At file:///export/home/thek/bzr/mysql-6.0-bug38551/
>
> 2742 Kristofer Pettersson 2008-10-29
> 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.
> added:
> mysql-test/r/query_cache_disabled.result
> mysql-test/t/query_cache_disabled-master.opt
> mysql-test/t/query_cache_disabled.test
> modified:
> sql/set_var.cc
> sql/set_var.h
> sql/share/errmsg.txt
> sql/sql_cache.cc
> sql/sql_cache.h
>
> per-file messages:
> mysql-test/r/query_cache_disabled.result
> * Added test case to reflect changes in query_cache_type behavior.
> mysql-test/t/query_cache_disabled-master.opt
> * Added test case to reflect changes in query_cache_type behavior.
> mysql-test/t/query_cache_disabled.test
> * Added test case to reflect changes in query_cache_type behavior.
> sql/set_var.cc
> * Added before trigger to verify that query_cache_type wasn't turned off or on
> during
> runtime.
> sql/set_var.h
> * Changed order on how the enumeration is processed. By first projecting the
> character representation of the variable to a temporary integer we can have one
> function instead of two to check if the value is valid.
> sql/sql_cache.cc
> * If the query cache is disabled at start up, shorten the execution path and
> avoid
> grabbing the query cache mutex each time the invalidate interface methods are
> called.
> sql/sql_cache.h
> * Added new methods to set the query cache into a disabled state.
> === added file 'mysql-test/r/query_cache_disabled.result'
> --- a/mysql-test/r/query_cache_disabled.result 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/r/query_cache_disabled.result 2008-10-29 12:22:50 +0000
> @@ -0,0 +1,13 @@
> +SHOW GLOBAL VARIABLES LIKE 'query_cache_type';
> +Variable_name Value
> +query_cache_type OFF
> +SET GLOBAL query_cache_type=ON;
> +ERROR HY000: Query cache is disabled
> +SET GLOBAL query_cache_type=DEMAND;
> +ERROR HY000: Query cache is disabled
> +SET GLOBAL query_cache_type=OFF;
> +ERROR HY000: Query cache is disabled
> +SET GLOBAL query_cache_size=1024*1024;
> +SHOW GLOBAL VARIABLES LIKE 'query_cache_size';
> +Variable_name Value
> +query_cache_size 1048576
>
> === added file 'mysql-test/t/query_cache_disabled-master.opt'
> --- a/mysql-test/t/query_cache_disabled-master.opt 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/t/query_cache_disabled-master.opt 2008-10-29 12:22:50 +0000
> @@ -0,0 +1 @@
> +--query_cache_type=0
Use --loose-query_cache_type=0 so it does not fail if query cache was
disbled at build time.
> === added file 'mysql-test/t/query_cache_disabled.test'
> --- a/mysql-test/t/query_cache_disabled.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/t/query_cache_disabled.test 2008-10-29 12:22:50 +0000
> @@ -0,0 +1,14 @@
> +-- source include/have_query_cache.inc
> +#
> +# Bug#38551 query cache can still consume [very little] cpu time even when it is
> off.
> +#
> +SHOW GLOBAL VARIABLES LIKE 'query_cache_type';
> +--error ER_QUERY_CACHE_DISABLED
> +SET GLOBAL query_cache_type=ON;
> +--error ER_QUERY_CACHE_DISABLED
> +SET GLOBAL query_cache_type=DEMAND;
> +--error ER_QUERY_CACHE_DISABLED
> +SET GLOBAL query_cache_type=OFF;
> +SET GLOBAL query_cache_size=1024*1024;
> +SHOW GLOBAL VARIABLES LIKE 'query_cache_size';
Please add a set query_cache_size=0 to free resources.
> +
>
> === modified file 'sql/set_var.cc'
> --- a/sql/set_var.cc 2008-10-08 15:37:32 +0000
> +++ b/sql/set_var.cc 2008-10-29 12:22:50 +0000
> @@ -126,8 +126,10 @@ static void fix_net_read_timeout(THD *th
> static void fix_net_write_timeout(THD *thd, enum_var_type type);
> static void fix_net_retry_count(THD *thd, enum_var_type type);
> static void fix_max_join_size(THD *thd, enum_var_type type);
> +#ifdef HAVE_QUERY_CACHE
> static void fix_query_cache_size(THD *thd, enum_var_type type);
> static void fix_query_cache_min_res_unit(THD *thd, enum_var_type type);
> +#endif
> static void fix_myisam_max_sort_file_size(THD *thd, enum_var_type type);
> static void fix_max_binlog_size(THD *thd, enum_var_type type);
> static void fix_max_relay_log_size(THD *thd, enum_var_type type);
> @@ -420,10 +422,6 @@ static sys_var_thd_ulong sys_div_precinc
> &SV::div_precincrement);
> static sys_var_long_ptr sys_rpl_recovery_rank(&vars, "rpl_recovery_rank",
> &rpl_recovery_rank);
> -static sys_var_long_ptr sys_query_cache_size(&vars, "query_cache_size",
> - &query_cache_size,
> - fix_query_cache_size);
> -
> static sys_var_thd_ulong sys_range_alloc_block_size(&vars,
> "range_alloc_block_size",
> &SV::range_alloc_block_size);
> static sys_var_thd_ulong sys_query_alloc_block_size(&vars,
> "query_alloc_block_size",
> @@ -445,14 +443,18 @@ sys_var_enum_const sys_thread_han
> NULL);
>
> #ifdef HAVE_QUERY_CACHE
> +static sys_var_long_ptr sys_query_cache_size(&vars, "query_cache_size",
> + &query_cache_size,
> + fix_query_cache_size);
> static sys_var_long_ptr sys_query_cache_limit(&vars, "query_cache_limit",
> &query_cache.query_cache_limit);
> static sys_var_long_ptr sys_query_cache_min_res_unit(&vars,
> "query_cache_min_res_unit",
> &query_cache_min_res_unit,
> fix_query_cache_min_res_unit);
> +static int check_query_cache_type(THD *thd, set_var *var);
> static sys_var_thd_enum sys_query_cache_type(&vars, "query_cache_type",
> &SV::query_cache_type,
> - &query_cache_type_typelib);
> + &query_cache_type_typelib, NULL, check_query_cache_type);
> static sys_var_thd_bool
> sys_query_cache_wlock_invalidate(&vars, "query_cache_wlock_invalidate",
> &SV::query_cache_wlock_invalidate);
> @@ -1103,10 +1105,9 @@ static void fix_net_retry_count(THD *thd
> {}
> #endif /* HAVE_REPLICATION */
>
> -
> +#ifdef HAVE_QUERY_CACHE
> static void fix_query_cache_size(THD *thd, enum_var_type type)
> {
> -#ifdef HAVE_QUERY_CACHE
> ulong new_cache_size= query_cache.resize(query_cache_size);
>
> /*
> @@ -1120,11 +1121,34 @@ static void fix_query_cache_size(THD *th
> query_cache_size, new_cache_size);
>
> query_cache_size= new_cache_size;
> -#endif
> }
>
>
> -#ifdef HAVE_QUERY_CACHE
> +/**
> + Trigger before query_cache_type variable is updated.
> + @param thd Thread handler
> + @param var Pointer to the new variable status
> +
> + @return Status code
> + @retval 1 Failure
> + @retval 0 Success
> +*/
> +
> +static int check_query_cache_type(THD *thd, set_var *var)
> +{
> + /*
> + Don't allow changes of the query_cache_type if the query cache
> + is disabled.
> + */
> + if (query_cache.is_disabled())
> + {
> + my_error(ER_QUERY_CACHE_DISABLED,MYF(0),ER_QUERY_CACHE_DISABLED);
The third argument is not necessary:
my_error(ER_QUERY_CACHE_DISABLED, MYF(0));
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> static void fix_query_cache_min_res_unit(THD *thd, enum_var_type type)
> {
> query_cache_min_res_unit=
> @@ -3775,6 +3799,16 @@ bool not_all_support_one_shot(List<set_v
> Functions to handle SET mysql_internal_variable=const_expr
> *****************************************************************************/
>
> +/**
> + Verify that the supplied value is correct.
> +
> + @param thd Thread handler
> +
> + @return status code
> + @retval -1 Failure
> + @retval 0 Success
> + */
> +
> int set_var::check(THD *thd)
> {
> if (var->is_readonly())
>
> === modified file 'sql/set_var.h'
> --- a/sql/set_var.h 2008-10-08 15:37:32 +0000
> +++ b/sql/set_var.h 2008-10-29 12:22:50 +0000
> @@ -485,10 +485,16 @@ public:
> { chain_sys_var(chain); }
> bool check(THD *thd, set_var *var)
> {
> - int ret= 0;
> - if (check_func)
> - ret= (*check_func)(thd, var);
> - return ret ? ret : check_enum(thd, var, enum_names);
> + /*
> + check_enum fails if the character representation supplied was wrong
> + or that the integer value was wrong or missing.
> + */
> + if (check_enum(thd, var, enum_names))
> + return TRUE;
> + else if ((check_func&& (*check_func)(thd, var)))
> + return TRUE;
> + else
> + return FALSE;
> }
> bool update(THD *thd, set_var *var);
> void set_default(THD *thd, enum_var_type type);
>
> === modified file 'sql/share/errmsg.txt'
> --- a/sql/share/errmsg.txt 2008-10-24 14:17:49 +0000
> +++ b/sql/share/errmsg.txt 2008-10-29 12:22:50 +0000
> @@ -6409,4 +6409,5 @@ WARN_PLUGIN_DELETE_BUILTIN
>
> WARN_PLUGIN_BUSY
> eng "Plugin is busy and will be uninstalled on shutdown"
> -
> +ER_QUERY_CACHE_DISABLED
> + eng "Query cache is disabled"
>
> === modified file 'sql/sql_cache.cc'
> --- a/sql/sql_cache.cc 2008-10-17 17:47:16 +0000
> +++ b/sql/sql_cache.cc 2008-10-29 12:22:50 +0000
> @@ -686,7 +686,7 @@ Query_cache::insert(Query_cache_tls *que
> DBUG_ENTER("Query_cache::insert");
>
> /* See the comment on double-check locking usage above. */
> - if (query_cache_tls->first_query_block == NULL)
> + if (is_disabled() || query_cache_tls->first_query_block == NULL)
> DBUG_VOID_RETURN;
>
> DBUG_EXECUTE_IF("wait_in_query_cache_insert",
> @@ -753,7 +753,7 @@ Query_cache::abort(Query_cache_tls *quer
> THD *thd= current_thd;
>
> /* See the comment on double-check locking usage above. */
> - if (query_cache_tls->first_query_block == NULL)
> + if (is_disabled() || query_cache_tls->first_query_block == NULL)
> DBUG_VOID_RETURN;
>
> STRUCT_LOCK(&structure_guard_mutex);
> @@ -906,7 +906,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)
> {
> ulong min_needed= (ALIGN_SIZE(sizeof(Query_cache_block)) +
> ALIGN_SIZE(sizeof(Query_cache_block_table)) +
> @@ -993,7 +993,7 @@ void Query_cache::store_query(THD *thd,
>
> See also a note on double-check locking usage above.
> */
> - if (thd->locked_tables_mode || query_cache_size == 0)
> + if (is_disabled() || thd->locked_tables_mode || query_cache_size == 0)
> DBUG_VOID_RETURN;
> uint8 tables_type= 0;
>
> @@ -1503,6 +1503,9 @@ void Query_cache::invalidate(THD *thd, T
> {
> DBUG_ENTER("Query_cache::invalidate (table list)");
>
> + if (is_disabled())
> + DBUG_VOID_RETURN;
> +
> using_transactions= using_transactions&&
> (thd->options& (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN));
> for (; tables_used; tables_used= tables_used->next_local)
> @@ -1529,6 +1532,9 @@ void Query_cache::invalidate(THD *thd, T
> void Query_cache::invalidate(CHANGED_TABLE_LIST *tables_used)
> {
> DBUG_ENTER("Query_cache::invalidate (changed table list)");
> + if (is_disabled())
> + DBUG_VOID_RETURN;
> +
> THD *thd= current_thd;
> for (; tables_used; tables_used= tables_used->next)
> {
> @@ -1576,7 +1582,10 @@ void Query_cache::invalidate(THD *thd, T
> my_bool using_transactions)
> {
> DBUG_ENTER("Query_cache::invalidate (table)");
> -
> +
> + if (is_disabled())
> + DBUG_VOID_RETURN;
> +
> using_transactions= using_transactions&&
> (thd->options& (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN));
> if (using_transactions&&
> @@ -1593,6 +1602,8 @@ void Query_cache::invalidate(THD *thd, c
> my_bool using_transactions)
> {
> DBUG_ENTER("Query_cache::invalidate (key)");
> + if (is_disabled())
> + DBUG_VOID_RETURN;
>
> using_transactions= using_transactions&&
> (thd->options& (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN));
> @@ -1654,7 +1665,8 @@ void Query_cache::invalidate(char *db)
> {
> bool restart= FALSE;
> DBUG_ENTER("Query_cache::invalidate (db)");
> -
> + if (is_disabled())
> + DBUG_VOID_RETURN;
> STRUCT_LOCK(&structure_guard_mutex);
> bool interrupt;
> wait_while_table_flush_is_in_progress(&interrupt);
> @@ -1742,6 +1754,9 @@ void Query_cache::invalidate_by_MyISAM_f
> void Query_cache::flush()
> {
> DBUG_ENTER("Query_cache::flush");
> + if (is_disabled())
> + DBUG_VOID_RETURN;
> +
> STRUCT_LOCK(&structure_guard_mutex);
> if (query_cache_size> 0)
> {
> @@ -1769,7 +1784,8 @@ void Query_cache::flush()
> void Query_cache::pack(ulong join_limit, uint iteration_limit)
> {
> DBUG_ENTER("Query_cache::pack");
> -
> + if (is_disabled())
> + DBUG_VOID_RETURN;
> bool interrupt;
> STRUCT_LOCK(&structure_guard_mutex);
> wait_while_table_flush_is_in_progress(&interrupt);
> @@ -1829,6 +1845,15 @@ void Query_cache::init()
> pthread_cond_init(&COND_cache_status_changed, NULL);
> m_cache_status= Query_cache::NO_FLUSH_IN_PROGRESS;
> initialized = 1;
> + /*
> + 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)
> + query_cache.disable_query_cache();
> +
> DBUG_VOID_RETURN;
> }
Please also add the is_disabled check to invalidate_locked_for_write.
> === modified file 'sql/sql_cache.h'
> --- a/sql/sql_cache.h 2008-08-10 14:49:52 +0000
> +++ b/sql/sql_cache.h 2008-10-29 12:22:50 +0000
> @@ -280,9 +280,10 @@ private:
> TABLE_FLUSH_IN_PROGRESS };
>
> Cache_status m_cache_status;
> -
> + bool m_query_cache_is_disabled;
> void free_query_internal(Query_cache_block *point);
> void invalidate_table_internal(THD *thd, uchar *key, uint32 key_length);
> + void disable_query_cache(void) { m_query_cache_is_disabled= TRUE; }
>
> protected:
> /*
> @@ -428,6 +429,8 @@ protected:
> uint def_query_hash_size = QUERY_CACHE_DEF_QUERY_HASH_SIZE,
> uint def_table_hash_size = QUERY_CACHE_DEF_TABLE_HASH_SIZE);
>
> + bool is_disabled(void) { return m_query_cache_is_disabled; }
> +
> /* initialize cache (mutex) */
> void init();
> /* resize query cache (return real query size, 0 if disabled) */
>
>
