From f748b69134c62cfd1767357b7f4adeb7e990ee2e Mon Sep 17 00:00:00 2001 From: unknown Date: Wed, 2 Aug 2006 22:18:49 -0700 Subject: Bug#8153 (Stored procedure with subquery and continue handler, wrong result) Before this fix, - a runtime error in a statement in a stored procedure with no error handlers was properly detected (as expected) - a runtime error in a statement with an error handler inherited from a non local runtime context (i.e., proc a with a handler, calling proc b) was properly detected (as expected) - a runtime error in a statement with a *local* error handler was executed as follows : a) the statement would succeed, regardless of the error condition, (bug) b) the error handler would be called (as expected). The root cause is that functions like my_messqge_sql would "forget" to set the thread flag thd->net.report_error to 1, because of the check involving sp_rcontext::found_handler_here(). Failure to set this flag would cause, later in the call stack, in Item_func::fix_fields() at line 190, the code to return FALSE and consider that executing the statement was successful. With this fix : - error handling code, that was duplicated in different places in the code, is now implemented in sp_rcontext::handle_error(), - handle_error() correctly sets thd->net.report_error when a handler is present, regardless of the handler location (local, or in the call stack). A test case, bug8153_subselect, has been written to demonstrate the change of behavior before and after the fix. Another test case, bug8153_function_a, as also been writen. This test has the same behavior before and after the fix. This test has been written to demonstrate that the previous expected result of procedure bug18787, was incorrect, since select no_such_function() should fail and therefore not produce a result. The incorrect result for bug18787 has the same root cause as Bug#8153, and the expected result has been adjusted. sql/mysqld.cc: Bug#8153, use sp_rcontext::handle_error() to handle errors. sql/sql_error.cc: Bug#8153, use sp_rcontext::handle_error() to handle errors. sql/protocol.cc: Bug#8153, use sp_rcontext::handle_error() to handle errors. sql/sp_rcontext.h: Bug#8153, created helper sp_rcontext::handle_error() to handle errors. sql/sp_rcontext.cc: Bug#8153, created helper sp_rcontext::handle_error() to handle errors. mysql-test/t/sp.test: Bug#8153, added test cases. mysql-test/r/sp.result: Bug#8153, added test cases, fixed expected result of bug18787. --- sql/mysqld.cc | 4 +--- sql/protocol.cc | 15 +++++++------- sql/sp_rcontext.cc | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ sql/sp_rcontext.h | 6 ++++++ sql/sql_error.cc | 8 +------- 5 files changed, 74 insertions(+), 18 deletions(-) (limited to 'sql') diff --git a/sql/mysqld.cc b/sql/mysqld.cc index 429bdee17d6..426edfed52f 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -2392,10 +2392,8 @@ static int my_message_sql(uint error, const char *str, myf MyFlags) if ((thd= current_thd)) { if (thd->spcont && - thd->spcont->find_handler(error, MYSQL_ERROR::WARN_LEVEL_ERROR)) + thd->spcont->handle_error(error, MYSQL_ERROR::WARN_LEVEL_ERROR, thd)) { - if (! thd->spcont->found_handler_here()) - thd->net.report_error= 1; /* Make "select" abort correctly */ DBUG_RETURN(0); } diff --git a/sql/protocol.cc b/sql/protocol.cc index f4efb8004ee..c6cacf978ea 100644 --- a/sql/protocol.cc +++ b/sql/protocol.cc @@ -70,13 +70,13 @@ void net_send_error(THD *thd, uint sql_errno, const char *err) DBUG_PRINT("info", ("sending error messages prohibited")); DBUG_VOID_RETURN; } - if (thd->spcont && thd->spcont->find_handler(sql_errno, - MYSQL_ERROR::WARN_LEVEL_ERROR)) + + if (thd->spcont && + thd->spcont->handle_error(sql_errno, MYSQL_ERROR::WARN_LEVEL_ERROR, thd)) { - if (! thd->spcont->found_handler_here()) - thd->net.report_error= 1; /* Make "select" abort correctly */ DBUG_VOID_RETURN; } + thd->query_error= 1; // needed to catch query errors during replication if (!err) { @@ -143,13 +143,12 @@ net_printf_error(THD *thd, uint errcode, ...) DBUG_VOID_RETURN; } - if (thd->spcont && thd->spcont->find_handler(errcode, - MYSQL_ERROR::WARN_LEVEL_ERROR)) + if (thd->spcont && + thd->spcont->handle_error(errcode, MYSQL_ERROR::WARN_LEVEL_ERROR, thd)) { - if (! thd->spcont->found_handler_here()) - thd->net.report_error= 1; /* Make "select" abort correctly */ DBUG_VOID_RETURN; } + thd->query_error= 1; // needed to catch query errors during replication #ifndef EMBEDDED_LIBRARY query_cache_abort(net); // Safety diff --git a/sql/sp_rcontext.cc b/sql/sp_rcontext.cc index 3bc27a029d0..67ee5459bb4 100644 --- a/sql/sp_rcontext.cc +++ b/sql/sp_rcontext.cc @@ -260,6 +260,65 @@ sp_rcontext::find_handler(uint sql_errno, return TRUE; } +/* + Handle the error for a given errno. + The severity of the error is adjusted depending of the current sql_mode. + If an handler is present for the error (see find_handler()), + this function will return true. + If a handler is found and if the severity of the error indicate + that the current instruction executed should abort, + the flag thd->net.report_error is also set. + This will cause the execution of the current instruction in a + sp_instr* to fail, and give control to the handler code itself + in the sp_head::execute() loop. + + SYNOPSIS + sql_errno The error code + level Warning level + thd The current thread + - thd->net.report_error is an optional output. + + RETURN + TRUE if a handler was found. + FALSE if no handler was found. +*/ +bool +sp_rcontext::handle_error(uint sql_errno, + MYSQL_ERROR::enum_warning_level level, + THD *thd) +{ + bool handled= FALSE; + MYSQL_ERROR::enum_warning_level elevated_level= level; + + + /* Depending on the sql_mode of execution, + warnings may be considered errors */ + if ((level == MYSQL_ERROR::WARN_LEVEL_WARN) && + thd->really_abort_on_warning()) + { + elevated_level= MYSQL_ERROR::WARN_LEVEL_ERROR; + } + + if (find_handler(sql_errno, elevated_level)) + { + if (elevated_level == MYSQL_ERROR::WARN_LEVEL_ERROR) + { + /* + Forces to abort the current instruction execution. + NOTE: This code is altering the original meaning of + the net.report_error flag (send an error to the client). + In the context of stored procedures with error handlers, + the flag is reused to cause error propagation, + until the error handler is reached. + No messages will be sent to the client in that context. + */ + thd->net.report_error= 1; + } + handled= TRUE; + } + + return handled; +} void sp_rcontext::push_cursor(sp_lex_keeper *lex_keeper, sp_instr_cpush *i) diff --git a/sql/sp_rcontext.h b/sql/sp_rcontext.h index 30521f6da84..5e03aa60d23 100644 --- a/sql/sp_rcontext.h +++ b/sql/sp_rcontext.h @@ -128,6 +128,12 @@ class sp_rcontext : public Sql_alloc bool find_handler(uint sql_errno,MYSQL_ERROR::enum_warning_level level); + // If there is an error handler for this error, handle it and return TRUE. + bool + handle_error(uint sql_errno, + MYSQL_ERROR::enum_warning_level level, + THD *thd); + // Returns handler type and sets *ip to location if one was found inline int found_handler(uint *ip, uint *fp) diff --git a/sql/sql_error.cc b/sql/sql_error.cc index 19811efbb12..ebd515bd209 100644 --- a/sql/sql_error.cc +++ b/sql/sql_error.cc @@ -139,14 +139,8 @@ MYSQL_ERROR *push_warning(THD *thd, MYSQL_ERROR::enum_warning_level level, } if (thd->spcont && - thd->spcont->find_handler(code, - ((int) level >= - (int) MYSQL_ERROR::WARN_LEVEL_WARN && - thd->really_abort_on_warning()) ? - MYSQL_ERROR::WARN_LEVEL_ERROR : level)) + thd->spcont->handle_error(code, level, thd)) { - if (! thd->spcont->found_handler_here()) - thd->net.report_error= 1; /* Make "select" abort correctly */ DBUG_RETURN(NULL); } query_cache_abort(&thd->net); -- cgit v1.2.1 From dec9116f4eec68cc15c4e543d1ad372762d20653 Mon Sep 17 00:00:00 2001 From: unknown Date: Thu, 17 Aug 2006 16:08:51 -0700 Subject: WL#3432 (Compile the Parser with a --debug --verbose option) Changed the automake build process : - ./configure.in - ./sql/Makefile.am to compile an instrumented parser for debug=yes or debug=full builds Changed the (primary) runtime invocation of the parser : - sql/sql_parse.cc to generate bison traces in stderr when the DBUG "parser_debug" flag is set. configure.in: WL#3432 (Compile the Parser with a --debug --verbose option) New Automake condition : MYSQL_CONF_DEBUG sql/Makefile.am: WL#3432 (Compile the Parser with a --debug --verbose option) In Debug mode, compile sql_yacc.yy with --debug --verbose sql/sql_parse.cc: WL#3432 (Compile the Parser with a --debug --verbose option) Conditionally turn the bison parser debug on at runtime. --- sql/Makefile.am | 7 ++++++- sql/sql_parse.cc | 23 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) (limited to 'sql') diff --git a/sql/Makefile.am b/sql/Makefile.am index 8428d6401b5..251fbaa1c9a 100644 --- a/sql/Makefile.am +++ b/sql/Makefile.am @@ -117,9 +117,14 @@ DEFS = -DMYSQL_SERVER \ BUILT_SOURCES = sql_yacc.cc sql_yacc.h lex_hash.h EXTRA_DIST = $(BUILT_SOURCES) -DISTCLEANFILES = lex_hash.h +DISTCLEANFILES = lex_hash.h sql_yacc.output + AM_YFLAGS = -d +if MYSQL_CONF_DEBUG +AM_YFLAGS += --debug --verbose +endif + mysql_tzinfo_to_sql.cc: rm -f mysql_tzinfo_to_sql.cc @LN_CP_F@ $(srcdir)/tztime.cc mysql_tzinfo_to_sql.cc diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 72098fb83e4..ceda7507d23 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -5776,6 +5776,26 @@ void mysql_init_multi_delete(LEX *lex) lex->query_tables_last= &lex->query_tables; } +#ifndef DBUG_OFF +static void turn_parser_debug_on() +{ + /* + MYSQLdebug is in sql/sql_yacc.cc, in bison generated code. + Turning this option on is **VERY** verbose, and should be + used when investigating a syntax error problem only. + + The syntax to run with bison traces is as follows : + - Starting a server manually : + mysqld --debug="d,parser_debug" ... + - Running a test : + mysql-test-run.pl --mysqld="--debug=d,parser_debug" ... + + The result will be in the process stderr (var/log/master.err) + */ + extern int MYSQLdebug; + MYSQLdebug= 1; +} +#endif /* When you modify mysql_parse(), you may need to mofify @@ -5785,6 +5805,9 @@ void mysql_init_multi_delete(LEX *lex) void mysql_parse(THD *thd, char *inBuf, uint length) { DBUG_ENTER("mysql_parse"); + + DBUG_EXECUTE_IF("parser_debug", turn_parser_debug_on();); + mysql_init_query(thd, (uchar*) inBuf, length); if (query_cache_send_result_to_client(thd, inBuf, length) <= 0) { -- cgit v1.2.1 From cd2c9eeccb9f7165c4ba07a3172b70076c46ac74 Mon Sep 17 00:00:00 2001 From: unknown Date: Fri, 18 Aug 2006 19:16:07 -0700 Subject: WL#3432 (Compile the Parser with a --debug --verbose option) Corrected build issues : the build can not be conditional. to keep a unique source .tar.gz distribution. configure.in: Rolling back previous change sql/Makefile.am: Partially rolling back previous change. The build has to be unconditional, for the source .tar.gz distribution sql/mysql_priv.h: WL#3432 (Compile the Parser with a --debug --verbose option) sql/sql_parse.cc: WL#3432 (Compile the Parser with a --debug --verbose option) Moved turn_parser_debug_on to sql_yacc.yy sql/sql_yacc.yy: WL#3432 (Compile the Parser with a --debug --verbose option) Moved turn_parser_debug_on to sql_yacc.yy --- sql/Makefile.am | 6 +----- sql/mysql_priv.h | 3 +++ sql/sql_parse.cc | 21 --------------------- sql/sql_yacc.yy | 28 ++++++++++++++++++++++++++++ 4 files changed, 32 insertions(+), 26 deletions(-) (limited to 'sql') diff --git a/sql/Makefile.am b/sql/Makefile.am index 251fbaa1c9a..ceec9757889 100644 --- a/sql/Makefile.am +++ b/sql/Makefile.am @@ -119,11 +119,7 @@ BUILT_SOURCES = sql_yacc.cc sql_yacc.h lex_hash.h EXTRA_DIST = $(BUILT_SOURCES) DISTCLEANFILES = lex_hash.h sql_yacc.output -AM_YFLAGS = -d - -if MYSQL_CONF_DEBUG -AM_YFLAGS += --debug --verbose -endif +AM_YFLAGS = -d --debug --verbose mysql_tzinfo_to_sql.cc: rm -f mysql_tzinfo_to_sql.cc diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h index c776fc72b16..0834af46d38 100644 --- a/sql/mysql_priv.h +++ b/sql/mysql_priv.h @@ -1501,6 +1501,9 @@ void free_list(I_List *list); /* sql_yacc.cc */ extern int MYSQLparse(void *thd); +#ifndef DBUG_OFF +extern void turn_parser_debug_on(); +#endif /* frm_crypt.cc */ #ifdef HAVE_CRYPTED_FRM diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index ceda7507d23..061c29e16c3 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -5776,27 +5776,6 @@ void mysql_init_multi_delete(LEX *lex) lex->query_tables_last= &lex->query_tables; } -#ifndef DBUG_OFF -static void turn_parser_debug_on() -{ - /* - MYSQLdebug is in sql/sql_yacc.cc, in bison generated code. - Turning this option on is **VERY** verbose, and should be - used when investigating a syntax error problem only. - - The syntax to run with bison traces is as follows : - - Starting a server manually : - mysqld --debug="d,parser_debug" ... - - Running a test : - mysql-test-run.pl --mysqld="--debug=d,parser_debug" ... - - The result will be in the process stderr (var/log/master.err) - */ - extern int MYSQLdebug; - MYSQLdebug= 1; -} -#endif - /* When you modify mysql_parse(), you may need to mofify mysql_test_parse_for_slave() in this same file. diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index ccf4a9d6687..1dbed6d3cdb 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -68,6 +68,34 @@ inline Item *is_truth_value(Item *A, bool v1, bool v2) new Item_int((char *) (v1 ? "FALSE" : "TRUE"),!v1, 1)); } +#ifndef DBUG_OFF +#define YYDEBUG 1 +#else +#define YYDEBUG 0 +#endif + +#ifndef DBUG_OFF +void turn_parser_debug_on() +{ + /* + MYSQLdebug is in sql/sql_yacc.cc, in bison generated code. + Turning this option on is **VERY** verbose, and should be + used when investigating a syntax error problem only. + + The syntax to run with bison traces is as follows : + - Starting a server manually : + mysqld --debug="d,parser_debug" ... + - Running a test : + mysql-test-run.pl --mysqld="--debug=d,parser_debug" ... + + The result will be in the process stderr (var/log/master.err) + */ + + extern int yydebug; + yydebug= 1; +} +#endif + %} %union { int num; -- cgit v1.2.1 From 877477f173419933f1d7c2b9e06dc7cfe925eb6e Mon Sep 17 00:00:00 2001 From: unknown Date: Tue, 22 Aug 2006 11:47:52 +0400 Subject: BUG#21051: RESET QUERY CACHE very slow when query_cache_type=0 There were two problems: RESET QUERY CACHE took a long time to complete and other threads were blocked during this time. The patch does three things: 1 fixes a bug with improper use of test-lock-test_again technique. AKA Double-Checked Locking is applicable here only in few places. 2 Somewhat improves performance of RESET QUERY CACHE. Do my_hash_reset() instead of deleting elements one by one. Note however that the slowdown also happens when inserting into sorted list of free blocks, should be rewritten using balanced tree. 3 Makes RESET QUERY CACHE non-blocking. The patch adjusts the locking protocol of the query cache in the following way: it introduces a flag flush_in_progress, which is set when Query_cache::flush_cache() is in progress. This call sets the flag on enter, and then releases the lock. Every other call is able to acquire the lock, but does nothing if flush_in_progress is set (as if the query cache is disabled). The only exception is the concurrent calls to Query_cache::flush_cache(), that are blocked until the flush is over. When leaving Query_cache::flush_cache(), the lock is acquired and the flag is reset, and one thread waiting on Query_cache::flush_cache() (if any) is notified that it may proceed. include/mysql_com.h: Add comment for NET::query_cache_query. sql/net_serv.cc: Use query_cache_init_query() for initialization of NET::query_cache_query if query cache is used. Do not access net->query_cache_query without a lock. sql/sql_cache.cc: Fix bug with accessing query_cache_size, Query_cache_query::wri and thd->net.query_cache_query before acquiring the lock---leave double-check locking only in safe places. Wherever we check that cache is usable (query_cache_size > 0) we now also check that flush_in_progress is false, i.e. we are not in the middle of cache flush. Add Query_cache::not_in_flush_or_wait() method and use it in Query_cache::flush_cache(), so that threads doing cache flush will wait it to finish, while other threads will bypass the cache as if it is disabled. Extract Query_cache::free_query_internal() from Query_cache::free_query(), which does not removes elements from the hash, and use it together with my_hash_reset() in Query_cache::flush_cache(). sql/sql_cache.h: Add declarations for new members and methods. Make is_cacheable() a static method. Add query_cache_init_query() function. sql/sql_class.cc: Use query_cache_init_query() for initialization of NET::query_cache_query. --- sql/net_serv.cc | 17 +- sql/sql_cache.cc | 582 +++++++++++++++++++++++++++++++++---------------------- sql/sql_cache.h | 17 +- sql/sql_class.cc | 2 +- 4 files changed, 382 insertions(+), 236 deletions(-) (limited to 'sql') diff --git a/sql/net_serv.cc b/sql/net_serv.cc index cf9dc6e3f60..e54d3abd29d 100644 --- a/sql/net_serv.cc +++ b/sql/net_serv.cc @@ -96,8 +96,11 @@ extern uint test_flags; extern ulong bytes_sent, bytes_received, net_big_packet_count; extern pthread_mutex_t LOCK_bytes_sent , LOCK_bytes_received; #ifndef MYSQL_INSTANCE_MANAGER -extern void query_cache_insert(NET *net, const char *packet, ulong length); +#ifdef HAVE_QUERY_CACHE #define USE_QUERY_CACHE +extern void query_cache_init_query(NET *net); +extern void query_cache_insert(NET *net, const char *packet, ulong length); +#endif // HAVE_QUERY_CACHE #define update_statistics(A) A #endif /* MYSQL_INSTANCE_MANGER */ #endif /* defined(MYSQL_SERVER) && !defined(MYSQL_INSTANCE_MANAGER) */ @@ -133,7 +136,11 @@ my_bool my_net_init(NET *net, Vio* vio) net->compress=0; net->reading_or_writing=0; net->where_b = net->remain_in_buf=0; net->last_errno=0; - net->query_cache_query=0; +#ifdef USE_QUERY_CACHE + query_cache_init_query(net); +#else + net->query_cache_query= 0; +#endif net->report_error= 0; if (vio != 0) /* If real connection */ @@ -552,10 +559,8 @@ net_real_write(NET *net,const char *packet,ulong len) my_bool net_blocking = vio_is_blocking(net->vio); DBUG_ENTER("net_real_write"); -#if defined(MYSQL_SERVER) && defined(HAVE_QUERY_CACHE) \ - && !defined(MYSQL_INSTANCE_MANAGER) - if (net->query_cache_query != 0) - query_cache_insert(net, packet, len); +#if defined(MYSQL_SERVER) && defined(USE_QUERY_CACHE) + query_cache_insert(net, packet, len); #endif if (net->error == 2) diff --git a/sql/sql_cache.cc b/sql/sql_cache.cc index f8f7bde3a62..ff033b69f98 100644 --- a/sql/sql_cache.cc +++ b/sql/sql_cache.cc @@ -564,22 +564,63 @@ byte *query_cache_query_get_key(const byte *record, uint *length, Functions to store things into the query cache *****************************************************************************/ +/* + Note on double-check locking (DCL) usage. + + Below, in query_cache_insert(), query_cache_abort() and + query_cache_end_of_result() we use what is called double-check + locking (DCL) for NET::query_cache_query. I.e. we test it first + without a lock, and, if positive, test again under the lock. + + This means that if we see 'NET::query_cache_query == 0' without a + lock we will skip the operation. But this is safe here: when we + started to cache a query, we called Query_cache::store_query(), and + NET::query_cache_query was set to non-zero in this thread (and the + thread always sees results of its memory operations, mutex or not). + If later we see 'NET::query_cache_query == 0' without locking a + mutex, that may only mean that some other thread have reset it by + invalidating the query. Skipping the operation in this case is the + right thing to do, as NET::query_cache_query won't get non-zero for + this query again. + + See also comments in Query_cache::store_query() and + Query_cache::send_result_to_client(). + + NOTE, however, that double-check locking is not applicable in + 'invalidate' functions, as we may erroneously skip invalidation, + because the thread doing invalidation may never see non-zero + NET::query_cache_query. +*/ + + +void query_cache_init_query(NET *net) +{ + /* + It is safe to initialize 'NET::query_cache_query' without a lock + here, because before it will be accessed from different threads it + will be set in this thread under a lock, and access from the same + thread is always safe. + */ + net->query_cache_query= 0; +} + + /* Insert the packet into the query cache. - This should only be called if net->query_cache_query != 0 */ void query_cache_insert(NET *net, const char *packet, ulong length) { DBUG_ENTER("query_cache_insert"); + /* See the comment on double-check locking usage above. */ + if (net->query_cache_query == 0) + DBUG_VOID_RETURN; + STRUCT_LOCK(&query_cache.structure_guard_mutex); - /* - It is very unlikely that following condition is TRUE (it is possible - only if other thread is resizing cache), so we check it only after guard - mutex lock - */ - if (unlikely(query_cache.query_cache_size == 0)) + + if (unlikely(query_cache.query_cache_size == 0 || + query_cache.flush_in_progress)) { STRUCT_UNLOCK(&query_cache.structure_guard_mutex); DBUG_VOID_RETURN; @@ -616,10 +657,10 @@ void query_cache_insert(NET *net, const char *packet, ulong length) header->result(result); header->last_pkt_nr= net->pkt_nr; BLOCK_UNLOCK_WR(query_block); + DBUG_EXECUTE("check_querycache",query_cache.check_integrity(0);); } else STRUCT_UNLOCK(&query_cache.structure_guard_mutex); - DBUG_EXECUTE("check_querycache",query_cache.check_integrity(0);); DBUG_VOID_RETURN; } @@ -628,33 +669,33 @@ void query_cache_abort(NET *net) { DBUG_ENTER("query_cache_abort"); - if (net->query_cache_query != 0) // Quick check on unlocked structure + /* See the comment on double-check locking usage above. */ + if (net->query_cache_query == 0) + DBUG_VOID_RETURN; + + STRUCT_LOCK(&query_cache.structure_guard_mutex); + + if (unlikely(query_cache.query_cache_size == 0 || + query_cache.flush_in_progress)) { - STRUCT_LOCK(&query_cache.structure_guard_mutex); - /* - It is very unlikely that following condition is TRUE (it is possible - only if other thread is resizing cache), so we check it only after guard - mutex lock - */ - if (unlikely(query_cache.query_cache_size == 0)) - { - STRUCT_UNLOCK(&query_cache.structure_guard_mutex); - DBUG_VOID_RETURN; - } + STRUCT_UNLOCK(&query_cache.structure_guard_mutex); + DBUG_VOID_RETURN; + } - Query_cache_block *query_block = ((Query_cache_block*) - net->query_cache_query); - if (query_block) // Test if changed by other thread - { - DUMP(&query_cache); - BLOCK_LOCK_WR(query_block); - // The following call will remove the lock on query_block - query_cache.free_query(query_block); - } - net->query_cache_query=0; + Query_cache_block *query_block= ((Query_cache_block*) + net->query_cache_query); + if (query_block) // Test if changed by other thread + { + DUMP(&query_cache); + BLOCK_LOCK_WR(query_block); + // The following call will remove the lock on query_block + query_cache.free_query(query_block); + net->query_cache_query= 0; DBUG_EXECUTE("check_querycache",query_cache.check_integrity(1);); - STRUCT_UNLOCK(&query_cache.structure_guard_mutex); } + + STRUCT_UNLOCK(&query_cache.structure_guard_mutex); + DBUG_VOID_RETURN; } @@ -663,60 +704,65 @@ void query_cache_end_of_result(THD *thd) { DBUG_ENTER("query_cache_end_of_result"); - if (thd->net.query_cache_query != 0) // Quick check on unlocked structure - { + /* See the comment on double-check locking usage above. */ + if (thd->net.query_cache_query == 0) + DBUG_VOID_RETURN; + #ifdef EMBEDDED_LIBRARY - query_cache_insert(&thd->net, (char*)thd, - emb_count_querycache_size(thd)); + query_cache_insert(&thd->net, (char*)thd, + emb_count_querycache_size(thd)); #endif - STRUCT_LOCK(&query_cache.structure_guard_mutex); - /* - It is very unlikely that following condition is TRUE (it is possible - only if other thread is resizing cache), so we check it only after guard - mutex lock - */ - if (unlikely(query_cache.query_cache_size == 0)) - { - STRUCT_UNLOCK(&query_cache.structure_guard_mutex); - DBUG_VOID_RETURN; - } - Query_cache_block *query_block = ((Query_cache_block*) - thd->net.query_cache_query); - if (query_block) - { - DUMP(&query_cache); - BLOCK_LOCK_WR(query_block); - Query_cache_query *header = query_block->query(); - Query_cache_block *last_result_block = header->result()->prev; - ulong allign_size = ALIGN_SIZE(last_result_block->used); - ulong len = max(query_cache.min_allocation_unit, allign_size); - if (last_result_block->length >= query_cache.min_allocation_unit + len) - query_cache.split_block(last_result_block,len); - STRUCT_UNLOCK(&query_cache.structure_guard_mutex); + STRUCT_LOCK(&query_cache.structure_guard_mutex); + + if (unlikely(query_cache.query_cache_size == 0 || + query_cache.flush_in_progress)) + { + STRUCT_UNLOCK(&query_cache.structure_guard_mutex); + DBUG_VOID_RETURN; + } + + Query_cache_block *query_block= ((Query_cache_block*) + thd->net.query_cache_query); + if (query_block) + { + DUMP(&query_cache); + BLOCK_LOCK_WR(query_block); + Query_cache_query *header= query_block->query(); + Query_cache_block *last_result_block= header->result()->prev; + ulong allign_size= ALIGN_SIZE(last_result_block->used); + ulong len= max(query_cache.min_allocation_unit, allign_size); + if (last_result_block->length >= query_cache.min_allocation_unit + len) + query_cache.split_block(last_result_block,len); #ifndef DBUG_OFF - if (header->result() == 0) - { - DBUG_PRINT("error", ("end of data whith no result. query '%s'", - header->query())); - query_cache.wreck(__LINE__, ""); - DBUG_VOID_RETURN; - } -#endif - header->found_rows(current_thd->limit_found_rows); - header->result()->type = Query_cache_block::RESULT; - header->writer(0); - BLOCK_UNLOCK_WR(query_block); - } - else + if (header->result() == 0) { - // Cache was flushed or resized and query was deleted => do nothing + DBUG_PRINT("error", ("end of data whith no result. query '%s'", + header->query())); + query_cache.wreck(__LINE__, ""); + STRUCT_UNLOCK(&query_cache.structure_guard_mutex); + + DBUG_VOID_RETURN; } - thd->net.query_cache_query=0; - DBUG_EXECUTE("check_querycache",query_cache.check_integrity(0);); +#endif + header->found_rows(current_thd->limit_found_rows); + header->result()->type= Query_cache_block::RESULT; + header->writer(0); + thd->net.query_cache_query= 0; + DBUG_EXECUTE("check_querycache",query_cache.check_integrity(1);); + + STRUCT_UNLOCK(&query_cache.structure_guard_mutex); + + BLOCK_UNLOCK_WR(query_block); + } + else + { + // Cache was flushed or resized and query was deleted => do nothing + STRUCT_UNLOCK(&query_cache.structure_guard_mutex); } + DBUG_VOID_RETURN; } @@ -762,8 +808,7 @@ ulong Query_cache::resize(ulong query_cache_size_arg) query_cache_size_arg)); DBUG_ASSERT(initialized); STRUCT_LOCK(&structure_guard_mutex); - if (query_cache_size > 0) - free_cache(); + free_cache(); query_cache_size= query_cache_size_arg; ::query_cache_size= init_cache(); STRUCT_UNLOCK(&structure_guard_mutex); @@ -784,7 +829,15 @@ void Query_cache::store_query(THD *thd, TABLE_LIST *tables_used) TABLE_COUNTER_TYPE local_tables; ulong tot_length; DBUG_ENTER("Query_cache::store_query"); - if (query_cache_size == 0 || thd->locked_tables) + /* + Testing 'query_cache_size' without a lock here is safe: the thing + we may loose is that the query won't be cached, but we save on + mutex locking in the case when query cache is disabled or the + query is uncachable. + + See also a note on double-check locking usage above. + */ + if (thd->locked_tables || query_cache_size == 0) DBUG_VOID_RETURN; uint8 tables_type= 0; @@ -836,9 +889,9 @@ sql mode: 0x%lx, sort len: %lu, conncat len: %lu", acquiring the query cache mutex. */ ha_release_temporary_latches(thd); - STRUCT_LOCK(&structure_guard_mutex); - if (query_cache_size == 0) + STRUCT_LOCK(&structure_guard_mutex); + if (query_cache_size == 0 || flush_in_progress) { STRUCT_UNLOCK(&structure_guard_mutex); DBUG_VOID_RETURN; @@ -912,11 +965,12 @@ sql mode: 0x%lx, sort len: %lu, conncat len: %lu", double_linked_list_simple_include(query_block, &queries_blocks); inserts++; queries_in_cache++; - STRUCT_UNLOCK(&structure_guard_mutex); - net->query_cache_query= (gptr) query_block; header->writer(net); header->tables_type(tables_type); + + STRUCT_UNLOCK(&structure_guard_mutex); + // init_n_lock make query block locked BLOCK_UNLOCK_WR(query_block); } @@ -970,12 +1024,16 @@ Query_cache::send_result_to_client(THD *thd, char *sql, uint query_length) Query_cache_query_flags flags; DBUG_ENTER("Query_cache::send_result_to_client"); - if (query_cache_size == 0 || thd->locked_tables || - thd->variables.query_cache_type == 0) - goto err; + /* + Testing 'query_cache_size' without a lock here is safe: the thing + we may loose is that the query won't be served from cache, but we + save on mutex locking in the case when query cache is disabled. - /* Check that we haven't forgot to reset the query cache variables */ - DBUG_ASSERT(thd->net.query_cache_query == 0); + See also a note on double-check locking usage above. + */ + if (thd->locked_tables || thd->variables.query_cache_type == 0 || + query_cache_size == 0) + goto err; if (!thd->lex->safe_to_cache_query) { @@ -1011,11 +1069,15 @@ Query_cache::send_result_to_client(THD *thd, char *sql, uint query_length) } STRUCT_LOCK(&structure_guard_mutex); - if (query_cache_size == 0) + if (query_cache_size == 0 || flush_in_progress) { DBUG_PRINT("qcache", ("query cache disabled")); goto err_unlock; } + + /* Check that we haven't forgot to reset the query cache variables */ + DBUG_ASSERT(thd->net.query_cache_query == 0); + Query_cache_block *query_block; tot_length= query_length + thd->db_length + 1 + QUERY_CACHE_FLAGS_SIZE; @@ -1241,45 +1303,43 @@ void Query_cache::invalidate(THD *thd, TABLE_LIST *tables_used, my_bool using_transactions) { DBUG_ENTER("Query_cache::invalidate (table list)"); - if (query_cache_size > 0) + STRUCT_LOCK(&structure_guard_mutex); + if (query_cache_size > 0 && !flush_in_progress) { - STRUCT_LOCK(&structure_guard_mutex); - if (query_cache_size > 0) - { - DUMP(this); + DUMP(this); - using_transactions = using_transactions && - (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)); - for (; tables_used; tables_used= tables_used->next_local) - { - DBUG_ASSERT(!using_transactions || tables_used->table!=0); - if (tables_used->derived) - continue; - if (using_transactions && - (tables_used->table->file->table_cache_type() == - HA_CACHE_TBL_TRANSACT)) - /* - Tables_used->table can't be 0 in transaction. - Only 'drop' invalidate not opened table, but 'drop' - force transaction finish. - */ - thd->add_changed_table(tables_used->table); - else - invalidate_table(tables_used); - } + using_transactions= using_transactions && + (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)); + for (; tables_used; tables_used= tables_used->next_local) + { + DBUG_ASSERT(!using_transactions || tables_used->table!=0); + if (tables_used->derived) + continue; + if (using_transactions && + (tables_used->table->file->table_cache_type() == + HA_CACHE_TBL_TRANSACT)) + /* + Tables_used->table can't be 0 in transaction. + Only 'drop' invalidate not opened table, but 'drop' + force transaction finish. + */ + thd->add_changed_table(tables_used->table); + else + invalidate_table(tables_used); } - STRUCT_UNLOCK(&structure_guard_mutex); } + STRUCT_UNLOCK(&structure_guard_mutex); + DBUG_VOID_RETURN; } void Query_cache::invalidate(CHANGED_TABLE_LIST *tables_used) { DBUG_ENTER("Query_cache::invalidate (changed table list)"); - if (query_cache_size > 0 && tables_used) + if (tables_used) { STRUCT_LOCK(&structure_guard_mutex); - if (query_cache_size > 0) + if (query_cache_size > 0 && !flush_in_progress) { DUMP(this); for (; tables_used; tables_used= tables_used->next) @@ -1309,10 +1369,10 @@ void Query_cache::invalidate(CHANGED_TABLE_LIST *tables_used) void Query_cache::invalidate_locked_for_write(TABLE_LIST *tables_used) { DBUG_ENTER("Query_cache::invalidate_locked_for_write"); - if (query_cache_size > 0 && tables_used) + if (tables_used) { STRUCT_LOCK(&structure_guard_mutex); - if (query_cache_size > 0) + if (query_cache_size > 0 && !flush_in_progress) { DUMP(this); for (; tables_used; tables_used= tables_used->next_local) @@ -1336,21 +1396,19 @@ void Query_cache::invalidate(THD *thd, TABLE *table, { DBUG_ENTER("Query_cache::invalidate (table)"); - if (query_cache_size > 0) + STRUCT_LOCK(&structure_guard_mutex); + if (query_cache_size > 0 && !flush_in_progress) { - STRUCT_LOCK(&structure_guard_mutex); - if (query_cache_size > 0) - { - using_transactions = using_transactions && - (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)); - if (using_transactions && - (table->file->table_cache_type() == HA_CACHE_TBL_TRANSACT)) - thd->add_changed_table(table); - else - invalidate_table(table); - } - STRUCT_UNLOCK(&structure_guard_mutex); + using_transactions= using_transactions && + (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)); + if (using_transactions && + (table->file->table_cache_type() == HA_CACHE_TBL_TRANSACT)) + thd->add_changed_table(table); + else + invalidate_table(table); } + STRUCT_UNLOCK(&structure_guard_mutex); + DBUG_VOID_RETURN; } @@ -1359,20 +1417,18 @@ void Query_cache::invalidate(THD *thd, const char *key, uint32 key_length, { DBUG_ENTER("Query_cache::invalidate (key)"); - if (query_cache_size > 0) + STRUCT_LOCK(&structure_guard_mutex); + if (query_cache_size > 0 && !flush_in_progress) { - using_transactions = using_transactions && + using_transactions= using_transactions && (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)); if (using_transactions) // used for innodb => has_transactions() is TRUE thd->add_changed_table(key, key_length); else - { - STRUCT_LOCK(&structure_guard_mutex); - if (query_cache_size > 0) - invalidate_table((byte*)key, key_length); - STRUCT_UNLOCK(&structure_guard_mutex); - } + invalidate_table((byte*)key, key_length); } + STRUCT_UNLOCK(&structure_guard_mutex); + DBUG_VOID_RETURN; } @@ -1383,38 +1439,36 @@ void Query_cache::invalidate(THD *thd, const char *key, uint32 key_length, void Query_cache::invalidate(char *db) { DBUG_ENTER("Query_cache::invalidate (db)"); - if (query_cache_size > 0) + STRUCT_LOCK(&structure_guard_mutex); + if (query_cache_size > 0 && !flush_in_progress) { - STRUCT_LOCK(&structure_guard_mutex); - if (query_cache_size > 0) - { - DUMP(this); + DUMP(this); restart_search: - if (tables_blocks) + if (tables_blocks) + { + Query_cache_block *curr= tables_blocks; + Query_cache_block *next; + do { - Query_cache_block *curr= tables_blocks; - Query_cache_block *next; - do - { - next= curr->next; - if (strcmp(db, (char*)(curr->table()->db())) == 0) - invalidate_table(curr); - /* - invalidate_table can freed block on which point 'next' (if - table of this block used only in queries which was deleted - by invalidate_table). As far as we do not allocate new blocks - and mark all headers of freed blocks as 'FREE' (even if they are - merged with other blocks) we can just test type of block - to be sure that block is not deleted - */ - if (next->type == Query_cache_block::FREE) - goto restart_search; - curr= next; - } while (curr != tables_blocks); - } + next= curr->next; + if (strcmp(db, (char*)(curr->table()->db())) == 0) + invalidate_table(curr); + /* + invalidate_table can freed block on which point 'next' (if + table of this block used only in queries which was deleted + by invalidate_table). As far as we do not allocate new blocks + and mark all headers of freed blocks as 'FREE' (even if they are + merged with other blocks) we can just test type of block + to be sure that block is not deleted + */ + if (next->type == Query_cache_block::FREE) + goto restart_search; + curr= next; + } while (curr != tables_blocks); } - STRUCT_UNLOCK(&structure_guard_mutex); } + STRUCT_UNLOCK(&structure_guard_mutex); + DBUG_VOID_RETURN; } @@ -1422,23 +1476,22 @@ void Query_cache::invalidate(char *db) void Query_cache::invalidate_by_MyISAM_filename(const char *filename) { DBUG_ENTER("Query_cache::invalidate_by_MyISAM_filename"); - if (query_cache_size > 0) + + STRUCT_LOCK(&structure_guard_mutex); + if (query_cache_size > 0 && !flush_in_progress) { /* Calculate the key outside the lock to make the lock shorter */ char key[MAX_DBKEY_LENGTH]; uint32 db_length; uint key_length= filename_2_table_key(key, filename, &db_length); - STRUCT_LOCK(&structure_guard_mutex); - if (query_cache_size > 0) // Safety if cache removed - { - Query_cache_block *table_block; - if ((table_block = (Query_cache_block*) hash_search(&tables, - (byte*) key, - key_length))) - invalidate_table(table_block); - } - STRUCT_UNLOCK(&structure_guard_mutex); + Query_cache_block *table_block; + if ((table_block = (Query_cache_block*) hash_search(&tables, + (byte*) key, + key_length))) + invalidate_table(table_block); } + STRUCT_UNLOCK(&structure_guard_mutex); + DBUG_VOID_RETURN; } @@ -1483,7 +1536,12 @@ void Query_cache::destroy() } else { + /* Underlying code expects the lock. */ + STRUCT_LOCK(&structure_guard_mutex); free_cache(); + STRUCT_UNLOCK(&structure_guard_mutex); + + pthread_cond_destroy(&COND_flush_finished); pthread_mutex_destroy(&structure_guard_mutex); initialized = 0; } @@ -1499,6 +1557,8 @@ void Query_cache::init() { DBUG_ENTER("Query_cache::init"); pthread_mutex_init(&structure_guard_mutex,MY_MUTEX_INIT_FAST); + pthread_cond_init(&COND_flush_finished, NULL); + flush_in_progress= FALSE; initialized = 1; DBUG_VOID_RETURN; } @@ -1694,6 +1754,17 @@ void Query_cache::make_disabled() } +/* + free_cache() - free all resources allocated by the cache. + + SYNOPSIS + free_cache() + + DESCRIPTION + This function frees all resources allocated by the cache. You + have to call init_cache() before using the cache again. +*/ + void Query_cache::free_cache() { DBUG_ENTER("Query_cache::free_cache"); @@ -1728,17 +1799,51 @@ void Query_cache::free_cache() Free block data *****************************************************************************/ + /* - The following assumes we have a lock on the cache + flush_cache() - flush the cache. + + SYNOPSIS + flush_cache() + + DESCRIPTION + This function will flush cache contents. It assumes we have + 'structure_guard_mutex' locked. The function sets the + flush_in_progress flag and releases the lock, so other threads may + proceed skipping the cache as if it is disabled. Concurrent + flushes are performed in turn. */ void Query_cache::flush_cache() { + /* + If there is flush in progress, wait for it to finish, and then do + our flush. This is necessary because something could be added to + the cache before we acquire the lock again, and some code (like + Query_cache::free_cache()) depends on the fact that after the + flush the cache is empty. + */ + while (flush_in_progress) + pthread_cond_wait(&COND_flush_finished, &structure_guard_mutex); + + /* + Setting 'flush_in_progress' will prevent other threads from using + the cache while we are in the middle of the flush, and we release + the lock so that other threads won't block. + */ + flush_in_progress= TRUE; + STRUCT_UNLOCK(&structure_guard_mutex); + + my_hash_reset(&queries); while (queries_blocks != 0) { BLOCK_LOCK_WR(queries_blocks); - free_query(queries_blocks); + free_query_internal(queries_blocks); } + + STRUCT_LOCK(&structure_guard_mutex); + flush_in_progress= FALSE; + pthread_cond_signal(&COND_flush_finished); } /* @@ -1784,36 +1889,48 @@ my_bool Query_cache::free_old_query() DBUG_RETURN(1); // Nothing to remove } + /* - Free query from query cache. - query_block must be locked for writing. - This function will remove (and destroy) the lock for the query. + free_query_internal() - free query from query cache. + + SYNOPSIS + free_query_internal() + query_block Query_cache_block representing the query + + DESCRIPTION + This function will remove the query from a cache, and place its + memory blocks to the list of free blocks. 'query_block' must be + locked for writing, this function will release (and destroy) this + lock. + + NOTE + 'query_block' should be removed from 'queries' hash _before_ + calling this method, as the lock will be destroyed here. */ -void Query_cache::free_query(Query_cache_block *query_block) +void Query_cache::free_query_internal(Query_cache_block *query_block) { - DBUG_ENTER("Query_cache::free_query"); + DBUG_ENTER("Query_cache::free_query_internal"); DBUG_PRINT("qcache", ("free query 0x%lx %lu bytes result", (ulong) query_block, query_block->query()->length() )); queries_in_cache--; - hash_delete(&queries,(byte *) query_block); - Query_cache_query *query = query_block->query(); + Query_cache_query *query= query_block->query(); if (query->writer() != 0) { /* Tell MySQL that this query should not be cached anymore */ - query->writer()->query_cache_query = 0; + query->writer()->query_cache_query= 0; query->writer(0); } double_linked_list_exclude(query_block, &queries_blocks); - Query_cache_block_table *table=query_block->table(0); + Query_cache_block_table *table= query_block->table(0); - for (TABLE_COUNTER_TYPE i=0; i < query_block->n_tables; i++) + for (TABLE_COUNTER_TYPE i= 0; i < query_block->n_tables; i++) unlink_table(table++); - Query_cache_block *result_block = query->result(); + Query_cache_block *result_block= query->result(); /* The following is true when query destruction was called and no results @@ -1827,11 +1944,11 @@ void Query_cache::free_query(Query_cache_block *query_block) refused++; inserts--; } - Query_cache_block *block = result_block; + Query_cache_block *block= result_block; do { - Query_cache_block *current = block; - block = block->next; + Query_cache_block *current= block; + block= block->next; free_memory_block(current); } while (block != result_block); } @@ -1848,6 +1965,32 @@ void Query_cache::free_query(Query_cache_block *query_block) DBUG_VOID_RETURN; } + +/* + free_query() - free query from query cache. + + SYNOPSIS + free_query() + query_block Query_cache_block representing the query + + DESCRIPTION + This function will remove 'query_block' from 'queries' hash, and + then call free_query_internal(), which see. +*/ + +void Query_cache::free_query(Query_cache_block *query_block) +{ + DBUG_ENTER("Query_cache::free_query"); + DBUG_PRINT("qcache", ("free query 0x%lx %lu bytes result", + (ulong) query_block, + query_block->query()->length() )); + + hash_delete(&queries,(byte *) query_block); + free_query_internal(query_block); + + DBUG_VOID_RETURN; +} + /***************************************************************************** Query data creation *****************************************************************************/ @@ -2431,12 +2574,8 @@ Query_cache::allocate_block(ulong len, my_bool not_less, ulong min, if (!under_guard) { STRUCT_LOCK(&structure_guard_mutex); - /* - It is very unlikely that following condition is TRUE (it is possible - only if other thread is resizing cache), so we check it only after - guard mutex lock - */ - if (unlikely(query_cache.query_cache_size == 0)) + + if (unlikely(query_cache.query_cache_size == 0 || flush_in_progress)) { STRUCT_UNLOCK(&structure_guard_mutex); DBUG_RETURN(0); @@ -2892,11 +3031,9 @@ static TABLE_COUNTER_TYPE process_and_count_tables(TABLE_LIST *tables_used, (query without tables are not cached) */ -TABLE_COUNTER_TYPE Query_cache::is_cacheable(THD *thd, uint32 query_len, - char *query, - LEX *lex, - TABLE_LIST *tables_used, - uint8 *tables_type) +TABLE_COUNTER_TYPE +Query_cache::is_cacheable(THD *thd, uint32 query_len, char *query, LEX *lex, + TABLE_LIST *tables_used, uint8 *tables_type) { TABLE_COUNTER_TYPE table_count; DBUG_ENTER("Query_cache::is_cacheable"); @@ -2979,13 +3116,10 @@ my_bool Query_cache::ask_handler_allowance(THD *thd, void Query_cache::pack_cache() { DBUG_ENTER("Query_cache::pack_cache"); + STRUCT_LOCK(&structure_guard_mutex); - /* - It is very unlikely that following condition is TRUE (it is possible - only if other thread is resizing cache), so we check it only after - guard mutex lock - */ - if (unlikely(query_cache_size == 0)) + + if (unlikely(query_cache_size == 0 || flush_in_progress)) { STRUCT_UNLOCK(&structure_guard_mutex); DBUG_VOID_RETURN; @@ -3300,7 +3434,7 @@ my_bool Query_cache::join_results(ulong join_limit) DBUG_ENTER("Query_cache::join_results"); STRUCT_LOCK(&structure_guard_mutex); - if (queries_blocks != 0) + if (queries_blocks != 0 && !flush_in_progress) { DBUG_ASSERT(query_cache_size > 0); Query_cache_block *block = queries_blocks; @@ -3587,31 +3721,23 @@ void Query_cache::tables_dump() } -my_bool Query_cache::check_integrity(bool not_locked) +my_bool Query_cache::check_integrity(bool locked) { my_bool result = 0; uint i; DBUG_ENTER("check_integrity"); - if (query_cache_size == 0) + if (!locked) + STRUCT_LOCK(&structure_guard_mutex); + + if (unlikely(query_cache_size == 0 || flush_in_progress)) { + if (!locked) + STRUCT_UNLOCK(&query_cache.structure_guard_mutex); + DBUG_PRINT("qcache", ("Query Cache not initialized")); DBUG_RETURN(0); } - if (!not_locked) - { - STRUCT_LOCK(&structure_guard_mutex); - /* - It is very unlikely that following condition is TRUE (it is possible - only if other thread is resizing cache), so we check it only after - guard mutex lock - */ - if (unlikely(query_cache_size == 0)) - { - STRUCT_UNLOCK(&query_cache.structure_guard_mutex); - DBUG_RETURN(0); - } - } if (hash_check(&queries)) { @@ -3856,7 +3982,7 @@ my_bool Query_cache::check_integrity(bool not_locked) } } DBUG_ASSERT(result == 0); - if (!not_locked) + if (!locked) STRUCT_UNLOCK(&structure_guard_mutex); DBUG_RETURN(result); } diff --git a/sql/sql_cache.h b/sql/sql_cache.h index 29d314d3c44..a66067159e2 100644 --- a/sql/sql_cache.h +++ b/sql/sql_cache.h @@ -195,7 +195,6 @@ extern "C" byte *query_cache_table_get_key(const byte *record, uint *length, my_bool not_used); } -void query_cache_insert(NET *thd, const char *packet, ulong length); extern "C" void query_cache_invalidate_by_MyISAM_filename(const char* filename); @@ -241,6 +240,12 @@ public: ulong free_memory, queries_in_cache, hits, inserts, refused, free_memory_blocks, total_blocks, lowmem_prunes; +private: + pthread_cond_t COND_flush_finished; + bool flush_in_progress; + + void free_query_internal(Query_cache_block *point); + protected: /* The following mutex is locked when searching or changing global @@ -249,6 +254,12 @@ protected: LOCK SEQUENCE (to prevent deadlocks): 1. structure_guard_mutex 2. query block (for operation inside query (query block/results)) + + Thread doing cache flush releases the mutex once it sets + flush_in_progress flag, so other threads may bypass the cache as + if it is disabled, not waiting for reset to finish. The exception + is other threads that were going to do cache flush---they'll wait + till the end of a flush operation. */ pthread_mutex_t structure_guard_mutex; byte *cache; // cache memory @@ -358,6 +369,7 @@ protected: If query is cacheable return number tables in query (query without tables not cached) */ + static TABLE_COUNTER_TYPE is_cacheable(THD *thd, uint32 query_len, char *query, LEX *lex, TABLE_LIST *tables_used, uint8 *tables_type); @@ -410,6 +422,7 @@ protected: void destroy(); + friend void query_cache_init_query(NET *net); friend void query_cache_insert(NET *net, const char *packet, ulong length); friend void query_cache_end_of_result(THD *thd); friend void query_cache_abort(NET *net); @@ -435,6 +448,8 @@ protected: extern Query_cache query_cache; extern TYPELIB query_cache_type_typelib; +void query_cache_init_query(NET *net); +void query_cache_insert(NET *net, const char *packet, ulong length); void query_cache_end_of_result(THD *thd); void query_cache_abort(NET *net); diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 5c8bd797e7c..b33c221c7a7 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -223,7 +223,7 @@ THD::THD() #endif client_capabilities= 0; // minimalistic client net.last_error[0]=0; // If error on boot - net.query_cache_query=0; // If error on boot + query_cache_init_query(&net); // If error on boot ull=0; system_thread= cleanup_done= abort_on_warning= no_warnings_for_error= 0; peer_port= 0; // For SHOW PROCESSLIST -- cgit v1.2.1 From 09e9b2f6cd92d7a75dfb6e46fadd9be2c326c8f5 Mon Sep 17 00:00:00 2001 From: unknown Date: Tue, 22 Aug 2006 18:58:14 -0700 Subject: Bug#8153 (Stored procedure with subquery and continue handler, wrong result) Implemented code review comments Test cleanup sql/protocol.cc: Bug#8153 (Stored procedure with subquery and continue handler, wrong result) Implemented code review comments --- sql/protocol.cc | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) (limited to 'sql') diff --git a/sql/protocol.cc b/sql/protocol.cc index c6cacf978ea..5de24ebdcb3 100644 --- a/sql/protocol.cc +++ b/sql/protocol.cc @@ -53,8 +53,18 @@ bool Protocol_prep::net_store_data(const char *from, uint length) } - /* Send a error string to client */ +/* + Send a error string to client + + Design note: + net_printf_error and net_send_error are low-level functions + that shall be used only when a new connection is being + established or at server startup. + For SIGNAL/RESIGNAL and GET DIAGNOSTICS functionality it's + critical that every error that can be intercepted is issued in one + place only, my_message_sql. +*/ void net_send_error(THD *thd, uint sql_errno, const char *err) { NET *net= &thd->net; @@ -64,6 +74,8 @@ void net_send_error(THD *thd, uint sql_errno, const char *err) err ? err : net->last_error[0] ? net->last_error : "NULL")); + DBUG_ASSERT(!thd->spcont); + if (net && net->no_send_error) { thd->clear_error(); @@ -71,12 +83,6 @@ void net_send_error(THD *thd, uint sql_errno, const char *err) DBUG_VOID_RETURN; } - if (thd->spcont && - thd->spcont->handle_error(sql_errno, MYSQL_ERROR::WARN_LEVEL_ERROR, thd)) - { - DBUG_VOID_RETURN; - } - thd->query_error= 1; // needed to catch query errors during replication if (!err) { @@ -117,6 +123,15 @@ void net_send_error(THD *thd, uint sql_errno, const char *err) Write error package and flush to client It's a little too low level, but I don't want to use another buffer for this + + Design note: + + net_printf_error and net_send_error are low-level functions + that shall be used only when a new connection is being + established or at server startup. + For SIGNAL/RESIGNAL and GET DIAGNOSTICS functionality it's + critical that every error that can be intercepted is issued in one + place only, my_message_sql. */ void @@ -136,6 +151,8 @@ net_printf_error(THD *thd, uint errcode, ...) DBUG_ENTER("net_printf_error"); DBUG_PRINT("enter",("message: %u",errcode)); + DBUG_ASSERT(!thd->spcont); + if (net && net->no_send_error) { thd->clear_error(); @@ -143,12 +160,6 @@ net_printf_error(THD *thd, uint errcode, ...) DBUG_VOID_RETURN; } - if (thd->spcont && - thd->spcont->handle_error(errcode, MYSQL_ERROR::WARN_LEVEL_ERROR, thd)) - { - DBUG_VOID_RETURN; - } - thd->query_error= 1; // needed to catch query errors during replication #ifndef EMBEDDED_LIBRARY query_cache_abort(net); // Safety -- cgit v1.2.1 From f96ee72fb07961faf1ee950fcb66c2dfa0589694 Mon Sep 17 00:00:00 2001 From: unknown Date: Wed, 23 Aug 2006 21:31:00 +0400 Subject: Fix for BUG#16899: Possible buffer overflow in handling of DEFINER-clause User name (host name) has limit on length. The server code relies on these limits when storing the names. The problem was that sometimes these limits were not checked properly, so that could lead to buffer overflow. The fix is to check length of user/host name in parser and if string is too long, throw an error. mysql-test/r/grant.result: Updated result file. mysql-test/r/sp.result: Updated result file. mysql-test/r/trigger.result: Updated result file. mysql-test/r/view.result: Updated result file. mysql-test/t/grant.test: Added test for BUG#16899. mysql-test/t/sp.test: Added test for BUG#16899. mysql-test/t/trigger.test: Added test for BUG#16899. mysql-test/t/view.test: Added test for BUG#16899. sql/mysql_priv.h: Added prototype for new function. sql/share/errmsg.txt: Added new resources. sql/sql_acl.cc: Remove outdated checks. sql/sql_parse.cc: Add a new function for checking string length. sql/sql_yacc.yy: Check length of user/host name. --- sql/mysql_priv.h | 1 + sql/share/errmsg.txt | 6 ++++++ sql/sql_acl.cc | 33 --------------------------------- sql/sql_parse.cc | 38 ++++++++++++++++++++++++++++---------- sql/sql_yacc.yy | 18 ++++++++++-------- 5 files changed, 45 insertions(+), 51 deletions(-) (limited to 'sql') diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h index 0834af46d38..3ec9dd718e8 100644 --- a/sql/mysql_priv.h +++ b/sql/mysql_priv.h @@ -557,6 +557,7 @@ void get_default_definer(THD *thd, LEX_USER *definer); LEX_USER *create_default_definer(THD *thd); LEX_USER *create_definer(THD *thd, LEX_STRING *user_name, LEX_STRING *host_name); LEX_USER *get_current_user(THD *thd, LEX_USER *user); +bool check_string_length(LEX_STRING *str, const char *err_msg, uint max_length); enum enum_mysql_completiontype { ROLLBACK_RELEASE=-2, ROLLBACK=1, ROLLBACK_AND_CHAIN=7, diff --git a/sql/share/errmsg.txt b/sql/share/errmsg.txt index 5c967ba19bd..b3cb33090ce 100644 --- a/sql/share/errmsg.txt +++ b/sql/share/errmsg.txt @@ -5623,3 +5623,9 @@ ER_NO_TRIGGERS_ON_SYSTEM_SCHEMA eng "Triggers can not be created on system tables" ER_REMOVED_SPACES eng "Leading spaces are removed from name '%s'" +ER_USERNAME + eng "user name" +ER_HOSTNAME + eng "host name" +ER_WRONG_STRING_LENGTH + eng "String '%-.70s' is too long for %s (should be no longer than %d)" diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index ae5ea210a47..0c7b8626c93 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -2900,14 +2900,6 @@ bool mysql_table_grant(THD *thd, TABLE_LIST *table_list, result= TRUE; continue; } - if (Str->host.length > HOSTNAME_LENGTH || - Str->user.length > USERNAME_LENGTH) - { - my_message(ER_GRANT_WRONG_HOST_OR_USER, ER(ER_GRANT_WRONG_HOST_OR_USER), - MYF(0)); - result= TRUE; - continue; - } /* Create user if needed */ error=replace_user_table(thd, tables[0].table, *Str, 0, revoke_grant, create_new_users, @@ -3112,15 +3104,6 @@ bool mysql_routine_grant(THD *thd, TABLE_LIST *table_list, bool is_proc, result= TRUE; continue; } - if (Str->host.length > HOSTNAME_LENGTH || - Str->user.length > USERNAME_LENGTH) - { - if (!no_error) - my_message(ER_GRANT_WRONG_HOST_OR_USER, ER(ER_GRANT_WRONG_HOST_OR_USER), - MYF(0)); - result= TRUE; - continue; - } /* Create user if needed */ error=replace_user_table(thd, tables[0].table, *Str, 0, revoke_grant, create_new_users, @@ -3246,14 +3229,6 @@ bool mysql_grant(THD *thd, const char *db, List &list, result= TRUE; continue; } - if (Str->host.length > HOSTNAME_LENGTH || - Str->user.length > USERNAME_LENGTH) - { - my_message(ER_GRANT_WRONG_HOST_OR_USER, ER(ER_GRANT_WRONG_HOST_OR_USER), - MYF(0)); - result= -1; - continue; - } if (replace_user_table(thd, tables[0].table, *Str, (!db ? rights : 0), revoke_grant, create_new_users, test(thd->variables.sql_mode & @@ -4144,14 +4119,6 @@ bool mysql_show_grants(THD *thd,LEX_USER *lex_user) DBUG_RETURN(TRUE); } - if (lex_user->host.length > HOSTNAME_LENGTH || - lex_user->user.length > USERNAME_LENGTH) - { - my_message(ER_GRANT_WRONG_HOST_OR_USER, ER(ER_GRANT_WRONG_HOST_OR_USER), - MYF(0)); - DBUG_RETURN(TRUE); - } - rw_rdlock(&LOCK_grant); VOID(pthread_mutex_lock(&acl_cache->lock)); diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 061c29e16c3..f3ac1280983 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -7533,16 +7533,34 @@ LEX_USER *create_definer(THD *thd, LEX_STRING *user_name, LEX_STRING *host_name) LEX_USER *get_current_user(THD *thd, LEX_USER *user) { - LEX_USER *curr_user; if (!user->user.str) // current_user - { - if (!(curr_user= (LEX_USER*) thd->alloc(sizeof(LEX_USER)))) - { - my_error(ER_OUTOFMEMORY, MYF(0), sizeof(LEX_USER)); - return 0; - } - get_default_definer(thd, curr_user); - return curr_user; - } + return create_default_definer(thd); + return user; } + + +/* + Check that length of a string does not exceed some limit. + + SYNOPSIS + check_string_length() + str string to be checked + err_msg error message to be displayed if the string is too long + max_length max length + + RETURN + FALSE the passed string is not longer than max_length + TRUE the passed string is longer than max_length +*/ + +bool check_string_length(LEX_STRING *str, const char *err_msg, + uint max_length) +{ + if (str->length <= max_length) + return FALSE; + + my_error(ER_WRONG_STRING_LENGTH, MYF(0), str->str, err_msg, max_length); + + return TRUE; +} diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 1dbed6d3cdb..133b6e18fee 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -7511,6 +7511,9 @@ user: $$->user = $1; $$->host.str= (char *) "%"; $$->host.length= 1; + + if (check_string_length(&$$->user, ER(ER_USERNAME), USERNAME_LENGTH)) + YYABORT; } | ident_or_text '@' ident_or_text { @@ -7518,6 +7521,11 @@ user: if (!($$=(LEX_USER*) thd->alloc(sizeof(st_lex_user)))) YYABORT; $$->user = $1; $$->host=$3; + + if (check_string_length(&$$->user, ER(ER_USERNAME), USERNAME_LENGTH) || + check_string_length(&$$->host, ER(ER_HOSTNAME), + HOSTNAME_LENGTH)) + YYABORT; } | CURRENT_USER optional_braces { @@ -8995,15 +9003,9 @@ definer: */ YYTHD->lex->definer= 0; } - | DEFINER_SYM EQ CURRENT_USER optional_braces + | DEFINER_SYM EQ user { - if (! (YYTHD->lex->definer= create_default_definer(YYTHD))) - YYABORT; - } - | DEFINER_SYM EQ ident_or_text '@' ident_or_text - { - if (!(YYTHD->lex->definer= create_definer(YYTHD, &$3, &$5))) - YYABORT; + YYTHD->lex->definer= get_current_user(YYTHD, $3); } ; -- cgit v1.2.1 From 965a3970c91dc4554a8ccbd3e358b1b2c20a0cfb Mon Sep 17 00:00:00 2001 From: unknown Date: Thu, 24 Aug 2006 15:49:12 +0400 Subject: BUG#21166: Prepared statement causes signal 11 on second execution Changes in an item tree done by optimizer weren't properly registered and went unnoticed, which resulted in preliminary freeing of used memory. mysql-test/r/ps.result: Add result for bug#21166: Prepared statement causes signal 11 on second execution. mysql-test/t/ps.test: Add test case for bug#21166: Prepared statement causes signal 11 on second execution. sql/item.cc: Move Item::transform() and Item_default_value::transform() from item.h here and use THD::change_item_tree() instead of plain assignment. Change Item_field::set_no_const_sub() to be used with Item::walk() instead of Item::transform(). sql/item.h: Move definition of Item::transform() and Item_default_value::transform() to item.cc. Change Item::set_no_const_sub() to be used with Item::walk() instead of Item::transform(). sql/item_cmpfunc.cc: Use Item::walk() to execute Item::set_no_const_sub(). Use THD::change_item_tree() instead of plain assignment. sql/item_func.cc: Add assert and comment to Item_func::traverse_cond(). sql/item_row.cc: Use THD::change_item_tree() instead of plain assignment. sql/item_strfunc.cc: Move Item_func_make_set::transform() from item_strfunc.h here and use THD::change_item_tree() instead of plain assignment. sql/item_strfunc.h: Move definition of Item_func_make_set::transform() to item_strfunc.cc. --- sql/item.cc | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++-- sql/item.h | 22 +++------------- sql/item_cmpfunc.cc | 26 ++++++++++++++++--- sql/item_func.cc | 9 +++++++ sql/item_row.cc | 12 ++++++++- sql/item_strfunc.cc | 20 +++++++++++++++ sql/item_strfunc.h | 9 +------ 7 files changed, 137 insertions(+), 33 deletions(-) (limited to 'sql') diff --git a/sql/item.cc b/sql/item.cc index 95ff5462fad..ac0658224b3 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -420,6 +420,50 @@ void Item::rename(char *new_name) } +/* + transform() - traverse item tree possibly transforming it (replacing + items) + + SYNOPSIS + transform() + transformer functor that performs transformation of a subtree + arg opaque argument passed to the functor + + DESCRIPTION + This function is designed to ease transformation of Item trees. + + Re-execution note: every such transformation is registered for + rollback by THD::change_item_tree() and is rolled back at the end + of execution by THD::rollback_item_tree_changes(). + + Therefore: + + - this function can not be used at prepared statement prepare + (in particular, in fix_fields!), as only permanent + transformation of Item trees are allowed at prepare. + + - the transformer function shall allocate new Items in execution + memory root (thd->mem_root) and not anywhere else: allocated + items will be gone in the end of execution. + + If you don't need to transform an item tree, but only traverse + it, please use Item::walk() instead. + + + RETURN + Returns pointer to the new subtree root. THD::change_item_tree() + should be called for it if transformation took place, i.e. if + pointer to newly allocated item is returned. +*/ + +Item* Item::transform(Item_transformer transformer, byte *arg) +{ + DBUG_ASSERT(!current_thd->is_stmt_prepare()); + + return (this->*transformer)(arg); +} + + Item_ident::Item_ident(Name_resolution_context *context_arg, const char *db_name_arg,const char *table_name_arg, const char *field_name_arg) @@ -3788,11 +3832,11 @@ Item *Item_field::equal_fields_propagator(byte *arg) See comments in Arg_comparator::set_compare_func() for details */ -Item *Item_field::set_no_const_sub(byte *arg) +bool Item_field::set_no_const_sub(byte *arg) { if (field->charset() != &my_charset_bin) no_const_subst=1; - return this; + return FALSE; } @@ -5294,6 +5338,30 @@ int Item_default_value::save_in_field(Field *field_arg, bool no_conversions) } +/* + This method like the walk method traverses the item tree, but at + the same time it can replace some nodes in the tree +*/ +Item *Item_default_value::transform(Item_transformer transformer, byte *args) +{ + DBUG_ASSERT(!current_thd->is_stmt_prepare()); + + Item *new_item= arg->transform(transformer, args); + if (!new_item) + return 0; + + /* + THD::change_item_tree() should be called only if the tree was + really transformed, i.e. when a new item has been created. + Otherwise we'll be allocating a lot of unnecessary memory for + change records at each execution. + */ + if (arg != new_item) + current_thd->change_item_tree(&arg, new_item); + return (this->*transformer)(args); +} + + bool Item_insert_value::eq(const Item *item, bool binary_cmp) const { return item->type() == INSERT_VALUE_ITEM && diff --git a/sql/item.h b/sql/item.h index 514c31c2d74..003f264fc7d 100644 --- a/sql/item.h +++ b/sql/item.h @@ -734,10 +734,7 @@ public: return (this->*processor)(arg); } - virtual Item* transform(Item_transformer transformer, byte *arg) - { - return (this->*transformer)(arg); - } + virtual Item* transform(Item_transformer transformer, byte *arg); virtual void traverse_cond(Cond_traverser traverser, void *arg, traverse_order order) @@ -755,7 +752,7 @@ public: virtual bool is_expensive_processor(byte *arg) { return 0; } virtual Item *equal_fields_propagator(byte * arg) { return this; } - virtual Item *set_no_const_sub(byte *arg) { return this; } + virtual bool set_no_const_sub(byte *arg) { return FALSE; } virtual Item *replace_equal_field(byte * arg) { return this; } /* @@ -1255,7 +1252,7 @@ public: } Item_equal *find_item_equal(COND_EQUAL *cond_equal); Item *equal_fields_propagator(byte *arg); - Item *set_no_const_sub(byte *arg); + bool set_no_const_sub(byte *arg); Item *replace_equal_field(byte *arg); inline uint32 max_disp_length() { return field->max_length(); } Item_field *filed_for_view_update() { return this; } @@ -2116,18 +2113,7 @@ public: (this->*processor)(args); } - /* - This method like the walk method traverses the item tree, but - at the same time it can replace some nodes in the tree - */ - Item *transform(Item_transformer transformer, byte *args) - { - Item *new_item= arg->transform(transformer, args); - if (!new_item) - return 0; - arg= new_item; - return (this->*transformer)(args); - } + Item *transform(Item_transformer transformer, byte *args); }; /* diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc index 34170124cd7..02b34a4d672 100644 --- a/sql/item_cmpfunc.cc +++ b/sql/item_cmpfunc.cc @@ -500,8 +500,8 @@ int Arg_comparator::set_compare_func(Item_bool_func2 *item, Item_result type) which would be transformed to: WHERE col= 'j' */ - (*a)->transform(&Item::set_no_const_sub, (byte*) 0); - (*b)->transform(&Item::set_no_const_sub, (byte*) 0); + (*a)->walk(&Item::set_no_const_sub, (byte*) 0); + (*b)->walk(&Item::set_no_const_sub, (byte*) 0); } break; } @@ -2753,6 +2753,8 @@ bool Item_cond::walk(Item_processor processor, byte *arg) Item *Item_cond::transform(Item_transformer transformer, byte *arg) { + DBUG_ASSERT(!current_thd->is_stmt_prepare()); + List_iterator li(list); Item *item; while ((item= li++)) @@ -2760,8 +2762,15 @@ Item *Item_cond::transform(Item_transformer transformer, byte *arg) Item *new_item= item->transform(transformer, arg); if (!new_item) return 0; + + /* + THD::change_item_tree() should be called only if the tree was + really transformed, i.e. when a new item has been created. + Otherwise we'll be allocating a lot of unnecessary memory for + change records at each execution. + */ if (new_item != item) - li.replace(new_item); + current_thd->change_item_tree(li.ref(), new_item); } return Item_func::transform(transformer, arg); } @@ -4006,6 +4015,8 @@ bool Item_equal::walk(Item_processor processor, byte *arg) Item *Item_equal::transform(Item_transformer transformer, byte *arg) { + DBUG_ASSERT(!current_thd->is_stmt_prepare()); + List_iterator it(fields); Item *item; while ((item= it++)) @@ -4013,8 +4024,15 @@ Item *Item_equal::transform(Item_transformer transformer, byte *arg) Item *new_item= item->transform(transformer, arg); if (!new_item) return 0; + + /* + THD::change_item_tree() should be called only if the tree was + really transformed, i.e. when a new item has been created. + Otherwise we'll be allocating a lot of unnecessary memory for + change records at each execution. + */ if (new_item != item) - it.replace((Item_field *) new_item); + current_thd->change_item_tree((Item **) it.ref(), new_item); } return Item_func::transform(transformer, arg); } diff --git a/sql/item_func.cc b/sql/item_func.cc index d3458103f6e..c7f2048b9dc 100644 --- a/sql/item_func.cc +++ b/sql/item_func.cc @@ -258,6 +258,8 @@ void Item_func::traverse_cond(Cond_traverser traverser, Item *Item_func::transform(Item_transformer transformer, byte *argument) { + DBUG_ASSERT(!current_thd->is_stmt_prepare()); + if (arg_count) { Item **arg,**arg_end; @@ -266,6 +268,13 @@ Item *Item_func::transform(Item_transformer transformer, byte *argument) Item *new_item= (*arg)->transform(transformer, argument); if (!new_item) return 0; + + /* + THD::change_item_tree() should be called only if the tree was + really transformed, i.e. when a new item has been created. + Otherwise we'll be allocating a lot of unnecessary memory for + change records at each execution. + */ if (*arg != new_item) current_thd->change_item_tree(arg, new_item); } diff --git a/sql/item_row.cc b/sql/item_row.cc index f5c8d511025..d7591b9865d 100644 --- a/sql/item_row.cc +++ b/sql/item_row.cc @@ -154,12 +154,22 @@ bool Item_row::walk(Item_processor processor, byte *arg) Item *Item_row::transform(Item_transformer transformer, byte *arg) { + DBUG_ASSERT(!current_thd->is_stmt_prepare()); + for (uint i= 0; i < arg_count; i++) { Item *new_item= items[i]->transform(transformer, arg); if (!new_item) return 0; - items[i]= new_item; + + /* + THD::change_item_tree() should be called only if the tree was + really transformed, i.e. when a new item has been created. + Otherwise we'll be allocating a lot of unnecessary memory for + change records at each execution. + */ + if (items[i] != new_item) + current_thd->change_item_tree(&items[i], new_item); } return (this->*transformer)(arg); } diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc index 4fc7340a3b0..eb5964c6e21 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -2051,6 +2051,26 @@ String *Item_func_make_set::val_str(String *str) } +Item *Item_func_make_set::transform(Item_transformer transformer, byte *arg) +{ + DBUG_ASSERT(!current_thd->is_stmt_prepare()); + + Item *new_item= item->transform(transformer, arg); + if (!new_item) + return 0; + + /* + THD::change_item_tree() should be called only if the tree was + really transformed, i.e. when a new item has been created. + Otherwise we'll be allocating a lot of unnecessary memory for + change records at each execution. + */ + if (item != new_item) + current_thd->change_item_tree(&item, new_item); + return Item_str_func::transform(transformer, arg); +} + + void Item_func_make_set::print(String *str) { str->append(STRING_WITH_LEN("make_set(")); diff --git a/sql/item_strfunc.h b/sql/item_strfunc.h index 488dc20b063..e501ccce687 100644 --- a/sql/item_strfunc.h +++ b/sql/item_strfunc.h @@ -475,14 +475,7 @@ public: return item->walk(processor, arg) || Item_str_func::walk(processor, arg); } - Item *transform(Item_transformer transformer, byte *arg) - { - Item *new_item= item->transform(transformer, arg); - if (!new_item) - return 0; - item= new_item; - return Item_str_func::transform(transformer, arg); - } + Item *transform(Item_transformer transformer, byte *arg); void print(String *str); }; -- cgit v1.2.1 From 2f0a610f91d37e21a3db86e82f93ebf98b08f3fd Mon Sep 17 00:00:00 2001 From: unknown Date: Thu, 24 Aug 2006 18:48:26 +0400 Subject: Polishing (was the part of original patch for BUG#16899): Changed trigger-handling code so that there will be the one place for generate statement string for replication log and for trigger file. sql/sql_trigger.cc: Changed trigger-handling code so that there will be the one place for generate statement string for replication log and for trigger file. sql/sql_trigger.h: Changed trigger-handling code so that there will be the one place for generate statement string for replication log and for trigger file. --- sql/sql_trigger.cc | 119 ++++++++++++++++++++++++++--------------------------- sql/sql_trigger.h | 6 +-- 2 files changed, 60 insertions(+), 65 deletions(-) (limited to 'sql') diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc index e806dd4a3f3..6bb50d602c3 100644 --- a/sql/sql_trigger.cc +++ b/sql/sql_trigger.cc @@ -158,11 +158,13 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create) { TABLE *table; bool result= TRUE; - LEX_STRING definer_user; - LEX_STRING definer_host; + String stmt_query; DBUG_ENTER("mysql_create_or_drop_trigger"); + /* Charset of the buffer for statement must be system one. */ + stmt_query.set_charset(system_charset_info); + /* QQ: This function could be merged in mysql_alter_table() function But do we want this ? @@ -264,8 +266,8 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create) } result= (create ? - table->triggers->create_trigger(thd, tables, &definer_user, &definer_host): - table->triggers->drop_trigger(thd, tables)); + table->triggers->create_trigger(thd, tables, &stmt_query): + table->triggers->drop_trigger(thd, tables, &stmt_query)); end: VOID(pthread_mutex_unlock(&LOCK_open)); @@ -277,32 +279,9 @@ end: { thd->clear_error(); - String log_query(thd->query, thd->query_length, system_charset_info); - - if (create) - { - log_query.set((char *) 0, 0, system_charset_info); /* reset log_query */ - - log_query.append(STRING_WITH_LEN("CREATE ")); - - if (definer_user.str && definer_host.str) - { - /* - Append definer-clause if the trigger is SUID (a usual trigger in - new MySQL versions). - */ - - append_definer(thd, &log_query, &definer_user, &definer_host); - } - - log_query.append(thd->lex->stmt_definition_begin, - (char *)thd->lex->sphead->m_body_begin - - thd->lex->stmt_definition_begin + - thd->lex->sphead->m_body.length); - } - /* Such a statement can always go directly to binlog, no trans cache. */ - Query_log_event qinfo(thd, log_query.ptr(), log_query.length(), 0, FALSE); + Query_log_event qinfo(thd, stmt_query.ptr(), stmt_query.length(), 0, + FALSE); mysql_bin_log.write(&qinfo); } @@ -322,22 +301,8 @@ end: LEX) tables - table list containing one open table for which the trigger is created. - definer_user - [out] after a call it points to 0-terminated string or - contains the NULL-string: - - 0-terminated is returned if the trigger is SUID. The - string contains user name part of the actual trigger - definer. - - NULL-string is returned if the trigger is non-SUID. - Anyway, the caller is responsible to provide memory for - storing LEX_STRING object. - definer_host - [out] after a call it points to 0-terminated string or - contains the NULL-string: - - 0-terminated string is returned if the trigger is - SUID. The string contains host name part of the - actual trigger definer. - - NULL-string is returned if the trigger is non-SUID. - Anyway, the caller is responsible to provide memory for - storing LEX_STRING object. + stmt_query - [OUT] after successful return, this string contains + well-formed statement for creation this trigger. NOTE - Assumes that trigger name is fully qualified. @@ -352,8 +317,7 @@ end: True - error */ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables, - LEX_STRING *definer_user, - LEX_STRING *definer_host) + String *stmt_query) { LEX *lex= thd->lex; TABLE *table= tables->table; @@ -361,6 +325,8 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables, trigname_path[FN_REFLEN]; LEX_STRING dir, file, trigname_file; LEX_STRING *trg_def, *name; + LEX_STRING definer_user; + LEX_STRING definer_host; ulonglong *trg_sql_mode; char trg_definer_holder[USER_HOST_BUFF_SIZE]; LEX_STRING *trg_definer; @@ -508,8 +474,6 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables, definers_list.push_back(trg_definer, &table->mem_root)) goto err_with_cleanup; - trg_def->str= thd->query; - trg_def->length= thd->query_length; *trg_sql_mode= thd->variables.sql_mode; #ifndef NO_EMBEDDED_ACCESS_CHECKS @@ -529,27 +493,54 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables, { /* SUID trigger. */ - *definer_user= lex->definer->user; - *definer_host= lex->definer->host; + definer_user= lex->definer->user; + definer_host= lex->definer->host; trg_definer->str= trg_definer_holder; - trg_definer->length= strxmov(trg_definer->str, definer_user->str, "@", - definer_host->str, NullS) - trg_definer->str; + trg_definer->length= strxmov(trg_definer->str, definer_user.str, "@", + definer_host.str, NullS) - trg_definer->str; } else { /* non-SUID trigger. */ - definer_user->str= 0; - definer_user->length= 0; + definer_user.str= 0; + definer_user.length= 0; - definer_host->str= 0; - definer_host->length= 0; + definer_host.str= 0; + definer_host.length= 0; trg_definer->str= (char*) ""; trg_definer->length= 0; } + /* + Create well-formed trigger definition query. Original query is not + appropriated, because definer-clause can be not truncated. + */ + + stmt_query->append(STRING_WITH_LEN("CREATE ")); + + if (trg_definer) + { + /* + Append definer-clause if the trigger is SUID (a usual trigger in + new MySQL versions). + */ + + append_definer(thd, stmt_query, &definer_user, &definer_host); + } + + stmt_query->append(thd->lex->stmt_definition_begin, + (char *) thd->lex->sphead->m_body_begin - + thd->lex->stmt_definition_begin + + thd->lex->sphead->m_body.length); + + trg_def->str= stmt_query->c_ptr(); + trg_def->length= stmt_query->length(); + + /* Create trigger definition file. */ + if (!sql_create_definition_file(&dir, &file, &triggers_file_type, (gptr)this, triggers_file_parameters, 0)) return 0; @@ -647,15 +638,19 @@ static bool save_trigger_file(Table_triggers_list *triggers, const char *db, SYNOPSIS drop_trigger() - thd - current thread context (including trigger definition in LEX) - tables - table list containing one open table for which trigger is - dropped. + thd - current thread context + (including trigger definition in LEX) + tables - table list containing one open table for which trigger + is dropped. + stmt_query - [OUT] after successful return, this string contains + well-formed statement for creation this trigger. RETURN VALUE False - success True - error */ -bool Table_triggers_list::drop_trigger(THD *thd, TABLE_LIST *tables) +bool Table_triggers_list::drop_trigger(THD *thd, TABLE_LIST *tables, + String *stmt_query) { LEX *lex= thd->lex; LEX_STRING *name; @@ -665,6 +660,8 @@ bool Table_triggers_list::drop_trigger(THD *thd, TABLE_LIST *tables) List_iterator it_definer(definers_list); char path[FN_REFLEN]; + stmt_query->append(thd->query, thd->query_length); + while ((name= it_name++)) { it_def++; diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h index e736c3e0e1a..521905a2c56 100644 --- a/sql/sql_trigger.h +++ b/sql/sql_trigger.h @@ -92,10 +92,8 @@ public: } ~Table_triggers_list(); - bool create_trigger(THD *thd, TABLE_LIST *table, - LEX_STRING *definer_user, - LEX_STRING *definer_host); - bool drop_trigger(THD *thd, TABLE_LIST *table); + bool create_trigger(THD *thd, TABLE_LIST *table, String *stmt_query); + bool drop_trigger(THD *thd, TABLE_LIST *table, String *stmt_query); bool process_triggers(THD *thd, trg_event_type event, trg_action_time_type time_type, bool old_row_is_record1); -- cgit v1.2.1 From 807ecdf43ae2c5f35c2f350e67242580618ff9a8 Mon Sep 17 00:00:00 2001 From: unknown Date: Thu, 24 Aug 2006 19:36:26 +0200 Subject: Fix for bug#21416 SP: Recursion level higher than zero needed for non-recursive call The following procedure was not possible if max_sp_recursion_depth is 0 create procedure show_proc() show create procedure show_proc; Actually there is no recursive call but the limit is checked. Solved by temporarily increasing the thread's limit just before the fetch from cache and decreasing after that. mysql-test/r/sp.result: update result mysql-test/t/sp.test: Test for bug #21416 SP: Recursion level higher than zero needed for non-recursive call sql/sp.cc: Increase the max_sp_recursion_depth temporarily for SHOW CREATE PROCEDURE call. This call is in fact not recursive but is counted as such. Outcome, it will work always but if max_sp_recursion_depth is reached we are going to cache one more sp_head instance. --- sql/sp.cc | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) (limited to 'sql') diff --git a/sql/sp.cc b/sql/sp.cc index b7bf049cb1d..fc72822c15e 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -1006,6 +1006,12 @@ sp_find_routine(THD *thd, int type, sp_name *name, sp_cache **cp, } DBUG_RETURN(sp->m_first_free_instance); } + /* + Actually depth could be +1 than the actual value in case a SP calls + SHOW CREATE PROCEDURE. Hence, the linked list could hold up to one more + instance. + */ + level= sp->m_last_cached_sp->m_recursion_level + 1; if (level > depth) { @@ -1175,19 +1181,22 @@ sp_update_procedure(THD *thd, sp_name *name, st_sp_chistics *chistics) int sp_show_create_procedure(THD *thd, sp_name *name) { + int ret= SP_KEY_NOT_FOUND; sp_head *sp; DBUG_ENTER("sp_show_create_procedure"); DBUG_PRINT("enter", ("name: %.*s", name->m_name.length, name->m_name.str)); + /* + Increase the recursion limit for this statement. SHOW CREATE PROCEDURE + does not do actual recursion. + */ + thd->variables.max_sp_recursion_depth++; if ((sp= sp_find_routine(thd, TYPE_ENUM_PROCEDURE, name, &thd->sp_proc_cache, FALSE))) - { - int ret= sp->show_create_procedure(thd); + ret= sp->show_create_procedure(thd); - DBUG_RETURN(ret); - } - - DBUG_RETURN(SP_KEY_NOT_FOUND); + thd->variables.max_sp_recursion_depth--; + DBUG_RETURN(ret); } -- cgit v1.2.1 From 5d37fce9e84d5efc669306bddc156849aeaade87 Mon Sep 17 00:00:00 2001 From: unknown Date: Fri, 25 Aug 2006 11:34:13 +0400 Subject: Comment cleanup after push of bug#21166. --- sql/item.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'sql') diff --git a/sql/item.cc b/sql/item.cc index ac0658224b3..007b3ee600f 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -421,11 +421,10 @@ void Item::rename(char *new_name) /* - transform() - traverse item tree possibly transforming it (replacing - items) + Traverse item tree possibly transforming it (replacing items). SYNOPSIS - transform() + Item::transform() transformer functor that performs transformation of a subtree arg opaque argument passed to the functor @@ -450,9 +449,9 @@ void Item::rename(char *new_name) it, please use Item::walk() instead. - RETURN + RETURN VALUE Returns pointer to the new subtree root. THD::change_item_tree() - should be called for it if transformation took place, i.e. if + should be called for it if transformation took place, i.e. if a pointer to newly allocated item is returned. */ @@ -5339,9 +5338,10 @@ int Item_default_value::save_in_field(Field *field_arg, bool no_conversions) /* - This method like the walk method traverses the item tree, but at - the same time it can replace some nodes in the tree + This method like the walk method traverses the item tree, but at the + same time it can replace some nodes in the tree */ + Item *Item_default_value::transform(Item_transformer transformer, byte *args) { DBUG_ASSERT(!current_thd->is_stmt_prepare()); -- cgit v1.2.1 From 133d2aa599a65637b28e1d0634274015c49dc4e2 Mon Sep 17 00:00:00 2001 From: unknown Date: Fri, 25 Aug 2006 15:51:29 +0200 Subject: Fix for bug#21795: SP: sp_head::is_not_allowed_in_function() contains erroneous check Problem: Actually there were two problems in the server code. The check for SQLCOM_FLUSH in SF/Triggers were not according to the existing architecture which uses sp_get_flags_for_command() from sp_head.cc . This function was also missing a check for SQLCOM_FLUSH which has a problem combined with prelocking. This changeset fixes both of these deficiencies as well as the erroneous check in sp_head::is_not_allowed_in_function() which was a copy&paste error. mysql-test/r/sp-error.result: update result mysql-test/r/trigger.result: update result mysql-test/t/sp-error.test: FLUSH can create a problem with prelocking, hence it's disabled. There is a better way to check this than a check in the parser. Now we use sp_get_flags_for_command() and the error returned is different. mysql-test/t/trigger.test: FLUSH can create a problem with prelocking, hence it's disabled. There is a better way to check this than a check in the parser. Now we use sp_get_flags_for_command() and the error returned is different. sql/sp_head.cc: FLUSH and RESET are not allowed inside a SF/Trigger. Because they don't imply a COMMIT sp_head::HAS_COMMIT_OR_ROLLBACK cannot be used. Two new flags were introduced for that reason. sql/sp_head.h: Don't check m_type as this check is erroneous. This is probably a copy and paste error when moving code from somewhere else. Another fact which supports this was prefixing the enum value with the name of class sp_head. Adding two new flags HAS_SQLCOM_RESET and HAS_SQLCOM_FLUSH. The values are 2048 and 4096 because in the 5.1 branch there are already new flags which are with values up-to 1024. sql/sql_parse.cc: FLUSH can cause a problem with prelocking in SF/Trigger and therefore is already disabled. RESET is also disabled because is handled by the same code as FLUSH. We won't allow RESET inside SF/Trigger at that stage without thorough analysis. The check for them is already done in the parser by calling is_not_allowed_in_function() sql/sql_yacc.yy: By listing SQLCOM_FLUSH as command which implies COMMIT in sp_get_flags_for_command() the check in sql_yacc.yy is obsolete. --- sql/sp_head.cc | 6 ++++++ sql/sp_head.h | 18 +++++++++++------- sql/sql_parse.cc | 6 +----- sql/sql_yacc.yy | 13 ++----------- 4 files changed, 20 insertions(+), 23 deletions(-) (limited to 'sql') diff --git a/sql/sp_head.cc b/sql/sp_head.cc index eec6e0fc3cd..9761de625be 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -230,6 +230,12 @@ sp_get_flags_for_command(LEX *lex) else flags= sp_head::HAS_COMMIT_OR_ROLLBACK; break; + case SQLCOM_FLUSH: + flags= sp_head::HAS_SQLCOM_FLUSH; + break; + case SQLCOM_RESET: + flags= sp_head::HAS_SQLCOM_RESET; + break; case SQLCOM_CREATE_INDEX: case SQLCOM_CREATE_DB: case SQLCOM_CREATE_VIEW: diff --git a/sql/sp_head.h b/sql/sp_head.h index 4cd34bc9e20..7f2da69aa0c 100644 --- a/sql/sp_head.h +++ b/sql/sp_head.h @@ -115,7 +115,9 @@ public: IS_INVOKED= 32, // Is set if this sp_head is being used HAS_SET_AUTOCOMMIT_STMT= 64,// Is set if a procedure with 'set autocommit' /* Is set if a procedure with COMMIT (implicit or explicit) | ROLLBACK */ - HAS_COMMIT_OR_ROLLBACK= 128 + HAS_COMMIT_OR_ROLLBACK= 128, + HAS_SQLCOM_RESET= 2048, + HAS_SQLCOM_FLUSH= 4096 }; /* TYPE_ENUM_FUNCTION, TYPE_ENUM_PROCEDURE or TYPE_ENUM_TRIGGER */ @@ -335,14 +337,16 @@ public: my_error(ER_SP_NO_RETSET, MYF(0), where); else if (m_flags & HAS_SET_AUTOCOMMIT_STMT) my_error(ER_SP_CANT_SET_AUTOCOMMIT, MYF(0)); - else if (m_type != TYPE_ENUM_PROCEDURE && - (m_flags & sp_head::HAS_COMMIT_OR_ROLLBACK)) - { + else if (m_flags & HAS_COMMIT_OR_ROLLBACK) my_error(ER_COMMIT_NOT_ALLOWED_IN_SF_OR_TRG, MYF(0)); - return TRUE; - } + else if (m_flags & HAS_SQLCOM_RESET) + my_error(ER_STMT_NOT_ALLOWED_IN_SF_OR_TRG, MYF(0), "RESET"); + else if (m_flags & HAS_SQLCOM_FLUSH) + my_error(ER_STMT_NOT_ALLOWED_IN_SF_OR_TRG, MYF(0), "FLUSH"); + return test(m_flags & - (CONTAINS_DYNAMIC_SQL|MULTI_RESULTS|HAS_SET_AUTOCOMMIT_STMT)); + (CONTAINS_DYNAMIC_SQL|MULTI_RESULTS|HAS_SET_AUTOCOMMIT_STMT| + HAS_COMMIT_OR_ROLLBACK|HAS_SQLCOM_RESET|HAS_SQLCOM_FLUSH)); } #ifndef DBUG_OFF diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index f3ac1280983..3d3e98a08cd 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -6681,11 +6681,7 @@ bool reload_acl_and_cache(THD *thd, ulong options, TABLE_LIST *tables, select_errors=0; /* Write if more errors */ bool tmp_write_to_binlog= 1; - if (thd && thd->in_sub_stmt) - { - my_error(ER_STMT_NOT_ALLOWED_IN_SF_OR_TRG, MYF(0), "FLUSH"); - return 1; - } + DBUG_ASSERT(!thd || !thd->in_sub_stmt); #ifndef NO_EMBEDDED_ACCESS_CHECKS if (options & REFRESH_GRANT) diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 133b6e18fee..6bf76f7e503 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -6783,17 +6783,8 @@ flush: FLUSH_SYM opt_no_write_to_binlog { LEX *lex=Lex; - if (lex->sphead && lex->sphead->m_type != TYPE_ENUM_PROCEDURE) - { - /* - Note that both FLUSH TABLES and FLUSH PRIVILEGES will break - execution in prelocked mode. So it is better to disable - FLUSH in stored functions and triggers completely. - */ - my_error(ER_STMT_NOT_ALLOWED_IN_SF_OR_TRG, MYF(0), "FLUSH"); - YYABORT; - } - lex->sql_command= SQLCOM_FLUSH; lex->type=0; + lex->sql_command= SQLCOM_FLUSH; + lex->type= 0; lex->no_write_to_binlog= $2; } flush_options -- cgit v1.2.1 From 7a5a2544bf1e1b1abb910968340ad4b9d6162065 Mon Sep 17 00:00:00 2001 From: unknown Date: Tue, 29 Aug 2006 14:32:59 +0400 Subject: BUG#17591: Updatable view not possible with trigger or stored function When a view was used inside a trigger or a function, lock type for tables used in a view was always set to READ (thus making the view non-updatable), even if we were trying to update the view. The solution is to set lock type properly. mysql-test/r/view.result: Add result for bug#17591: Updatable view not possible with trigger or stored function. mysql-test/t/view.test: Add test case for bug#17591: Updatable view not possible with trigger or stored function. sql/sql_view.cc: Move the code that sets requested lock type before the point where we exit from mysql_make_view() when we process a placeholder for prelocked table. --- sql/sql_view.cc | 52 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 22 deletions(-) (limited to 'sql') diff --git a/sql/sql_view.cc b/sql/sql_view.cc index 637d2cc3684..361e86fb048 100644 --- a/sql/sql_view.cc +++ b/sql/sql_view.cc @@ -1052,6 +1052,31 @@ bool mysql_make_view(THD *thd, File_parser *parser, TABLE_LIST *table) table->next_global= view_tables; } + bool view_is_mergeable= (table->algorithm != VIEW_ALGORITHM_TMPTABLE && + lex->can_be_merged()); + TABLE_LIST *view_main_select_tables; + if (view_is_mergeable) + { + /* + Currently 'view_main_select_tables' differs from 'view_tables' + only then view has CONVERT_TZ() function in its select list. + This may change in future, for example if we enable merging of + views with subqueries in select list. + */ + view_main_select_tables= + (TABLE_LIST*)lex->select_lex.table_list.first; + + /* + Let us set proper lock type for tables of the view's main + select since we may want to perform update or insert on + view. This won't work for view containing union. But this is + ok since we don't allow insert and update on such views + anyway. + */ + for (tbl= view_main_select_tables; tbl; tbl= tbl->next_local) + tbl->lock_type= table->lock_type; + } + /* If we are opening this view as part of implicit LOCK TABLES, then this view serves as simple placeholder and we should not continue @@ -1106,43 +1131,26 @@ bool mysql_make_view(THD *thd, File_parser *parser, TABLE_LIST *table) - VIEW SELECT allow merging - VIEW used in subquery or command support MERGE algorithm */ - if (table->algorithm != VIEW_ALGORITHM_TMPTABLE && - lex->can_be_merged() && + if (view_is_mergeable && (table->select_lex->master_unit() != &old_lex->unit || old_lex->can_use_merged()) && !old_lex->can_not_use_merged()) { - List_iterator_fast ti(view_select->top_join_list); - /* - Currently 'view_main_select_tables' differs from 'view_tables' - only then view has CONVERT_TZ() function in its select list. - This may change in future, for example if we enable merging - of views with subqueries in select list. - */ - TABLE_LIST *view_main_select_tables= - (TABLE_LIST*)lex->select_lex.table_list.first; /* lex should contain at least one table */ DBUG_ASSERT(view_main_select_tables != 0); + List_iterator_fast ti(view_select->top_join_list); + table->effective_algorithm= VIEW_ALGORITHM_MERGE; DBUG_PRINT("info", ("algorithm: MERGE")); table->updatable= (table->updatable_view != 0); table->effective_with_check= old_lex->get_effective_with_check(table); table->merge_underlying_list= view_main_select_tables; - /* - Let us set proper lock type for tables of the view's main select - since we may want to perform update or insert on view. This won't - work for view containing union. But this is ok since we don't - allow insert and update on such views anyway. - Also we fill correct wanted privileges. - */ - for (tbl= table->merge_underlying_list; tbl; tbl= tbl->next_local) - { - tbl->lock_type= table->lock_type; + /* Fill correct wanted privileges. */ + for (tbl= view_main_select_tables; tbl; tbl= tbl->next_local) tbl->grant.want_privilege= top_view->grant.orig_want_privilege; - } /* prepare view context */ lex->select_lex.context.resolve_in_table_list_only(view_main_select_tables); -- cgit v1.2.1 From 4355ea5a38c819cf1e98089b3d85fc628c48da46 Mon Sep 17 00:00:00 2001 From: unknown Date: Wed, 30 Aug 2006 00:38:58 +0400 Subject: A fix for Bug#14897 "ResultSet.getString("table.column") sometimes doesn't find the column" When a user was using 4.1 tables with VARCHAR column and 5.0 server and a query that used a temporary table to resolve itself, the table metadata for the varchar column sent to client was incorrect: MYSQL_FIELD::table member was empty. The bug was caused by implicit "upgrade" from old VARCHAR to new VARCHAR hard-coded in Field::new_field, which did not preserve the information about the original table. Thus, the field metadata of the "upgraded" field pointed to an auxiliary temporary table created for query execution. The fix is to copy the pointer to the original table to the new field. mysql-test/r/type_varchar.result: Update test results (Bug#14897) mysql-test/t/type_varchar.test: Add a test case for Bug#14897 "ResultSet.getString("table.column") sometimes doesn't find the column" sql/field.cc: Preserve the original table name when converting fields from old VARCHAR to new VARCHAR. mysql-test/std_data/14897.frm: New BitKeeper file ``mysql-test/std_data/14897.frm'' --- sql/field.cc | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) (limited to 'sql') diff --git a/sql/field.cc b/sql/field.cc index 921148e8f0f..05eb7d2d744 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -6154,15 +6154,26 @@ Field *Field_string::new_field(MEM_ROOT *root, struct st_table *new_table, Field *new_field; if (type() != MYSQL_TYPE_VAR_STRING || keep_type) - return Field::new_field(root, new_table, keep_type); + new_field= Field::new_field(root, new_table, keep_type); + else + { - /* - Old VARCHAR field which should be modified to a VARCHAR on copy - This is done to ensure that ALTER TABLE will convert old VARCHAR fields - to now VARCHAR fields. - */ - return new Field_varstring(field_length, maybe_null(), - field_name, new_table, charset()); + /* + Old VARCHAR field which should be modified to a VARCHAR on copy + This is done to ensure that ALTER TABLE will convert old VARCHAR fields + to now VARCHAR fields. + */ + new_field= new Field_varstring(field_length, maybe_null(), + field_name, new_table, charset()); + /* + Normally orig_table is different from table only if field was created + via ::new_field. Here we alter the type of field, so ::new_field is + not applicable. But we still need to preserve the original field + metadata for the client-server protocol. + */ + new_field->orig_table= orig_table; + } + return new_field; } /**************************************************************************** -- cgit v1.2.1