Re: bk commit into 5.0 tree (thek:1.2489) BUG#28249

[Sergei Golubchik]

Hi!

On Jul 09, kpettersson@stripped wrote:
> ChangeSet@stripped, 2007-07-09 10:09:31+02:00, thek@adventure.(none) +5 -0
>   Bug#28249 Query Cache returns wrong result with concurrent insert /
>   certain lock
>   
>   A race condition in the integration between MyISAM and the query
>   cache code caused the query cache to fail to invalidate itself on
>   concurrently inserted data.
>   
>   This patch fix this problem by using the existing handler interface
>   which, upon each statement cache attempt, compare the size of the
>   table as viewed from the cache writing thread and with any snap shot
>   of the global table state. If the two sizes are different the global
>   table size is unknown and the current statement can't be cached.
> 
> --- 1.178/sql/ha_myisam.cc	2007-03-28 10:22:20 +02:00
> +++ 1.179/sql/ha_myisam.cc	2007-07-09 10:09:29 +02:00
> @@ -1922,3 +1922,66 @@ uint ha_myisam::checksum() const
>    return (uint)file->state->checksum;
>  }
>  
> +#ifdef HAVE_QUERY_CACHE
> +/**
> +  @brief Register a named table with a call back function to the query cache.
> +
> +  @param thd The thread handle
> +  @param table_key A pointer to the table name in the table cache
> +  @param key_length The length of the table name
> +  @param[out] engine_callback The pointer to the storage engine call back
> +    function, currently 0
> +  @param[out] engine_data The table size as seen by the current thread.

No, don't return the table size in engine_data. It creates an impression
that this is actually needed for something, while, in fact, the value
will be ignored, so it's completely useless.

> +  @note Despite the name of this function, it is used to check each statement
> +    before it is cached and not to register a table or callback function.
> +
> +  @see handler::register_query_cache_table
> +
> +  @return Upon success the engine_callback will point to the storage engine
> +    call back function and engine_data will point to the data length of the
> +    specified table.
> +    @retval TRUE Success
> +    @retval FALSE An error occured
> +*/
> +
> +my_bool ha_myisam::register_query_cache_table(THD *thd, char *table_name,
> +                                              uint table_name_len,
> +                                              qc_engine_callback
> +                                              *engine_callback,
> +                                              ulonglong *engine_data)
> +{
> +  /*
> +    No call back function is needed to determine if a cached statement
> +    is valid or not.
> +  */
> +  *engine_callback= 0;
> +
> +  /*
> +    If a concurrent insert has happened just before the current select
> +    statement the total size of the table is unknown and the statement can't
> +    be cached. This works if the global table state is updated on the data lock
> +    before any query cache invalidation happens. An insert invalidation will
> +    interrupt any ongoing result set writer from storing data in an invalidated
> +    table thanks to the structure_guard_mutex.

Very confusing comment. Even though I know what happens in the
bugreport, how insert invalidation works in general and why it fails
here, and how your fix helps, I still have troubles understanding the
comment. Try to rewrite it please.

> +  */
> +  ulonglong actual_data_file_length;
> +  ulonglong current_data_file_length;
> +
> +  pthread_mutex_lock(&file->lock.lock->mutex);

As we discussed with Kostja, you don't need a mutex here.
You only want to see if actual_data_file_length !=
current_data_file_length, you don't care about the exact value of
actual_data_file_length. So, it's ok if you'll see garbage there because
of non-atomic update.

> +  actual_data_file_length= file->s->state.state.data_file_length;
> +  current_data_file_length= file->save_state.data_file_length;
> +  pthread_mutex_unlock(&file->lock.lock->mutex);
> +
> +  *engine_data= current_data_file_length;
> +
> +  if (current_data_file_length != actual_data_file_length)
> +  {
> +    /* Don't cache current statement. */
> +    return FALSE;
> +  }
> +
> +  /* It is ok to try to cache current statement. */
> +  return TRUE;
> +}
> +#endif
> 
Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Developer
/_/  /_/\_, /___/\___\_\___/  MySQL GmbH, Radlkoferstr. 2, D-81373 München
       <___/                  Geschäftsführer: Kaj Arnö - HRB
München 162140