List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:January 21 2009 11:50pm
Subject:Re: bzr commit into mysql-5.0-bugteam branch (davi:2734) Bug#40264
View as plain text  
On 1/21/09 9:37 PM, Konstantin Osipov wrote:
> * 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?

I don't know, I think the client side won't ACK data sent after the 
socket is closed on the client side.

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

There might be a race but the test case is good enough to catch 
regressions as is. Let's just move on, no need to have a perfect test 
case :)

>> +  /*
>> +    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.

Actually, the comment is unrelated to the check. Looking now, seems 
irrelevant. Will substitute with a more relevant one.

> 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 ;)
>

I'll add the following comment: /* Read the changeset comments first! */

-- Davi Arnaut
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