Re: [Maria-developers] [Commits] Rev 4056: MDEV-5607: Query cache destroys uninitialized rwlock in file:///home/bell/maria/bzr/work-maria-5.5-MDEV-5607/
Hi Sanja, JFYI: while reporting this bug I came up with two questions that I couldn't answer quickly. Probably they make sense, if not just ignore them: 1. This code was introduced with https://bugs.launchpad.net/maria/+bug/782223, revision: wlad@montyprogram.com-20110514163720-rmico2qegtptjguk <quot> The callstack leading of "free" containing critical section is: mysqld!free my_no_flags_free Query_cache::free_cache Query_cache::resize fix_query_cache_size set_var::update sql_set_variables mysql_execute_command mysql_parse <quot> It means that this code was supposed to be executed exactly by Query_cache::resize(), where you disable it. Question: if this code is not supposed to be executed by ::resize(), is it needed at all? 2. In Query_cache::resize() I can see block-level locks are acquired with the following comment: <quot> Wait for all readers and writers to exit. When the list of all queries is iterated over with a block level lock, we are done. </quot> Isn't it needed in ::free_cache() also? Regards, Sergey On Sat, Feb 08, 2014 at 01:25:11PM +0000, sanja@askmonty.org wrote:
At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-5607/
------------------------------------------------------------ revno: 4056 revision-id: sanja@askmonty.org-20140208132448-qdkkpdncnya0tr6j parent: elenst@wheezy-64.home-20140205102537-7ern5gfca6a6ojg3 committer: sanja@askmonty.org branch nick: work-maria-5.5-MDEV-5607 timestamp: Sat 2014-02-08 15:24:48 +0200 message: MDEV-5607: Query cache destroys uninitialized rwlock
- Resize destroyed rw_lock twice, now it is prevented. - Trash destroyed rw lock.
=== modified file 'include/my_valgrind.h' --- a/include/my_valgrind.h 2012-03-22 11:31:09 +0000 +++ b/include/my_valgrind.h 2014-02-08 13:24:48 +0000 @@ -13,7 +13,8 @@ along with this program; if not, write to the Free Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
- +#ifndef my_valgrind_h +#define my_valgrind_h /* Some defines to make it easier to use valgrind */ #include <m_string.h> /* bfill */
@@ -45,3 +46,4 @@ #define TRASH_FREE(A,B) TRASH_FILL(A,B,0x8F) #define TRASH(A,B) TRASH_FREE(A,B)
+#endif //my_valgrind_h
=== modified file 'include/mysql/psi/mysql_thread.h' --- a/include/mysql/psi/mysql_thread.h 2013-01-11 00:03:43 +0000 +++ b/include/mysql/psi/mysql_thread.h 2014-02-08 13:24:48 +0000 @@ -16,6 +16,8 @@ #ifndef MYSQL_THREAD_H #define MYSQL_THREAD_H
+#include <my_valgrind.h> + /** @file mysql/psi/mysql_thread.h Instrumentation helpers for mysys threads, mutexes, @@ -714,6 +716,7 @@ static inline int inline_mysql_prlock_in static inline int inline_mysql_rwlock_destroy( mysql_rwlock_t *that) { + int res; #ifdef HAVE_PSI_INTERFACE if (likely(PSI_server && that->m_psi)) { @@ -721,7 +724,9 @@ static inline int inline_mysql_rwlock_de that->m_psi= NULL; } #endif - return rwlock_destroy(&that->m_rwlock); + res= rwlock_destroy(&that->m_rwlock); + TRASH(that, sizeof(*that)); + return res; }
#ifndef DISABLE_MYSQL_PRLOCK_H
=== modified file 'mysql-test/r/query_cache.result' --- a/mysql-test/r/query_cache.result 2013-10-16 13:07:25 +0000 +++ b/mysql-test/r/query_cache.result 2014-02-08 13:24:48 +0000 @@ -2077,6 +2077,21 @@ drop procedure p1; drop table t1; set GLOBAL query_cache_size=1355776; SET GLOBAL userstat=default; +# +#MDEV-5607 Query cache destroys uninitialized rwlock +# +SET @global_query_cache_size = @@global.query_cache_size; +SET @global_query_cache_type = @@global.query_cache_type; +SET GLOBAL query_cache_type = default; +SET GLOBAL query_cache_size = default; +SET GLOBAL query_cache_type = ON; +SET GLOBAL query_cache_size = 131072; +CREATE TABLE t1(a INT); +SELECT * FROM t1; +a +SET GLOBAL query_cache_size = @global_query_cache_size; +SET GLOBAL query_cache_type = @global_query_cache_type; +DROP TABLE t1; End of 5.5 tests restore defaults SET GLOBAL query_cache_type= default;
=== modified file 'mysql-test/t/query_cache.test' --- a/mysql-test/t/query_cache.test 2013-10-16 13:07:25 +0000 +++ b/mysql-test/t/query_cache.test 2014-02-08 13:24:48 +0000 @@ -1708,6 +1708,23 @@ drop table t1; set GLOBAL query_cache_size=1355776; SET GLOBAL userstat=default;
+--echo # +--echo #MDEV-5607 Query cache destroys uninitialized rwlock +--echo # +SET @global_query_cache_size = @@global.query_cache_size; +SET @global_query_cache_type = @@global.query_cache_type; +SET GLOBAL query_cache_type = default; +SET GLOBAL query_cache_size = default; +SET GLOBAL query_cache_type = ON; +SET GLOBAL query_cache_size = 131072; + +CREATE TABLE t1(a INT); +SELECT * FROM t1; + +SET GLOBAL query_cache_size = @global_query_cache_size; +SET GLOBAL query_cache_type = @global_query_cache_type; +DROP TABLE t1; + --echo End of 5.5 tests
--echo restore defaults
=== modified file 'sql/sql_cache.cc' --- a/sql/sql_cache.cc 2013-11-19 12:16:25 +0000 +++ b/sql/sql_cache.cc 2014-02-08 13:24:48 +0000 @@ -1326,6 +1326,7 @@ ulong Query_cache::resize(ulong query_ca query->unlock_n_destroy(); block= block->next; } while (block != queries_blocks); + queries_blocks= NULL; // avoid second destroying by free_cache } free_cache();
=== modified file 'storage/perfschema/ha_perfschema.cc' --- a/storage/perfschema/ha_perfschema.cc 2012-03-27 23:04:46 +0000 +++ b/storage/perfschema/ha_perfschema.cc 2014-02-08 13:24:48 +0000 @@ -19,9 +19,9 @@ */
#include "my_global.h" +#include "sql_plugin.h" #include "my_pthread.h" #include "my_atomic.h" -#include "sql_plugin.h" #include "mysql/plugin.h" #include "ha_perfschema.h" #include "pfs_engine_table.h"
_______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
Hi, Sergey! 08.02.2014 18:50, Sergey Vojtovich пишет:
Hi Sanja,
JFYI: while reporting this bug I came up with two questions that I couldn't answer quickly. Probably they make sense, if not just ignore them:
1. This code was introduced with https://bugs.launchpad.net/maria/+bug/782223, revision: wlad@montyprogram.com-20110514163720-rmico2qegtptjguk <quot> The callstack leading of "free" containing critical section is: mysqld!free my_no_flags_free Query_cache::free_cache Query_cache::resize fix_query_cache_size set_var::update sql_set_variables mysql_execute_command mysql_parse <quot>
It means that this code was supposed to be executed exactly by Query_cache::resize(), where you disable it.
Question: if this code is not supposed to be executed by ::resize(), is it needed at all?
It makes some other things so probably it is needed (and as can see stack it was more about whole cache, but of course I can't revise head of Wlad :)
2. In Query_cache::resize() I can see block-level locks are acquired with the following comment: <quot> Wait for all readers and writers to exit. When the list of all queries is iterated over with a block level lock, we are done. </quot>
Isn't it needed in ::free_cache() also?
Free cache used in 1) init (it definitely do not need) 2) unlock with check that there is no requests (when switching cache off) 3) destruction here (as I think) all clients should be disconnected already because of server shutdown. 4) resize (but here you see that it was locked and unlocked [skip]
participants (2)
-
Oleksandr Byelkin
-
Sergey Vojtovich