Re: bk commit into 5.1 tree (davi:1.2597) BUG#30882

[Dmitri Lenev]

Hi Davi!

I have only a few minor comments about your patch:

* Davi Arnaut <davi@stripped> [07/10/31 00:10]:
> ChangeSet@stripped, 2007-10-30 19:02:22-02:00, davi@stripped +15 -0
>   Bug#30882 Dropping a temporary table inside a stored function may cause a server
> crash
>   
>   If a stored function that contains a drop temporary table statement
>   is invoked by a create temporary table of the same name may cause
>   a server crash. The problem is that when dropping a table no check
>   is done to ensure that table is not being used by some outer query
>   (or outer statement), potentially leaving the outer query with a
>   reference to a stale (freed) table.
>   
>   The solution is when dropping a temporary table, always check if
>   the table is being used by some outer statement as a temporary
>   table can be dropped inside stored procedures.
>   
>   The check is performed by looking at the query_id of the temporary
>   tables, it should always be zero for unused tables. But this was not
>   always the case because of a bug in prelocked mode that prevented the
>   query_id from being reset. Now, all temporary tables which were used by
>   a statement are marked as free for reuse after it's execution has been
>   completed.
> 

I think this comment is a bit misleading. IMO it is better explicitly say
something like:

  The check is performed by looking at the TABLE::query_id value for
  temporary tables. To simplify this check and to solve a bug related
  to handling of temporary tables in prelocked mode current patch
  changes the way in which this member is used to track the fact that
  table is used/unused. Now we ensure that TABLE::query_id is zero for
  unused temporary tables (which means that all temporary tables which
  were used by a statement should be marked as free for reuse after
  it's execution has been completed).

In other words, to state clearly that we have changed the way in which
TABLE::query_id is used for tracking that table is used/unused.

...

>   sql/sql_base.cc@stripped, 2007-10-30 19:02:19-02:00, davi@stripped +90 -50
>     When entering in prelocked mode, mark as free for reuse all temp
>     tables which are not in use by the main statement. Also re-factor
>     close_thread_tables() to not take LOCK_open unless there are open
>     tables.

Hmm... IMO this comment doesn't quite correctly reflect the changes in
sql_base.cc. How about replacing it with something like:

 Changed the approach to distinguishing currently unused temporary tables.
 Now we ensure that such tables always have TABLE::query_id set to 0 and
 use this fact to perform checks during opening and dropping of temporary
 tables. This means that we have to call close_thread_tables() even for
 statements which use only temporary tables. To make this call cheaper
 re-factored close_thread_tables() to not take LOCK_open unless there
 are open base tables.

?

...

>   sql/sql_parse.cc@stripped, 2007-10-30 19:02:20-02:00, davi@stripped +6 -9
>     The condition doesn't cover all the possibilities under which
>     close_thread_tables should be called. It's safe to unconditionally
>     call close_thread_tables as the function was refactored to take
>     LOCK_open only when necessary.

I think it is better to mention explicitly here that now close_thread_tables()
should be called even for statements that use only temporary tables. So the
condition in question no longer covers all cases.

...

> diff -Nrup a/sql/sql_base.cc b/sql/sql_base.cc
> --- a/sql/sql_base.cc	2007-10-29 12:07:15 -02:00
> +++ b/sql/sql_base.cc	2007-10-30 19:02:19 -02:00
> @@ -1054,6 +1054,29 @@ bool close_cached_connection_tables(THD 
>  }
>  
>  
> +/**
> +  Mark all temporary tables which were used by the current substatement or
                                                              ^^^^^^^^^^^^
Should be "statement" ?

> +  substatement as free for reuse, but only if the query_id can be cleared.
> +
> +  @param thd thread context
> +
> +  @remark For temp tables associated with a open SQL HANDLER the query_id
> +          is not reset until the HANDLER is closed.
> +*/

...

>  /*
>    Close all tables used by the current substatement, or all tables
>    used by this thread if we are on the upper level.
> @@ -1097,10 +1156,6 @@ static void mark_used_tables_as_free_for
>    SYNOPSIS
>      close_thread_tables()
>      thd			Thread handler
> -    lock_in_use		Set to 1 (0 = default) if caller has a lock on
> -			LOCK_open
> -    skip_derived	Set to 1 (0 = default) if we should not free derived
> -			tables.
>      stopper             When closing tables from thd->open_tables(->next)*, 
>                          don't close/remove tables starting from stopper.
>

Please remove description of non-existing 'stopper' argument as well.

...

> @@ -2295,7 +2336,7 @@ TABLE *open_table(THD *thd, TABLE_LIST *
>  	  DBUG_RETURN(0);
>  	}
>  	table->query_id= thd->query_id;
> -	table->clear_query_id= 1;
> +	table->open_by_handler= 0;
>  	thd->thread_specific_used= TRUE;
>          DBUG_PRINT("info",("Using temporary table"));
>          goto reset;

Do we really need this assignment? I think for temporary tables which are
not yet "opened" (i.e. used by statement) this flag is always zero.

...

> diff -Nrup a/sql/sql_parse.cc b/sql/sql_parse.cc
> --- a/sql/sql_parse.cc	2007-10-23 13:10:27 -02:00
> +++ b/sql/sql_parse.cc	2007-10-30 19:02:20 -02:00
> @@ -1336,12 +1334,11 @@ bool dispatch_command(enum enum_server_c
>      my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0));
>      break;
>    }
> -  if (thd->lock || thd->open_tables || thd->derived_tables ||
> -      thd->prelocked_mode)
> -  {
> -    thd->proc_info="closing tables";
> -    close_thread_tables(thd);			/* Free tables */
> -  }
> +
> +  thd->proc_info="closing tables";
                   ^^
Our coding style says that space is required after '='.

> diff -Nrup a/sql/sql_table.cc b/sql/sql_table.cc
> --- a/sql/sql_table.cc	2007-10-29 12:07:16 -02:00
> +++ b/sql/sql_table.cc	2007-10-30 19:02:20 -02:00
> @@ -1503,7 +1503,7 @@ int mysql_rm_table_part2(THD *thd, TABLE
>    char path[FN_REFLEN], *alias;
>    uint path_length;
>    String wrong_tables;
> -  int error;
> +  int error= 0;
>    int non_temp_tables_count= 0;
>    bool some_tables_deleted=0, tmp_table_deleted=0, foreign_key_error=0;
>    String built_query;
> @@ -1563,10 +1563,21 @@ int mysql_rm_table_part2(THD *thd, TABLE
>      enum legacy_db_type frm_db_type;
>  
>      mysql_ha_flush(thd, table, MYSQL_HA_CLOSE_FINAL, 1);
> -    if (!close_temporary_table(thd, table))
> -    {
> -      tmp_table_deleted=1;
> -      continue;					// removed temporary table
> +
> +    error= drop_temporary_table(thd, table);
> +
> +    switch (error) {
> +    case  0:
> +      // removed temporary table
> +      tmp_table_deleted= 1;
> +      continue;
> +    case -1:
> +      // table already in use
> +      error= 1;
> +      goto err_with_placeholders;

Hmm... In current conditions this branch can't be taken outside of 
SF/trigger/prelocked mode. So it does not matter that we return
from mysql_rm_table_part2() without trying to write anything to
binary log/to invalidate query cache. But may be it makes sense
to place an assert here which will ensure that we won't do this
outside of SF/trigger/prelocked mode (since in this case it
would be an error)?

> +    default:
> +      // temporary table not found
> +      error= 0;
>      }
>  
>      /*
> @@ -1593,7 +1604,6 @@ int mysql_rm_table_part2(THD *thd, TABLE
>        built_query.append("`,");
>      }
>  
> -    error=0;
>      table_type= table->db_type;
>      if (!drop_temporary)
>      {

I think it is OK to push this patch after fixing/discussing above issues.

-- 
Dmitri Lenev, Software Developer
MySQL AB, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification