Hi Konstantin,
(off topic: we have a performance problem with the new MERGE
implementation. The test case for Bug#33362 (mysqld hangs on select) has
9600 child tables. A debug server on my machine opens about 5 tables per
second in average. More at start of the open and less towards the end of
the 9600 tables. In total opening the 9600 children took about 30
minutes (LOCK TABLE READ) on a debug server (but without --debug). In
5.0 it was just one and a half minute. Non-debug servers took 13 and 10
seconds respectively. Repeating the statements (all frm files in OS
filesystem cache) I got 0.01 and 10 seconds.)
Also we might want to document that --table-open-cache and
--table-definition-cache should take the children into account in 5.1.
Now to the review. I am not done yet, but I wanted to give you an
intermediate update. Still to do are some tests suggested by you and the
"less important comments" from you.
Konstantin Osipov, 29.02.2008 23:10:
...
> Unfortunately, I found two crashes, and I don't see
> an easy solution for them.
> The crashes stem from where the new functionality is invoked --
> attach isn't always followed by detach or can happen twice.
...
> But what's worse, it can lead to a premature detach, as with this
> test case:
>
> drop table if exists trg, t1,t2,t3,t4,t5,tm;
> drop function if exists f1;
> drop view if exists v1;
> create table t1 (a int);
> create table t2 (a int);
> insert into t1 (a) values (1);
> create table tm (a int) union (t1,t2) insert_method=first engine=mrg_myisam;
> create table t3 (a int);
> create table t4 (a int);
> create table t5 (a int);
> insert into t5 (a) values (1);
> create function f1() returns int return (select max(a) from t5);
> create view v1 as select foo.a a, f1() b, bar.a c, f1() d from tm foo, tm
> bar,t5;
> select * from v1;
>
> It crashes this way:
> mysqld: ha_myisammrg.cc:939: virtual int ha_myisammrg::rnd_next(uchar*): Assertion
> `this->file->children_attached' failed.
>
> In other words, if a merge table is used in a pre-locked
> statement, close_thread_tables() is invoked for it also inside a
> sub-statement. Since close_thread_tables() detaches all children,
> even those of the tables used in the main statement, an assert
> fires when the table is used in the main statement.
I think, the following fixes it (proved by test suite):
+++ edited/sql/sql_base.cc 2008-04-27 12:19:49 +02:00
@@ -1275,12 +1275,20 @@ void close_thread_tables(THD *thd)
/* Detach MERGE children after every statement. Even under LOCK
TABLES. */
for (table= thd->open_tables; table; table= table->next)
- if (table->db_stat)
+ {
+ /* Table might be in use by some outer statement. */
+ DBUG_PRINT("tcache", ("table: '%s' query_id: %lu",
+ table->s->table_name.str, (ulong)
table->query_id));
+ if (table->db_stat && (!prelocked_mode ||
+ (table->query_id == thd->query_id)))
{
DBUG_ASSERT(table->file);
VOID(table->file->extra(HA_EXTRA_DETACH_CHILDREN));
}
+ }
>
> Solution for this problem depends on a design choice:
> - do we attach and detach merge children for top level statement
> only, or individually for each sub-statement?
...
> When executing select, should we:
>
> - attach children of tm2 at the same time as children of tm1,
> i.e. in the open_tables run at start of the statement or
>
> - attach children of tm1 at start of the statement, and
> attach/detach children of tm2 for each invocation of the
> stored function?
>
> The problem is complicated by a difference in behavior of temporary
> and base tables. In your current patch, children of a temporary
> table are attached and detached for every sub-statement.
If this is true, then open_tables() is called for temporary tables for
every sub-statement. I call attach_children only from open_tables().
Likewise detach_children is called from close_thread_table() and
close_thread_tables() only. The former is not called for temporary
tables AFAIK.
But when open_tables()/close_thread_tables() is called for every
sub-statement, why is attach/detach not done for base tables too?
> Unfortunately, an attempt sometimes is made to attach children twice,
> which leads to a crash:
>
> create temporary table t1 (a int);
> create temporary table t2 (a int);
> create temporary table tm (a int);
> union (t1,t2) insert_method=first engine=merge;
> create function f1() returns int return (select max(a) from tm);
> insert into tm (a) values (1);
> select f1() from (select 1) as a;
>
> Crashes in:
>
> Version: '5.1.24-rc-valgrind-max-debug' socket: '/opt/local/var/mysql/mysql.sock'
> port: 3307 Source distribution
> mysqld: sql_base.cc:4037: int add_merge_table_list(TABLE_LIST*): Assertion
> `!parent->children_attached' failed.
The problem here is that the temporary TABLE object is opened with all
other tables at statement begin and then reused for the function.
This situation should be easily fixed by detaching the children on
open_table() when the TABLE object is "reset":
+++ edited/sql/sql_base.cc 2008-04-27 11:17:11 +02:00
@@ -2944,6 +2944,9 @@ TABLE *open_table(THD *thd, TABLE_LIST *
table_list->updatable= 1; // It is not derived table nor
non-updatable VIEW
table->clear_column_bitmaps();
DBUG_ASSERT(table->key_read == 0);
+ /* Temporary tables may be reused in a sub statement. */
+ if (table->s->tmp_table &&
table->file->extra(HA_EXTRA_IS_ATTACHED_CHILDREN))
+ table->file->extra(HA_EXTRA_DETACH_CHILDREN);
DBUG_RETURN(table);
}
This fixes the crash. Re-attaching for base tables can be implemented
later anyway. But that's outside of the MERGE patch, I think.
...
> So at least for temporary tables it seems we have no choice but
> attach and detach merge children at each sub-statement.
>
> If we decide to treat temporary and base tables differently, we
> should document this very clearly in the code.
Again, I don't see how I made it differently. I added stuff to
open_tables() and close_thread_table[s](), where I attach/detach all
MERGE tables.
>
> Alternatively, we could consider attaching and detaching base
> tables for each sub-statement as well.
> Unfortunately, it's not that easy to do.
> For base tables table->query_id is not always tracked, so we are
> bound to have different code paths for normal statements
> and sub-statements or statements under LOCK TABLES.
>
> Perhaps the easiest way, and best long-term, is to start
> assigning/tracking table->query_id in non-prelocked/lock-tables
> mode.
I didn't make any attempt to change the gross behavior here. I just
fixed the crashes.
>
> Whichever solution you decide to choose, the following cases
> should be considered (and covered with tests):
To be done.
...
> An issue I did not get to is how addition/removal of TABLE_LIST
> elements from the global list at detach interacts with
> pre-locking.
I didn't understand prelocking very well yet. One of the above findings
was that it didn't work with prelocking correctly.
Can you please point me at detailed documentation about prelocking?
...
Other "less important" things to be done.
Regards
Ingo
--
Ingo Strüwing, Senior Software Developer
MySQL GmbH, Dachauer Str. 37, D-80335 München
Geschäftsführer: Kaj Arnö - HRB München 162140
