From c290b26c0eb3010eb7d605d119015eb6953060f9 Mon Sep 17 00:00:00 2001 From: "davi@moksha.local" <> Date: Wed, 15 Aug 2007 12:13:34 -0300 Subject: [PATCH 1/5] Bug#25856 (HANDLER table OPEN in one connection lock DROP TABLE in another one) mysql_ha_open calls mysql_ha_close on the error path (unsupported) to close the (opened) table before inserting it into the tables hash list handler_tables_hash) but mysql_ha_close only closes tables which are on the hash list, causing the table to be left open and locked. This change moves the table close logic into a separate function that is always called on the error path of mysql_ha_open or on a normal handler close (mysql_ha_close). --- mysql-test/r/handler.result | 7 ++++ mysql-test/t/handler.test | 14 +++++++ sql/sql_handler.cc | 80 ++++++++++++++++++++++--------------- 3 files changed, 68 insertions(+), 33 deletions(-) diff --git a/mysql-test/r/handler.result b/mysql-test/r/handler.result index 85cf47b5806..5e123df9103 100644 --- a/mysql-test/r/handler.result +++ b/mysql-test/r/handler.result @@ -482,3 +482,10 @@ ERROR 42S02: Table 'test.t1' doesn't exist drop table if exists t1; Warnings: Note 1051 Unknown table 't1' +drop table if exists t1; +create table t1 (a int) ENGINE=MEMORY; +--> client 2 +handler t1 open; +ERROR HY000: Table storage engine for 't1' doesn't have this option +--> client 1 +drop table t1; diff --git a/mysql-test/t/handler.test b/mysql-test/t/handler.test index bf18b8da941..4edd5b5ec32 100644 --- a/mysql-test/t/handler.test +++ b/mysql-test/t/handler.test @@ -427,3 +427,17 @@ select * from t1; # Just to be sure and not confuse the next test case writer. drop table if exists t1; +# +# Bug#25856 - HANDLER table OPEN in one connection lock DROP TABLE in another one +# +--disable_warnings +drop table if exists t1; +--enable_warnings +create table t1 (a int) ENGINE=MEMORY; +--echo --> client 2 +connection con2; +--error 1031 +handler t1 open; +--echo --> client 1 +connection default; +drop table t1; diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc index e1318aa2736..e00d9110378 100644 --- a/sql/sql_handler.cc +++ b/sql/sql_handler.cc @@ -119,6 +119,44 @@ static void mysql_ha_hash_free(TABLE_LIST *tables) my_free((char*) tables, MYF(0)); } +/** + * Close a HANDLER table. + * + * @param thd Thread identifier. + * @param A list of tables with the first entry to close. + * + * @note Though this function takes a list of tables, only the first list entry + * will be closed. + * @note Broadcasts refresh if it closed the table. + */ + +static void mysql_ha_close_table(THD *thd, TABLE_LIST *tables) +{ + TABLE **table_ptr; + + /* + Though we could take the table pointer from hash_tables->table, + we must follow the thd->handler_tables chain anyway, as we need the + address of the 'next' pointer referencing this table + for close_thread_table(). + */ + for (table_ptr= &(thd->handler_tables); + *table_ptr && (*table_ptr != tables->table); + table_ptr= &(*table_ptr)->next) + ; + + if (*table_ptr) + { + (*table_ptr)->file->ha_index_or_rnd_end(); + VOID(pthread_mutex_lock(&LOCK_open)); + if (close_thread_table(thd, table_ptr)) + { + /* Tell threads waiting for refresh that something has happened */ + broadcast_refresh(); + } + VOID(pthread_mutex_unlock(&LOCK_open)); + } +} /* Open a HANDLER table. @@ -145,7 +183,7 @@ static void mysql_ha_hash_free(TABLE_LIST *tables) bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen) { - TABLE_LIST *hash_tables; + TABLE_LIST *hash_tables = NULL; char *db, *name, *alias; uint dblen, namelen, aliaslen, counter; int error; @@ -197,7 +235,6 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen) { if (! reopen) my_error(ER_ILLEGAL_HA, MYF(0), tables->alias); - mysql_ha_close(thd, tables); goto err; } @@ -225,11 +262,7 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen) /* add to hash */ if (my_hash_insert(&thd->handler_tables_hash, (byte*) hash_tables)) - { - my_free((char*) hash_tables, MYF(0)); - mysql_ha_close(thd, tables); goto err; - } } if (! reopen) @@ -238,13 +271,17 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen) DBUG_RETURN(FALSE); err: + if (hash_tables) + my_free((char*) hash_tables, MYF(0)); + if (tables->table) + mysql_ha_close_table(thd, tables); DBUG_PRINT("exit",("ERROR")); DBUG_RETURN(TRUE); } /* - Close a HANDLER table. + Close a HANDLER table by alias or table name SYNOPSIS mysql_ha_close() @@ -252,9 +289,8 @@ err: tables A list of tables with the first entry to close. DESCRIPTION - Though this function takes a list of tables, only the first list entry - will be closed. - Broadcasts refresh if it closed the table. + Closes the table that is associated (on the handler tables hash) with the + name (table->alias) of the specified table. RETURN FALSE ok @@ -264,7 +300,6 @@ err: bool mysql_ha_close(THD *thd, TABLE_LIST *tables) { TABLE_LIST *hash_tables; - TABLE **table_ptr; DBUG_ENTER("mysql_ha_close"); DBUG_PRINT("enter",("'%s'.'%s' as '%s'", tables->db, tables->table_name, tables->alias)); @@ -273,28 +308,7 @@ bool mysql_ha_close(THD *thd, TABLE_LIST *tables) (byte*) tables->alias, strlen(tables->alias) + 1))) { - /* - Though we could take the table pointer from hash_tables->table, - we must follow the thd->handler_tables chain anyway, as we need the - address of the 'next' pointer referencing this table - for close_thread_table(). - */ - for (table_ptr= &(thd->handler_tables); - *table_ptr && (*table_ptr != hash_tables->table); - table_ptr= &(*table_ptr)->next) - ; - - if (*table_ptr) - { - (*table_ptr)->file->ha_index_or_rnd_end(); - VOID(pthread_mutex_lock(&LOCK_open)); - if (close_thread_table(thd, table_ptr)) - { - /* Tell threads waiting for refresh that something has happened */ - broadcast_refresh(); - } - VOID(pthread_mutex_unlock(&LOCK_open)); - } + mysql_ha_close_table(thd, hash_tables); hash_delete(&thd->handler_tables_hash, (byte*) hash_tables); } else From d4aad350957bb8f75ad5c60084ded86f063bdc28 Mon Sep 17 00:00:00 2001 From: "davi@moksha.local" <> Date: Wed, 15 Aug 2007 17:12:09 -0300 Subject: [PATCH 2/5] Rework doxygen documentation for the function mysql_ha_close_table. --- sql/sql_handler.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc index e00d9110378..4ff91bd40a6 100644 --- a/sql/sql_handler.cc +++ b/sql/sql_handler.cc @@ -120,15 +120,15 @@ static void mysql_ha_hash_free(TABLE_LIST *tables) } /** - * Close a HANDLER table. - * - * @param thd Thread identifier. - * @param A list of tables with the first entry to close. - * - * @note Though this function takes a list of tables, only the first list entry - * will be closed. - * @note Broadcasts refresh if it closed the table. - */ + Close a HANDLER table. + + @param thd Thread identifier. + @param tables A list of tables with the first entry to close. + + @note Though this function takes a list of tables, only the first list entry + will be closed. + @note Broadcasts refresh if it closed the table. +*/ static void mysql_ha_close_table(THD *thd, TABLE_LIST *tables) { From d6692280061d9391e5cae67fcf11f87dc6fd8a13 Mon Sep 17 00:00:00 2001 From: "davi@moksha.local" <> Date: Thu, 16 Aug 2007 14:51:37 -0300 Subject: [PATCH 3/5] Bug#29936 (Stored Procedure DML ignores low_priority_updates setting) This is a follow up for the patch for Bug#26162 "Trigger DML ignores low_priority_updates setting", where the stored procedure ignores the session setting of low_priority_updates. For every table open operation with default write (TL_WRITE_DEFAULT) lock_type, downgrade the lock type to the session setting of low_priority_updates. --- sql/lock.cc | 1 + sql/sql_base.cc | 9 ++------- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/sql/lock.cc b/sql/lock.cc index f730ac56d35..0036d0aef77 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -730,6 +730,7 @@ static MYSQL_LOCK *get_lock_data(THD *thd, TABLE **table_ptr, uint count, if ((table=table_ptr[i])->s->tmp_table == NON_TRANSACTIONAL_TMP_TABLE) continue; lock_type= table->reginfo.lock_type; + DBUG_ASSERT (lock_type != TL_WRITE_DEFAULT); if (lock_type >= TL_WRITE_ALLOW_WRITE) { *write_lock_used=table; diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 1c01248c283..0477c40b42d 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1537,7 +1537,6 @@ TABLE *open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root, HASH_SEARCH_STATE state; DBUG_ENTER("open_table"); - DBUG_ASSERT (table_list->lock_type != TL_WRITE_DEFAULT); /* find a unused table in the open table cache */ if (refresh) *refresh=0; @@ -2708,11 +2707,6 @@ int open_tables(THD *thd, TABLE_LIST **start, uint *counter, uint flags) { safe_to_ignore_table= FALSE; - if (tables->lock_type == TL_WRITE_DEFAULT) - { - tables->lock_type= thd->update_lock_default; - DBUG_ASSERT (tables->lock_type >= TL_WRITE_ALLOW_WRITE); - } /* Ignore placeholders for derived tables. After derived tables processing, link to created temporary table will be put here. @@ -2857,7 +2851,8 @@ int open_tables(THD *thd, TABLE_LIST **start, uint *counter, uint flags) } if (tables->lock_type != TL_UNLOCK && ! thd->locked_tables) - tables->table->reginfo.lock_type=tables->lock_type; + tables->table->reginfo.lock_type= tables->lock_type == TL_WRITE_DEFAULT ? + thd->update_lock_default : tables->lock_type; tables->table->grant= tables->grant; process_view_routines: From a4248c2dd370fd7c64b4fa48085f55fd4fe41272 Mon Sep 17 00:00:00 2001 From: "thek@adventure.(none)" <> Date: Fri, 17 Aug 2007 16:55:20 +0200 Subject: [PATCH 4/5] Bug #30269 Query cache eats memory Although the query cache doesn't support retrieval of statements containing column level access control, it was still possible to cache such statements thus wasting memory. This patch extends the access control check on the target tables to avoid caching a statement with column level restrictions. --- mysql-test/r/query_cache.result | 24 ++++++++++++++++++++++++ mysql-test/t/query_cache.test | 30 +++++++++++++++++++++++++++++- sql/sql_cache.cc | 24 +++++++++++++++++++++--- sql/sql_cache.h | 8 +++++--- 4 files changed, 79 insertions(+), 7 deletions(-) diff --git a/mysql-test/r/query_cache.result b/mysql-test/r/query_cache.result index f1f99012910..ecf7df2d2ae 100644 --- a/mysql-test/r/query_cache.result +++ b/mysql-test/r/query_cache.result @@ -1502,6 +1502,30 @@ a (select count(*) from t2) 3 0 4 0 drop table t1,t2; +DROP DATABASE IF EXISTS bug30269; +CREATE DATABASE bug30269; +USE bug30269; +CREATE TABLE test1 (id int, name varchar(23)); +CREATE VIEW view1 AS SELECT id FROM test1; +INSERT INTO test1 VALUES (5, 'testit'); +GRANT SELECT (id) ON TABLE bug30269.test1 TO 'bug30269'@'localhost'; +GRANT SELECT ON TABLE bug30269.view1 TO 'bug30269'@'localhost'; +set global query_cache_size= 81920; +USE bug30269; +show status like 'Qcache_queries_in_cache'; +Variable_name Value +Qcache_queries_in_cache 0 +SELECT id FROM test1 WHERE id>2; +id +5 +SELECT id FROM view1 WHERE id>2; +id +5 +show status like 'Qcache_queries_in_cache'; +Variable_name Value +Qcache_queries_in_cache 0 +DROP DATABASE bug30269; +DROP USER 'bug30269'@'localhost'; set GLOBAL query_cache_type=default; set GLOBAL query_cache_limit=default; set GLOBAL query_cache_min_res_unit=default; diff --git a/mysql-test/t/query_cache.test b/mysql-test/t/query_cache.test index 962f53936a3..7f4d4227f41 100644 --- a/mysql-test/t/query_cache.test +++ b/mysql-test/t/query_cache.test @@ -1096,9 +1096,37 @@ connection default; disconnect user1; disconnect user2; disconnect user3; + +# +# Bug #30269 Query cache eats memory +# +--disable_warnings +DROP DATABASE IF EXISTS bug30269; +--enable_warnings +CREATE DATABASE bug30269; +USE bug30269; +CREATE TABLE test1 (id int, name varchar(23)); +CREATE VIEW view1 AS SELECT id FROM test1; +INSERT INTO test1 VALUES (5, 'testit'); +GRANT SELECT (id) ON TABLE bug30269.test1 TO 'bug30269'@'localhost'; +GRANT SELECT ON TABLE bug30269.view1 TO 'bug30269'@'localhost'; +set global query_cache_size= 81920; +connect (bug30269, localhost, bug30269,,); +connection bug30269; +USE bug30269; +show status like 'Qcache_queries_in_cache'; +SELECT id FROM test1 WHERE id>2; +SELECT id FROM view1 WHERE id>2; +show status like 'Qcache_queries_in_cache'; + +connection default; +DROP DATABASE bug30269; +disconnect bug30269; +DROP USER 'bug30269'@'localhost'; + set GLOBAL query_cache_type=default; set GLOBAL query_cache_limit=default; set GLOBAL query_cache_min_res_unit=default; set GLOBAL query_cache_size=default; -# End of 5.0 tests +# End of 5.0 tests diff --git a/sql/sql_cache.cc b/sql/sql_cache.cc index 33d658ce6a1..4a5bb263a5f 100644 --- a/sql/sql_cache.cc +++ b/sql/sql_cache.cc @@ -2991,14 +2991,31 @@ void Query_cache::double_linked_list_join(Query_cache_block *head_tail, >0 number of tables */ -static TABLE_COUNTER_TYPE process_and_count_tables(TABLE_LIST *tables_used, - uint8 *tables_type) +TABLE_COUNTER_TYPE +Query_cache::process_and_count_tables(THD *thd, TABLE_LIST *tables_used, + uint8 *tables_type) { DBUG_ENTER("process_and_count_tables"); TABLE_COUNTER_TYPE table_count = 0; for (; tables_used; tables_used= tables_used->next_global) { table_count++; +#ifdef HAVE_QUERY_CACHE + /* + Disable any attempt to store this statement if there are + column level grants on any referenced tables. + The grant.want_privileges flag was set to 1 in the + check_grant() function earlier if the TABLE_LIST object + had any associated column privileges. + */ + if (tables_used->grant.want_privilege) + { + DBUG_PRINT("qcache", ("Don't cache statement as it refers to " + "tables with column privileges.")); + thd->lex->safe_to_cache_query= 0; + DBUG_RETURN(0); + } +#endif if (tables_used->view) { DBUG_PRINT("qcache", ("view: %s db: %s", @@ -3071,7 +3088,8 @@ Query_cache::is_cacheable(THD *thd, uint32 query_len, char *query, LEX *lex, (long) lex->select_lex.options, (int) thd->variables.query_cache_type)); - if (!(table_count= process_and_count_tables(tables_used, tables_type))) + if (!(table_count= process_and_count_tables(thd, tables_used, + tables_type))) DBUG_RETURN(0); if ((thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)) && diff --git a/sql/sql_cache.h b/sql/sql_cache.h index bc00f7ea629..34fc3a5c8d5 100644 --- a/sql/sql_cache.h +++ b/sql/sql_cache.h @@ -368,10 +368,12 @@ 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); + LEX *lex, TABLE_LIST *tables_used, + uint8 *tables_type); + TABLE_COUNTER_TYPE process_and_count_tables(THD *thd, + TABLE_LIST *tables_used, + uint8 *tables_type); static my_bool ask_handler_allowance(THD *thd, TABLE_LIST *tables_used); public: From acfe3fc92420e4fd51fe7c05803026d93891939a Mon Sep 17 00:00:00 2001 From: "thek@adventure.(none)" <> Date: Tue, 21 Aug 2007 13:43:09 +0200 Subject: [PATCH 5/5] Bug#30269 Query cache eats memory Although the query cache doesn't support retrieval of statements containing column level access control, it was still possible to cache such statements thus wasting memory. This patch extends the access control check on the target tables to avoid caching a statement with column level restrictions. Views are excepted and can be cached but only retrieved by super user account. --- mysql-test/r/query_cache.result | 13 +++++++++---- ...y_cache.result => query_cache_with_views.result} | 0 mysql-test/t/query_cache.test | 5 ++++- ...query_cache.test => query_cache_with_views.test} | 0 sql/sql_cache.cc | 13 ++++++++++++- 5 files changed, 25 insertions(+), 6 deletions(-) rename mysql-test/r/{view_query_cache.result => query_cache_with_views.result} (100%) rename mysql-test/t/{view_query_cache.test => query_cache_with_views.test} (100%) diff --git a/mysql-test/r/query_cache.result b/mysql-test/r/query_cache.result index ecf7df2d2ae..4bae61ea494 100644 --- a/mysql-test/r/query_cache.result +++ b/mysql-test/r/query_cache.result @@ -1503,10 +1503,11 @@ a (select count(*) from t2) 4 0 drop table t1,t2; DROP DATABASE IF EXISTS bug30269; +FLUSH STATUS; CREATE DATABASE bug30269; USE bug30269; CREATE TABLE test1 (id int, name varchar(23)); -CREATE VIEW view1 AS SELECT id FROM test1; +CREATE VIEW view1 AS SELECT * FROM test1; INSERT INTO test1 VALUES (5, 'testit'); GRANT SELECT (id) ON TABLE bug30269.test1 TO 'bug30269'@'localhost'; GRANT SELECT ON TABLE bug30269.view1 TO 'bug30269'@'localhost'; @@ -1515,15 +1516,19 @@ USE bug30269; show status like 'Qcache_queries_in_cache'; Variable_name Value Qcache_queries_in_cache 0 +# Select statement not stored in query cache because of column privileges. SELECT id FROM test1 WHERE id>2; id 5 -SELECT id FROM view1 WHERE id>2; -id -5 show status like 'Qcache_queries_in_cache'; Variable_name Value Qcache_queries_in_cache 0 +SELECT id FROM view1 WHERE id>2; +id +5 +show status like 'Qcache_queries_in_cache'; +Variable_name Value +Qcache_queries_in_cache 1 DROP DATABASE bug30269; DROP USER 'bug30269'@'localhost'; set GLOBAL query_cache_type=default; diff --git a/mysql-test/r/view_query_cache.result b/mysql-test/r/query_cache_with_views.result similarity index 100% rename from mysql-test/r/view_query_cache.result rename to mysql-test/r/query_cache_with_views.result diff --git a/mysql-test/t/query_cache.test b/mysql-test/t/query_cache.test index 7f4d4227f41..9d2e05fb874 100644 --- a/mysql-test/t/query_cache.test +++ b/mysql-test/t/query_cache.test @@ -1103,10 +1103,11 @@ disconnect user3; --disable_warnings DROP DATABASE IF EXISTS bug30269; --enable_warnings +FLUSH STATUS; CREATE DATABASE bug30269; USE bug30269; CREATE TABLE test1 (id int, name varchar(23)); -CREATE VIEW view1 AS SELECT id FROM test1; +CREATE VIEW view1 AS SELECT * FROM test1; INSERT INTO test1 VALUES (5, 'testit'); GRANT SELECT (id) ON TABLE bug30269.test1 TO 'bug30269'@'localhost'; GRANT SELECT ON TABLE bug30269.view1 TO 'bug30269'@'localhost'; @@ -1115,7 +1116,9 @@ connect (bug30269, localhost, bug30269,,); connection bug30269; USE bug30269; show status like 'Qcache_queries_in_cache'; +--echo # Select statement not stored in query cache because of column privileges. SELECT id FROM test1 WHERE id>2; +show status like 'Qcache_queries_in_cache'; SELECT id FROM view1 WHERE id>2; show status like 'Qcache_queries_in_cache'; diff --git a/mysql-test/t/view_query_cache.test b/mysql-test/t/query_cache_with_views.test similarity index 100% rename from mysql-test/t/view_query_cache.test rename to mysql-test/t/query_cache_with_views.test diff --git a/sql/sql_cache.cc b/sql/sql_cache.cc index 4a5bb263a5f..cc5b4276c82 100644 --- a/sql/sql_cache.cc +++ b/sql/sql_cache.cc @@ -3007,8 +3007,19 @@ Query_cache::process_and_count_tables(THD *thd, TABLE_LIST *tables_used, The grant.want_privileges flag was set to 1 in the check_grant() function earlier if the TABLE_LIST object had any associated column privileges. + + We need to check that the TABLE_LIST object isn't part + of a VIEW definition because we want to be able to cache + views. + + TODO: Although it is possible to cache views, the privilege + check on view tables always fall back on column privileges + even if there are more generic table privileges. Thus it isn't + currently possible to retrieve cached view-tables unless the + client has the super user privileges. */ - if (tables_used->grant.want_privilege) + if (tables_used->grant.want_privilege && + tables_used->belong_to_view == NULL) { DBUG_PRINT("qcache", ("Don't cache statement as it refers to " "tables with column privileges."));