List:Commits« Previous MessageNext Message »
From:Konstantin Osipov Date:February 20 2008 4:14pm
Subject:Bug#23588 code review
View as plain text  
Hello Sergey,

> ChangeSet@stripped, 2007-12-17 14:52:09+04:00, gluh@stripped +3 -0
>   Bug#23588 SHOW COLUMNS on a temporary table causes locking issues
>   skip lock_type update for temporary tables

The patch is OK to push but please see one item below.

> --- a/sql/sql_base.cc	2007-11-22 05:34:24 +04:00
> +++ b/sql/sql_base.cc	2007-12-17 14:52:07 +04:00
> @@ -2851,7 +2851,8 @@ int open_tables(THD *thd, TABLE_LIST **s
>        free_root(&new_frm_mem, MYF(MY_KEEP_PREALLOC));
>      }
>  
> -    if (tables->lock_type != TL_UNLOCK && ! thd->locked_tables)
> +    if (tables->table->s->tmp_table == NO_TMP_TABLE &&
> +        tables->lock_type != TL_UNLOCK && ! thd->locked_tables)
>        tables->table->reginfo.lock_type= tables->lock_type ==
> TL_WRITE_DEFAULT ?
> 
>      if (tables->lock_type == TL_WRITE_DEFAULT)
>        tables->table->reginfo.lock_type= tables->lock_type=
> thd->update_lock_default;
>          thd->update_lock_default : tables->lock_type;
>      tables->table->grant= tables->grant;

I think that never updating the lock type for temporary tables
will lead to an undesirable side effect -- TL_WRITE_DEFAULT will
be passed to handler::store_lock for transactional temporary
tables locked with LOCK TABLES.

I suggest to change this if in the following way:

if (tables->lock_type == TL_WRITE_DEFAULT)
  tables->table->reginfo.lock_type= tables->lock_type=
thd->update_lock_default;
else if (tables->lock_type != TL_UNLOCK && ! thd->locked_tables &&
         tables->s->tmp_table == NO_TMP_TABLE)
  tables->table->reginfo.lock_type= tables->lock_type;

The patch is OK to push if the suggestion is correct.

Thanks for working on this,

-- 
-- Konstantin Osipov              Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com   The best DATABASE COMPANY in the GALAXY
Thread
Bug#23588 code reviewKonstantin Osipov20 Feb