* kpettersson@stripped <kpettersson@stripped> [07/09/07 19:07]:
> @@ -727,15 +729,16 @@ void query_cache_abort(NET *net)
> if (query_block)
> {
> DUMP(&query_cache);
> + safe_mutex_assert_owner(&query_cache.structure_guard_mutex);
> BLOCK_LOCK_WR(query_block);
> // The following call will remove the lock on query_block
> + query_cache.drop_writer(query_block);
> query_cache.free_query(query_block);
Please move the new line before the comment.
Do you need the block being locked for write for drop_writer()?
I suspect that you don't, in this case, move the new line before
BLOCK_LOCK_WR.
> @@ -762,14 +765,6 @@ void query_cache_end_of_result(THD *thd)
>
> STRUCT_LOCK(&query_cache.structure_guard_mutex);
>
> - bool interrupt;
> - query_cache.wait_while_table_flush_is_in_progress(&interrupt);
> - if (interrupt)
> - {
> - STRUCT_UNLOCK(&query_cache.structure_guard_mutex);
> - DBUG_VOID_RETURN;
> - }
> -
> query_block= ((Query_cache_block*) thd->net.query_cache_query);
> if (query_block)
> {
> @@ -787,14 +782,15 @@ void query_cache_end_of_result(THD *thd)
>
> if (header->result() == 0)
> {
> - DBUG_PRINT("error", ("End of data with no result blocks; "
> - "Query '%s' removed from cache.", header->query()));
> /*
> - Extra safety: empty result should not happen in the normal call
> - to this function. In the release version that query should be ignored
> - and removed from QC.
> + This can happen if query cache is being invalidated under
> + m_cache_status= Query_cache::FLUSH_IN_PROGRESS or
> + Query_cache::TABLE_FLUSH_IN_PROGRESS
> */
> - DBUG_ASSERT(0);
> + DBUG_PRINT("error", ("End of data with no result blocks; "
> + "Query '%s' removed from cache.", header->query()));
> + /* Drop the writer. */
> + query_cache.drop_writer(query_block);
> query_cache.free_query(query_block);
> STRUCT_UNLOCK(&query_cache.structure_guard_mutex);
> DBUG_VOID_RETURN;
If flush() happens to reset writer()->query_cache_query to 0
before you get to it, you never drop the writer. In other words,
this change introduces a race.
It's best to leave the old code as is and drop the writer inside
flush().
> @@ -1067,7 +1066,11 @@ def_week_frmt: %lu",
> inserts++;
> queries_in_cache++;
> net->query_cache_query= (uchar*) query_block;
> - header->writer(net);
> +
> + /* Register result writer */
> + m_number_of_active_writers++;
> + header->writer(net);
> +
Personal taste perhaps, I would add 'add_writer()' call.
> /**
> + @brief Wait on the predicate m_number_of_active_writers==0 and then locks
> + structure_guard_mutex.
> +
> + @pre Query_cache:m_status has to be FLUSH_IN_PROGRESS to be able to prevent
> + new writers from being created and structure_guard_mutex has to be locked.
> +*/
> +void Query_cache::wait_until_no_active_writers(void)
Missing empty line.
> +{
> + safe_mutex_assert_owner(&structure_guard_mutex);
> + while (m_number_of_active_writers > 0)
> + pthread_cond_wait(&COND_no_active_writers, &structure_guard_mutex);
> +}
I would perhaps move the assignment of the m_cache_status to this
function, instead of making it a pre-condition.
Or even more: I would rename the function to something along "lock cache
exclusively", call wait_while_table_flush_is_in_progress from it,
then set the status, and then wait till all writers are gone.
> +
> +
> +/**
> @brief Remove all cached queries that uses the given database
> */
>
> @@ -1581,6 +1600,8 @@ void Query_cache::invalidate(char *db)
> STRUCT_UNLOCK(&structure_guard_mutex);
> return;
> }
> + m_cache_status= Query_cache::FLUSH_IN_PROGRESS;
> + wait_until_no_active_writers();
This introduces a race condition -- breaks the predicate
associated with 'interrupt' boolean out variable of
wait_while_table_flush_is_in_progress -- namely that if
'interrupt' is set, we can cancel the current operation since the
entire contents of the cache will be flushed out.
I presume the old code had a bug then, too -- since it would try
to free a block without locking it first. The correct change is to
lock the block before accessing it.
What does BLOCK_LOCK_WR actually protect?
> @@ -1697,6 +1720,9 @@ void Query_cache::pack(ulong join_limit,
> DBUG_VOID_RETURN;
> }
>
> + m_cache_status= Query_cache::FLUSH_IN_PROGRESS;
> + wait_until_no_active_writers();
Same race condition can happen here.
> + DBUG_ASSERT(m_number_of_active_writers >0);
^^ (style)
> + query->writer()->query_cache_query= 0;
> + query->writer(0);
> + --m_number_of_active_writers;
> + if (m_number_of_active_writers == 0)
> + pthread_cond_broadcast(&COND_no_active_writers);
> + }
> }
> /*
> @@ -3075,6 +3126,8 @@ void Query_cache::insert_into_free_memor
> uint Query_cache::find_bin(ulong size)
> {
> DBUG_ENTER("Query_cache::find_bin");
> + if (size < min_allocation_unit)
> + DBUG_RETURN(mem_bin_steps);
What is this change for? Could it be pushed separately? (along with
safe_mutex_assert_owner, comment and whitespace changes).
--
-- Konstantin Osipov Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com The best DATABASE COMPANY in the GALAXY
