Re: bk commit into 5.0 tree (kostja:1.2477) BUG#24918

[Sergei Golubchik]

Hi!

On Jul 24, konstantin@stripped wrote:
> ChangeSet@stripped, 2007-07-24 22:52:18+04:00, kostja@bodhi.(none) +9 -0
>   A fix and a test case for Bug#24918 drop table and lock / inconsistent
>   between perm and temp tables

A couple of questions and style comments below.
otherwise looks ok.

>   This patch introduces no changes in behavior -- the discrepancy in
>   behavior will be fixed when we start calling
>   ::store_lock()/::external_lock() for all tables, regardless whether
>   they are transactional or not, temporary or not.

So, what will happen with this bug when you push ? cannot be closed,
right ? you didn't fix the sympthoms in the original bugreport.
 
> diff -Nrup a/sql/lock.cc b/sql/lock.cc
> --- a/sql/lock.cc	2007-06-01 12:54:30 +04:00
> +++ b/sql/lock.cc	2007-07-24 22:52:15 +04:00
> @@ -151,7 +151,8 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, 
>      }
>  
>      thd->proc_info="System lock";
> -    if (lock_external(thd, tables, count))
> +    if (sql_lock->table_count && lock_external(thd, sql_lock->table,
> +                                               sql_lock->table_count))

Why ? Am I correct that this is just an optimization, not critical for
correctness of the code ?

>      {
>        /* Clear the lock type of all lock data to avoid reusage. */
>        reset_lock_data(sql_lock);
> @@ -353,10 +354,28 @@ void mysql_lock_remove(THD *thd, MYSQL_LOCK
>  }
>  
>  
> +/**
> +  Try to find the table in the list of locked tables.
> +  In case of success, unlock the table and remove it from this list.
> +
> +  @note This function has a legacy side effect: the table is
> +  unlocked even if it is not found in the locked list.

Could that happen ?

> +  It's not clear if this side effect is intentional or still
> +  desirable. It might lead to unmatched calls to
> +  unlock_external(). Moreover, a discrepancy can be left
> +  unnoticed by the storage engine, because in
> +  unlock_external() we call handler::external_lock(F_UNLCK) only
> +  if table->current_lock is not F_UNLCK.
> +
> +  @param  always_unlock   specify explicitly if the legacy side
> +                          effect is desired.
> +*/
> +
> -void mysql_lock_remove(THD *thd, MYSQL_LOCK *locked,TABLE *table)
> +void mysql_lock_remove(THD *thd, MYSQL_LOCK *locked,TABLE *table,
> +                       bool always_unlock)
>  {
> -  mysql_unlock_some_tables(thd, &table,1);
> +  if (always_unlock == TRUE)
> +    mysql_unlock_some_tables(thd, &table, /* table count */ 1);
>    if (locked)
>    {
>      reg1 uint i;
> diff -Nrup a/sql/sql_base.cc b/sql/sql_base.cc
> --- a/sql/sql_base.cc	2007-07-20 19:46:11 +04:00
> +++ b/sql/sql_base.cc	2007-07-24 22:52:16 +04:00
> @@ -2343,7 +2387,10 @@ bool drop_locked_tables(THD *thd,const c
>      if (!strcmp(table->s->table_name, table_name) &&
>  	!strcmp(table->s->db, db))
>      {
> -      mysql_lock_remove(thd, thd->locked_tables,table);
> +      mysql_lock_remove(thd, thd->locked_tables, table,
> +                        /* Unlock the table even if it is not
> +                          found in the list of locked tables. */
> +                        TRUE);

If you absolutely must have this last argument documented, instead of
adding a comment to the every call, give it a better name, for example:

#define UNLOCK_EVEN_IF_NOT_IN_THE_LOCKED_LIST TRUE

>        VOID(hash_delete(&open_cache,(byte*) table));
>        found=1;
>      }
> diff -Nrup a/sql/sql_class.h b/sql/sql_class.h
> --- a/sql/sql_class.h	2007-07-21 17:52:13 +04:00
> +++ b/sql/sql_class.h	2007-07-24 22:52:16 +04:00
> @@ -997,11 +997,18 @@ class Open_tables_state
>  public:
>    /*
>      open_tables - list of regular tables in use by this thread
> -    temporary_tables - list of temp tables in use by this thread
>      handler_tables - list of tables that were opened with HANDLER OPEN
>       and are still in use by this thread
>    */
> -  TABLE *open_tables, *temporary_tables, *handler_tables, *derived_tables;
> +  TABLE *open_tables, *handler_tables, *derived_tables;
> +  /**
> +    List of temporary tables used by this thread. Contains user-level
> +    temporary tables, created with CREATE TEMPORARY TABLE, and
> +    internal temporary tables, created, e.g., to resolve a SELECT,
> +    or for an intermediate table used in ALTER.
> +    XXX Why are internal temporary tables added to this list?
> +  */
> +  TABLE *temporary_tables;

Generally it's best to avoid moving members in the class when adding
comments. Otherwise constructors that initialize them will start to
trigger compiler warnings.

>    /*
>      During a MySQL session, one can lock tables in two modes: automatic

Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Developer
/_/  /_/\_, /___/\___\_\___/  MySQL GmbH, Radlkoferstr. 2, D-81373 München
       <___/                  Geschäftsführer: Kaj Arnö - HRB
München 162140