Hello Davi!
Here is my comments about your patch:
* Davi Arnaut <davi@stripped> [07/10/03 06:38]:
> ChangeSet@stripped, 2007-10-02 23:35:50-03:00, davi@stripped +4 -0
> Bug#21587 FLUSH TABLES causes server crash when used with HANDLER statements
...
> The current implementation also fails to properly discard handlers of
> dropped tables but this and other problems are going to be addressed
> in future bug reports and patches.
IMO it makes sense to mention bug number for this problem here
(without preceding # of course). Please report this bug if it
has not been reported yet.
...
> diff -Nrup a/sql/sql_handler.cc b/sql/sql_handler.cc
> --- a/sql/sql_handler.cc 2007-08-30 16:23:53 -03:00
> +++ b/sql/sql_handler.cc 2007-10-02 23:35:45 -03:00
> @@ -187,6 +182,7 @@ bool mysql_ha_open(THD *thd, TABLE_LIST
> char *db, *name, *alias;
> uint dblen, namelen, aliaslen, counter;
> int error;
> + TABLE *backup_open_tables, *backup_handler_tables;
> DBUG_ENTER("mysql_ha_open");
> DBUG_PRINT("enter",("'%s'.'%s' as '%s' reopen: %d",
> tables->db, tables->table_name, tables->alias,
> @@ -215,17 +211,30 @@ bool mysql_ha_open(THD *thd, TABLE_LIST
> }
> }
>
> + /* save open_ and handler_ tables state */
> + backup_open_tables= thd->open_tables;
> + backup_handler_tables= thd->handler_tables;
> +
> + /* no pre-opened tables */
> + thd->open_tables= NULL;
> + /* to avoid flushes */
> + thd->handler_tables= NULL;
> +
I have been contemplating if it makes sense to introduce couple of
macros HANDLER_TABLES_HACK_BEGIN and HANDLER_TABLES_HACK_END to which
we could move duplicated code manipulating with those lists. But since
a) we only have two places where we do this
b) it will to some extent hide from the reader what really happens here
c) this code is likely to change soon anyway (see my note about RENAME)
probably it isn't worth doing it.
> /*
> open_tables() will set 'tables->table' if successful.
> It must be NULL for a real open when calling open_tables().
> */
> DBUG_ASSERT(! tables->table);
> - HANDLER_TABLES_HACK(thd);
>
> /* for now HANDLER can be used only for real TABLES */
> tables->required_type= FRMTYPE_TABLE;
> error= open_tables(thd, &tables, &counter, 0);
> - HANDLER_TABLES_HACK(thd);
> +
> + /* restore the state and merge the opened table */
> + thd->handler_tables= thd->open_tables ?
> + thd->open_tables->next= backup_handler_tables,
> + thd->open_tables : backup_handler_tables;
> + thd->open_tables= backup_open_tables;
>
Indeed, such solution ensures that open_tables() won't close any
handler tables except the one which is being opened and therefore
ensures that references for them stay valid and thus fixes the
crash.
But there is another related problem which is still not fixed.
Imagine that we try to open table for HANDLER statement and
encounter pending name-lock on it. In this case open_tables() above
will wait until this name-lock goes away. But if connection which
holds this name-lock also needs name-lock on some other table which
is already open as HANDLER table in current connection this wait
will be indefinite. Such situation, for example, can be easily
repeated by using version of your test case in which FLUSH TABLES
command replaced with RENAME TABLE t1 TO t2 (and without your
patch we will also get crash in this case).
Since this problem has slightly different cause and your current
patch already improves situation I suggest to report it as a
separate bug and fix in another patch.
...
> @@ -428,9 +438,29 @@ bool mysql_ha_read(THD *thd, TABLE_LIST
> }
> tables->table=table;
>
> - HANDLER_TABLES_HACK(thd);
> - lock= mysql_lock_tables(thd, &tables->table, 1, 0, ¬_used);
> - HANDLER_TABLES_HACK(thd);
> + /* save open_ and handler_ tables state */
> + backup_open_tables= thd->open_tables;
> + backup_handler_tables= thd->handler_tables;
> +
> + /* no pre-opened tables */
> + thd->open_tables= NULL;
> + /* to avoid flushes */
> + thd->handler_tables= NULL;
> +
> + lock= mysql_lock_tables(thd, &tables->table, 1,
> + MYSQL_LOCK_NOTIFY_IF_NEED_REOPEN, &need_reopen);
> +
> + /* restore previous context */
> + thd->handler_tables= backup_handler_tables;
> + thd->open_tables= backup_open_tables;
> +
> + if (need_reopen)
> + {
> + mysql_ha_close_table(thd, tables);
> + hash_tables->table= NULL;
> + close_thread_tables(thd);
> + goto retry;
> + }
AFAIU this call to close_thread_tables() does nothing. Please either
remove it or explain why it is needed.
...
I think it is OK to push this patch into 5.0 after fixing or discussing
above issues.
--
Dmitri Lenev, Software Developer
MySQL AB, www.mysql.com
Are you MySQL certified? http://www.mysql.com/certification
