List:Commits« Previous MessageNext Message »
From:Konstantin Osipov Date:January 21 2009 11:37pm
Subject:Re: bzr commit into mysql-5.0-bugteam branch (davi:2734) Bug#40264
View as plain text  
* Davi Arnaut <Davi.Arnaut@stripped> [09/01/20 02:52]:
>  2734 Davi Arnaut	2009-01-19
>       Bug#40264: Aborted cached query causes query to hang
>       indefinitely on next cache hit

OK to push.

>       The problem is that the query cache was storing partial results
>       if the statement failed when sending the results to the client.
>       This could cause clients to hang when trying to read the results
>       from the cache as they would, for example, wait indefinitely for
>       a eof packet that wasn't saved.
>       
>       The solution is to always discard the caching of a query that
>       failed to send its results to the associated client.
> modified:
>   mysql-test/r/query_cache_notembedded.result
>   mysql-test/t/query_cache_notembedded.test
>   sql/sql_cache.cc
> 
> per-file messages:
>   mysql-test/r/query_cache_notembedded.result
>     Add test case result for bug#40264
>   mysql-test/t/query_cache_notembedded.test
>     Add test case for bug#40264
>   sql/sql_cache.cc
>     Abort if a error was reported.
> === modified file 'mysql-test/r/query_cache_notembedded.result'
> --- a/mysql-test/r/query_cache_notembedded.result	2006-10-04 11:09:37 +0000
> +++ b/mysql-test/r/query_cache_notembedded.result	2009-01-19 23:48:55 +0000
> @@ -345,3 +345,19 @@ id
>  drop table t1;
>  drop function f1;
>  set GLOBAL query_cache_size=0;
> +DROP TABLE IF EXISTS t1;
> +FLUSH STATUS;
> +SET GLOBAL query_cache_size=1048576;
> +CREATE TABLE t1 (a INT);
> +INSERT INTO t1 VALUES (1),(2),(3),(4),(5);
> +SHOW STATUS LIKE 'Qcache_queries_in_cache';
> +Variable_name	Value
> +Qcache_queries_in_cache	0
> +LOCK TABLES t1 WRITE;
> +SELECT * FROM t1;
> +UNLOCK TABLES;
> +SHOW STATUS LIKE 'Qcache_queries_in_cache';
> +Variable_name	Value
> +Qcache_queries_in_cache	0
> +DROP TABLE t1;
> +SET GLOBAL query_cache_size= default;
> 
> === modified file 'mysql-test/t/query_cache_notembedded.test'
> --- a/mysql-test/t/query_cache_notembedded.test	2006-12-19 14:31:10 +0000
> +++ b/mysql-test/t/query_cache_notembedded.test	2009-01-19 23:48:55 +0000
> @@ -222,3 +222,34 @@ disconnect con2;
>  connection default;
>  
>  set GLOBAL query_cache_size=0;
> +
> +#
> +# Bug#40264: Aborted cached query causes query to hang indefinitely on next cache
> hit
> +#
> +
> +--disable_warnings
> +DROP TABLE IF EXISTS t1;
> +--enable_warnings
> +
> +FLUSH STATUS;
> +SET GLOBAL query_cache_size=1048576;
> +CREATE TABLE t1 (a INT);
> +INSERT INTO t1 VALUES (1),(2),(3),(4),(5);
> +SHOW STATUS LIKE 'Qcache_queries_in_cache';
> +LOCK TABLES t1 WRITE;
> +connect(con1,localhost,root,,);
> +--send SELECT * FROM t1
> +connection default;
> +let $show_type= open tables where `table`='t1' and in_use=2;
> +let $show_pattern= '%t1%2%';
> +--source include/wait_show_pattern.inc
> +dirty_close con1;

I can't think of a race here right now, but I do suspect one.

Does dirty_close() guarantee the  other side has received the FIN
packet? It seems no? Unless this is the case, we sometimes may have FIN
come after the first query is finished?

Please feel free to push the test case, if you're ready to 
investigate races in it later on :)

> +  /*
> +    We could probably check if the entire response (including eof packet)
> +    is already cached (ie: packets are being buffered on the server) before
> +    the connection broke, but in practice what is necessary is to not store
> +    partial results and the aforementioned corner case is also aborted for
> +    simplicity's sake (not trivial to implement in later server versions).
> +  */
> +  if (thd->killed || thd->net.report_error)
>    {

My two cents: instead of a comment about something hypothetical,
it'd be more appropriate (IMHO) to explain here that we're
actually checking for cases, when NET layer sets
thd->net.report_error without having set an error message in THD,
and thus without having aborted the query cache. When reading this
patch, the first question I asked myself was why do you need this
check, isn't my_error() calling query_cache_abort() already? And
then I remembered this edge case with NET.

Is it perhaps because I first read the patch, and then the
changeset comments?-) But that's exactly how others will be
reading it, so I think it's the right order ;)

>      query_cache_abort(&thd->net);
>      DBUG_VOID_RETURN;

-- 
Thread
bzr commit into mysql-5.0-bugteam branch (davi:2734) Bug#40264Davi Arnaut20 Jan
  • Re: bzr commit into mysql-5.0-bugteam branch (davi:2734) Bug#40264Konstantin Osipov22 Jan
    • Re: bzr commit into mysql-5.0-bugteam branch (davi:2734) Bug#40264Davi Arnaut22 Jan