From: Dmitry Lenev Date: October 26 2010 4:26am Subject: bzr push into mysql-5.5-runtime branch (Dmitry.Lenev:3180 to 3181) List-Archive: http://lists.mysql.com/commits/121875 Message-Id: <20101026042630.3C4862F0E23@mockturtle> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 3181 Dmitry Lenev 2010-10-26 More changes to draft patch refactoring global read lock implementation. Makes GRL yet another type of metadata lock and thus exposes it to deadlock detector in MDL subsystem. Solves bugs #54673 "It takes too long to get readlock for 'FLUSH TABLES WITH READ LOCK'" and #57006 "Deadlock between HANDLER and FLUSH TABLES WITH READ LOCK". Work-in-progress. Get rid of special GRL handling in CREATE/DROP PROCEDURE/FUNCTION. modified: sql/lock.cc sql/sql_class.h sql/sql_parse.cc 3180 Dmitry Lenev 2010-10-25 More changes to draft patch refactoring global read lock implementation. Makes GRL yet another type of metadata lock and thus exposes it to deadlock detector in MDL subsystem. Solves bugs #54673 "It takes too long to get readlock for 'FLUSH TABLES WITH READ LOCK'" and #57006 "Deadlock between HANDLER and FLUSH TABLES WITH READ LOCK". Work-in-progress. Extend test coverage for special kinds of SET statement. modified: mysql-test/r/flush_read_lock.result mysql-test/t/flush_read_lock.test === modified file 'sql/lock.cc' --- a/sql/lock.cc 2010-10-25 15:16:12 +0000 +++ b/sql/lock.cc 2010-10-26 04:25:15 +0000 @@ -1145,36 +1145,6 @@ void Global_read_lock::move_tickets_afte thd->mdl_context.move_ticket_after_trans_sentinel(m_mdl_blocks_commits_lock); } - -/** - Acquire metadata lock protecting statement against global read lock - and move it after transactional sentinel, so it won't be released - in the middle of statement execution. - - @param thd Reference to thread. - - @retval non-NULL - Ticket for metadata lock providing protection. - @retval NULL - Error. -*/ - -MDL_ticket* Global_read_lock::acquire_explicit_protection(THD *thd) -{ - MDL_request mdl_request; - - if (can_acquire_protection()) - return NULL; - - init_protection_request(&mdl_request); - - if (thd->mdl_context.acquire_lock(&mdl_request, - thd->variables.lock_wait_timeout)) - return NULL; - - thd->mdl_context.move_ticket_after_trans_sentinel(mdl_request.ticket); - - return mdl_request.ticket; -} - /** @} (end of group Locking) */ === modified file 'sql/sql_class.h' --- a/sql/sql_class.h 2010-10-22 07:16:34 +0000 +++ b/sql/sql_class.h 2010-10-26 04:25:15 +0000 @@ -1350,7 +1350,6 @@ public: bool make_global_read_lock_block_commit(THD *thd); bool is_acquired() const { return m_state != GRL_NONE; } void move_tickets_after_trans_sentinel(THD *thd); - MDL_ticket *acquire_explicit_protection(THD *thd); private: enum_grl_state m_state; /** @@ -3507,29 +3506,10 @@ public: #define CF_DIAGNOSTIC_STMT (1U << 8) /** - SQL statements that require explicit protection against impending - global read lock. - For most of DML and DDL statements metadata lock providing protection - will be acquired automatically at the time when tables are opened/ - metadata locks are acquired. This lock is released at the end of - statement execution. - Unfortunately, a few DDL statements release metadata locks too early - and so this flag has to be used. For statements marked by this flag - we acquire metadata lock providing protection against GRL early during - statement execution and protect it from premature release by moving - corresponding ticket behind transactional sentinel. We release this - lock at the end of statement execution. - - Usage of this flag is discouraged as it this hack should be removed - long-term. -*/ -#define CF_EXPLICIT_PROTECT_AGAINST_GRL (1U << 9) - -/** Identifies statements that may generate row events and that may end up in the binary log. */ -#define CF_CAN_GENERATE_ROW_EVENTS (1U << 10) +#define CF_CAN_GENERATE_ROW_EVENTS (1U << 9) /* Bits in server_command_flags */ === modified file 'sql/sql_parse.cc' --- a/sql/sql_parse.cc 2010-10-25 18:11:22 +0000 +++ b/sql/sql_parse.cc 2010-10-26 04:25:15 +0000 @@ -362,14 +362,10 @@ void init_update_queries(void) sql_command_flags[SQLCOM_REVOKE]= CF_CHANGES_DATA; sql_command_flags[SQLCOM_OPTIMIZE]= CF_CHANGES_DATA; sql_command_flags[SQLCOM_CREATE_FUNCTION]= CF_CHANGES_DATA; - sql_command_flags[SQLCOM_CREATE_PROCEDURE]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS | - CF_EXPLICIT_PROTECT_AGAINST_GRL; - sql_command_flags[SQLCOM_CREATE_SPFUNCTION]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS | - CF_EXPLICIT_PROTECT_AGAINST_GRL; - sql_command_flags[SQLCOM_DROP_PROCEDURE]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS | - CF_EXPLICIT_PROTECT_AGAINST_GRL; - sql_command_flags[SQLCOM_DROP_FUNCTION]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS | - CF_EXPLICIT_PROTECT_AGAINST_GRL; + sql_command_flags[SQLCOM_CREATE_PROCEDURE]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS; + sql_command_flags[SQLCOM_CREATE_SPFUNCTION]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS; + sql_command_flags[SQLCOM_DROP_PROCEDURE]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS; + sql_command_flags[SQLCOM_DROP_FUNCTION]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS; sql_command_flags[SQLCOM_ALTER_PROCEDURE]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS; sql_command_flags[SQLCOM_ALTER_FUNCTION]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS; sql_command_flags[SQLCOM_INSTALL_PLUGIN]= CF_CHANGES_DATA; @@ -1765,7 +1761,6 @@ mysql_execute_command(THD *thd) /* have table map for update for multi-update statement (BUG#37051) */ bool have_table_map_for_update= FALSE; #endif - MDL_ticket *grl_protection= NULL; DBUG_ENTER("mysql_execute_command"); #ifdef WITH_PARTITION_STORAGE_ENGINE thd->work_part_info= 0; @@ -1957,18 +1952,6 @@ mysql_execute_command(THD *thd) thd->mdl_context.release_transactional_locks(); } - /* - Check if this command needs explicit protection against the global - read lock. See comment for CF_EXPLICIT_PROTECT_AGAINST_GRL flag for - details. We release metadata lock providing protection at the end - of mysql_execute_command(). - */ - if (((sql_command_flags[lex->sql_command] & - CF_EXPLICIT_PROTECT_AGAINST_GRL) != 0) && - !thd->locked_tables_mode && - !(grl_protection= thd->global_read_lock.acquire_explicit_protection(thd))) - goto error; - #ifndef DBUG_OFF if (lex->sql_command != SQLCOM_SET_OPTION) DEBUG_SYNC(thd,"before_execute_sql_command"); @@ -3733,13 +3716,22 @@ end_with_restore_list: Security_context *backup= NULL; LEX_USER *definer= thd->lex->definer; /* - We're going to issue an implicit GRANT statement. - It takes metadata locks and updates system tables. - Make sure that sp_create_routine() did not leave any - locks in the MDL context, so there is no risk to - deadlock. + We're going to issue an implicit GRANT statement so we close all + open tables. We have to keep metadata locks as this ensures that + this statement is atomic against concurent FLUSH TABLES WITH READ + LOCK. Deadlocks which can arise due to fact that this implicit + statement takes metadata locks should be detected by a deadlock + detector in MDL subsystem and reported as errors. + + No need to commit/rollback statement transaction, it's not started. + + TODO: Long-term we should either ensure that implicit GRANT statement + is written into binary log as a separate statement or make both + creation of routine and implicit GRANT parts of one fully atomic + statement. */ - close_mysql_tables(thd); + DBUG_ASSERT(thd->transaction.stmt.is_empty()); + close_thread_tables(thd); /* Check if the definer exists on slave, then use definer privilege to insert routine privileges to mysql.procs_priv. @@ -3999,13 +3991,22 @@ create_sp_error: #ifndef NO_EMBEDDED_ACCESS_CHECKS /* - We're going to issue an implicit REVOKE statement. - It takes metadata locks and updates system tables. - Make sure that sp_create_routine() did not leave any - locks in the MDL context, so there is no risk to - deadlock. + We're going to issue an implicit REVOKE statement so we close all + open tables. We have to keep metadata locks as this ensures that + this statement is atomic against concurent FLUSH TABLES WITH READ + LOCK. Deadlocks which can arise due to fact that this implicit + statement takes metadata locks should be detected by a deadlock + detector in MDL subsystem and reported as errors. + + No need to commit/rollback statement transaction, it's not started. + + TODO: Long-term we should either ensure that implicit REVOKE statement + is written into binary log as a separate statement or make both + dropping of routine and implicit REVOKE parts of one fully atomic + statement. */ - close_mysql_tables(thd); + DBUG_ASSERT(thd->transaction.stmt.is_empty()); + close_thread_tables(thd); if (sp_result != SP_KEY_NOT_FOUND && sp_automatic_privileges && !opt_noacl && @@ -4334,13 +4335,6 @@ finish: DEBUG_SYNC(thd, "execute_command_after_close_tables"); #endif - /* - It is time to release metadata lock protecting against global read - lock if it is one of statements which require explicit protection. - */ - if (grl_protection) - thd->mdl_context.release_lock(grl_protection); - if (stmt_causes_implicit_commit(thd, CF_IMPLICIT_COMMIT_END)) { /* No transaction control allowed in sub-statements. */ No bundle (reason: useless for push emails).