List:Commits« Previous MessageNext Message »
From:Konstantin Osipov Date:March 9 2010 9:58pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (davi:3119)
Bug#33669
View as plain text  
Hi,

I see some pretty old gotchas are being fixed :-)

* Davi Arnaut <Davi.Arnaut@stripped> [10/03/08 21:49]:

> === modified file 'mysql-test/r/read_only_innodb.result'
> --- a/mysql-test/r/read_only_innodb.result	2009-12-09 15:56:34 +0000
> +++ b/mysql-test/r/read_only_innodb.result	2010-03-08 18:43:02 +0000
> @@ -46,3 +46,73 @@ UNLOCK TABLES;
>  DROP TABLE t1;
>  DROP USER test@localhost;
>  echo End of 5.1 tests 
> +#
> +# Bug#33669: Transactional temporary tables do not work under --read-only
> +#
> +DROP DATABASE IF EXISTS db1;
> +# Setup user and tables
> +CREATE USER bug33669@localhost;
> +CREATE DATABASE db1;
> +CREATE TABLE db1.t1 (a INT) ENGINE=INNODB;
> +CREATE TABLE db1.t2 (a INT) ENGINE=INNODB;
> +INSERT INTO db1.t1 VALUES (1);
> +INSERT INTO db1.t2 VALUES (2);
> +GRANT CREATE TEMPORARY TABLES, DROP, INSERT,
> +SELECT, LOCK TABLES ON db1.* TO bug33669@localhost;
> +SET GLOBAL READ_ONLY = ON;
> +# Connection con1 (user bug33669):
> +# Create, insert and drop temporary table:
> +CREATE TEMPORARY TABLE temp (a INT) ENGINE=INNODB;
> +INSERT INTO temp VALUES (1);
> +DROP TABLE temp;
> +# Lock base tables and use temporary table:
> +CREATE TEMPORARY TABLE temp (a INT) ENGINE=INNODB;
> +LOCK TABLES t1 READ, t2 READ;
> +SELECT * FROM t1;
> +a
> +1
> +INSERT INTO temp values (1);
> +SELECT * FROM t2;
> +a
> +2
> +UNLOCK TABLES;
> +DROP TABLE temp;
> +# Transaction
> +BEGIN;
> +SELECT * FROM t1;
> +a
> +1
> +CREATE TEMPORARY TABLE temp (a INT) ENGINE=INNODB;
> +INSERT INTO t1 VALUES (1);
> +ERROR HY000: The MySQL server is running with the --read-only option so it cannot
> execute this statement
> +INSERT INTO temp VALUES (1);
> +SELECT * FROM t2;
> +a
> +2
> +ROLLBACK;
> +SELECT * FROM temp;
> +a
> +DROP TABLE temp;
> +# Lock base table as READ and temporary table as WRITE:
> +CREATE TEMPORARY TABLE temp (a INT) ENGINE=INNODB;
> +LOCK TABLES t1 READ, temp WRITE;
> +SELECT * FROM t1;
> +a
> +1
> +SELECT * FROM temp;
> +a
> +INSERT INTO t1 VALUES (1);
> +ERROR HY000: The MySQL server is running with the --read-only option so it cannot
> execute this statement
> +INSERT INTO temp VALUES (1);
> +DROP TABLE temp;
> +UNLOCK TABLES;
> +# Lock temporary table that shadows a base table.
> +CREATE TEMPORARY TABLE t1 (a INT) ENGINE=INNODB;
> +LOCK TABLES t1 WRITE;
> +DROP TABLE t1;
> +SELECT * FROM t1;
> +ERROR HY000: Table 't1' was not locked with LOCK TABLES
> +# Disconnect and cleanup
> +SET GLOBAL READ_ONLY = OFF;
> +DROP USER bug33669@localhost;
> +DROP DATABASE db1;

Please add a test case for multi-insert into two temporary tables,
a mix of tmp and base tables,
multi-update, insert-select (select from a non-temp), an insert
that uses a subquery that is not using a temp table,
an  insert <tmp> ... select <base> from (subquery <base>).

> -int mysql_lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags)
> +static int
> +lock_tables_check(THD *thd, TABLE **tables, uint count,
> +                  bool *write_lock_used, uint flags)
>  {
> +  bool superuser;

Please consider calling the variable is_superuser.

>    bool log_table_write_query;
>    uint system_count;
>    uint i;

OK, I haven't yet removed the loop from mysql_lock_tables(),
but it currently almost never works in 5.5, so moving the check
outside the loop is OK.


> -  DBUG_ENTER("mysql_lock_tables_check");
> +  DBUG_ENTER("lock_tables_check");
>  
>    system_count= 0;
> +  *write_lock_used= FALSE;
> +  superuser= thd->security_ctx->master_access & SUPER_ACL;
>    log_table_write_query= (is_log_table_write_query(thd->lex->sql_command)
>                           || ((flags & MYSQL_LOCK_PERF_SCHEMA) != 0));
>  
> @@ -148,10 +153,18 @@ int mysql_lock_tables_check(THD *thd, TA
>        }
>      }
>  
> -    if ((t->s->table_category == TABLE_CATEGORY_SYSTEM) &&
> -        (t->reginfo.lock_type >= TL_WRITE_ALLOW_WRITE))
> +    if (t->reginfo.lock_type >= TL_WRITE_ALLOW_WRITE)
>      {
> -      system_count++;
> +      *write_lock_used= TRUE;
> +
> +      if (t->s->table_category == TABLE_CATEGORY_SYSTEM)
> +        system_count++;
> +
> +      if (t->db_stat & HA_READ_ONLY)
> +      {
> +        my_error(ER_OPEN_AS_READONLY, MYF(0), t->alias);
> +        DBUG_RETURN(1);
> +      }
>      }
>  
>      /*
> @@ -172,6 +185,20 @@ int mysql_lock_tables_check(THD *thd, TA
>                   thd->mdl_context.is_lock_owner(MDL_key::TABLE,
>                                    t->s->db.str, t->s->table_name.str,
>                                    MDL_SHARED)));
> +
> +    /*
> +      Prevent modifications to base tables if READ_ONLY is activated.
> +      In any case, read only does not apply to temporary tables.
> +    */
> +    if (!(flags & MYSQL_LOCK_IGNORE_GLOBAL_READ_ONLY) &&
> !t->s->tmp_table)
> +    {
> +      if (t->reginfo.lock_type >= TL_WRITE_ALLOW_WRITE &&
> +          !superuser && opt_readonly && !thd->slave_thread)
> +      {
> +        my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
> +        DBUG_RETURN(1);
> +      }
> +    }
>    }
>  
>    /*
> @@ -267,15 +294,15 @@ static void reset_lock_data_and_free(MYS
>  MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count,
>                                uint flags, bool *need_reopen)
>  {
> -  MYSQL_LOCK *sql_lock;
> -  TABLE *write_lock_used;
>    int rc;
> +  MYSQL_LOCK *sql_lock;
> +  bool write_lock_used;
>  
>    DBUG_ENTER("mysql_lock_tables");
>  
>    *need_reopen= FALSE;
>  
> -  if (mysql_lock_tables_check(thd, tables, count, flags))
> +  if (lock_tables_check(thd, tables, count, &write_lock_used, flags))
>      DBUG_RETURN (NULL);
>  
>    ulong timeout= (flags & MYSQL_LOCK_IGNORE_TIMEOUT) ?
> @@ -283,8 +310,7 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, 
>  
>    for (;;)
>    {
> -    if (! (sql_lock= get_lock_data(thd, tables, count, GET_LOCK_STORE_LOCKS,
> -                                   &write_lock_used)))
> +    if (! (sql_lock= get_lock_data(thd, tables, count, GET_LOCK_STORE_LOCKS)))
>        break;
>  
>      if (global_read_lock && write_lock_used &&
> @@ -308,21 +334,6 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, 
>        }
>      }
>  
> -    if (!(flags & MYSQL_LOCK_IGNORE_GLOBAL_READ_ONLY) &&
> -        write_lock_used &&
> -        opt_readonly &&
> -        !(thd->security_ctx->master_access & SUPER_ACL) &&
> -        !thd->slave_thread)
> -    {
> -      /*
> -	Someone has issued SET GLOBAL READ_ONLY=1 and we want a write lock.
> -        We do not wait for READ_ONLY=0, and fail.
> -      */
> -      reset_lock_data_and_free(&sql_lock);
> -      my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
> -      break;
> -    }
> -
>      thd_proc_info(thd, "System lock");
>      DBUG_PRINT("info", ("thd->proc_info %s", thd->proc_info));
>      if (sql_lock->table_count && lock_external(thd, sql_lock->table,
> @@ -459,9 +470,7 @@ void mysql_unlock_tables(THD *thd, MYSQL
>  void mysql_unlock_some_tables(THD *thd, TABLE **table,uint count)
>  {
>    MYSQL_LOCK *sql_lock;
> -  TABLE *write_lock_used;
> -  if ((sql_lock= get_lock_data(thd, table, count, GET_LOCK_UNLOCK,
> -                               &write_lock_used)))
> +  if ((sql_lock= get_lock_data(thd, table, count, GET_LOCK_UNLOCK)))
>      mysql_unlock_tables(thd, sql_lock);
>  }
>  
> @@ -603,9 +612,7 @@ void mysql_lock_downgrade_write(THD *thd
>                                  thr_lock_type new_lock_type)
>  {
>    MYSQL_LOCK *locked;
> -  TABLE *write_lock_used;
> -  if ((locked = get_lock_data(thd, &table, 1, GET_LOCK_UNLOCK,
> -                              &write_lock_used)))
> +  if ((locked = get_lock_data(thd, &table, 1, GET_LOCK_UNLOCK)))
>    {
>      for (uint i=0; i < locked->lock_count; i++)
>        thr_downgrade_write_lock(locked->locks[i], new_lock_type);
> @@ -619,11 +626,9 @@ void mysql_lock_downgrade_write(THD *thd
>  void mysql_lock_abort(THD *thd, TABLE *table, bool upgrade_lock)
>  {
>    MYSQL_LOCK *locked;
> -  TABLE *write_lock_used;
>    DBUG_ENTER("mysql_lock_abort");
>  
> -  if ((locked= get_lock_data(thd, &table, 1, GET_LOCK_UNLOCK,
> -                             &write_lock_used)))
> +  if ((locked= get_lock_data(thd, &table, 1, GET_LOCK_UNLOCK)))
>    {
>      for (uint i=0; i < locked->lock_count; i++)
>        thr_abort_locks(locked->locks[i]->lock, upgrade_lock);
> @@ -648,12 +653,10 @@ void mysql_lock_abort(THD *thd, TABLE *t
>  bool mysql_lock_abort_for_thread(THD *thd, TABLE *table)
>  {
>    MYSQL_LOCK *locked;
> -  TABLE *write_lock_used;
>    bool result= FALSE;
>    DBUG_ENTER("mysql_lock_abort_for_thread");
>  
> -  if ((locked= get_lock_data(thd, &table, 1, GET_LOCK_UNLOCK,
> -                             &write_lock_used)))
> +  if ((locked= get_lock_data(thd, &table, 1, GET_LOCK_UNLOCK)))
>    {
>      for (uint i=0; i < locked->lock_count; i++)
>      {
> @@ -848,11 +851,10 @@ static int unlock_external(THD *thd, TAB
>    @param flags		    One of:
>             - GET_LOCK_UNLOCK      : If we should send TL_IGNORE to store lock
>             - GET_LOCK_STORE_LOCKS : Store lock info in TABLE
> -  @param write_lock_used   Store pointer to last table with WRITE_ALLOW_WRITE
>  */
>  
>  static MYSQL_LOCK *get_lock_data(THD *thd, TABLE **table_ptr, uint count,
> -				 uint flags, TABLE **write_lock_used)
> +                                 uint flags)
>  {
>    uint i,tables,lock_count;
>    MYSQL_LOCK *sql_lock;
> @@ -861,9 +863,8 @@ static MYSQL_LOCK *get_lock_data(THD *th
>    DBUG_ENTER("get_lock_data");
>  
>    DBUG_ASSERT((flags == GET_LOCK_UNLOCK) || (flags == GET_LOCK_STORE_LOCKS));
> -
>    DBUG_PRINT("info", ("count %d", count));
> -  *write_lock_used=0;
> +
>    for (i=tables=lock_count=0 ; i < count ; i++)
>    {
>      TABLE *t= table_ptr[i];
> @@ -895,24 +896,12 @@ static MYSQL_LOCK *get_lock_data(THD *th
>    {
>      TABLE *table;
>      enum thr_lock_type lock_type;
> +    THR_LOCK_DATA **org_locks = locks;
>  
>      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 && lock_type !=
> TL_READ_DEFAULT);
> -    if (lock_type >= TL_WRITE_ALLOW_WRITE)
> -    {
> -      *write_lock_used=table;
> -      if (table->db_stat & HA_READ_ONLY)
> -      {
> -	my_error(ER_OPEN_AS_READONLY,MYF(0),table->alias);
> -        /* Clear the lock type of the lock data that are stored already. */
> -        sql_lock->lock_count= (uint) (locks - sql_lock->locks);
> -        reset_lock_data_and_free(&sql_lock);
> -	DBUG_RETURN(0);
> -      }
> -    }
> -    THR_LOCK_DATA **org_locks = locks;
>      locks_start= locks;
>      locks= table->file->store_lock(thd, locks,
>                                     (flags & GET_LOCK_UNLOCK) ? TL_IGNORE :


The patch is OK to push after fixing the above.
You should push into trunk-bugfixing, the new mdl is in the trunk.

Thread
bzr commit into mysql-next-mr-bugfixing branch (davi:3119) Bug#33669Davi Arnaut8 Mar
  • Re: bzr commit into mysql-next-mr-bugfixing branch (davi:3119)Bug#33669Konstantin Osipov9 Mar