* Ingo Struewing <ingo@stripped> [08/01/24 17:58]:
> ChangeSet@stripped, 2008-01-24 15:41:40+01:00, istruewing@stripped +16 -0
> WL#4144 - Lock MERGE engine children
>
> Step #1: Move locking from parent to children.
>
> MERGE children are now left in the query list of tables
> after inserted there in open_tables(). So they are locked
> by lock_tables() as all other tables are.
>
> The MERGE parent does not store locks any more. It appears
> in a MYSQL_LOCK with zero lock data. This is kind of a "dummy"
> lock.
Hello Ingo,
I am very happy with this patch. The new way of merge tables locking,
the encapsulation of attach/detach functionality inside merge handler,
the validation at attach, all look very good and allow to track
changes of merge children metadata remarkably well.
I'm tempted to say OK to push -- and look at the remaining
problems separately.
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.
Let me demonstrate:
Currently DETACH_CHILDREN is called for every locked table,
even for the one that is not used in the statement:
create table t1 (a int);
create table t2 like t1;
create table tm (a int) union (t1,t2) insert_method=first engine=mrg_myisam;
lock table tm;
select * from t1; -- calls extra(DETACH_CHILDREN) for t1,t2 and tm;
Even though it is redundant, it is OK.
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.
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?
Example:
create table t1 (a int);
create table t2 (a int);
create table t3 (a int);
create table t4 (a int);
create table tm1 (a int) union (t1,t2) engine=merge;
create table tm2 (a int) union (t3,t4) engine=merge;
create function f1() returns int return (select max(a) from tm2);
select f1() from tm1;
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.
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.
Yes, it also crashes in 5.1 without the patch for WL#4144.
We should keep in mind that with temporary tables a DDL can
happen inside a stored function or trigger.
I.e. we can have the following function:
create function f1() returns int
begin
drop temporary table if exists t1,t2,tm;
create temporary table t1 (a int);
create temporary table t2 (a int);
create temporary table tm (a int) union (t1,t2) engine=merge;
insert into t1 (a) values (1);
return (select max(a) from tm);
end|
select f1() from (select 1 union select 1) a;
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.
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.
Whichever solution you decide to choose, the following cases
should be considered (and covered with tests):
- how attach/detach work when there are no functions/triggers and
no LOCK TABLES (normal mode)
- how they work for a sub-statement that is run inside a function
- the two above, but under LOCK TABLES
- attach/detach for LOCK TABLES statement itself
- LOCK TABLES statement that locks a table has a trigger that
inserts into a merge table, so an attempt is made to lock
tables of a sub-statement.
- what is the difference in all above cases between behavior
of temporary and base tables.
This concludes the main part of my review. The issues below
are less important, I will present them to you since they were on
my plan to look at in your patch.
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.
Could it happen that a table that is a merge child is by
mistake also used by a pre-locked statement? E.g. the algorithm
that searches for the "best_table" in open_table() considers
all tables with table->query_id == 0. Could it mistakenly
return a merge child?
How does addition and removal of children affects
LEX::query_tables_own_last pointer?
It seems to interact OK, but I will need to check again
when the new patch is done.
Perhaps tables should never be removed from the list, only added?
Statement memory could be used for that. In that case we will
need to link them to next_local pointer of the TABLE_LIST element
of the merge parent and add a boolean member in merge parent
TABLE_LIST to reflect whether an addition has already been done
and so on. Right now I can't suggest a concrete action plan
to do this change ;(.
The change of mysql_unlock_read_tables() should go into a
separate changeset -- I will be happy to review it.
There are some minor comments:
- we should add a comment in mark_temp_tables_as_free_for_reuse()
and in mark_used_tables_as_free_for_reuse() that ha_reset()
detaches merge children. This was not obvious when reading the
patch.
- perhaps detach_children() should somehow reflect in its name
that it also removes TABLE_LIST elements from the global list
- when looping over tables to attach children in open_tables()
you leave 'tables' pointer pointing at the middle of a list
in case of an error. I believe it's a subtle bug, since
tables is used later in err: label. Moving this loop to a separate
function broke merge.test in one place.
- err_tables variable in reopen_tables() is not used
Thank you for working on this, and let's chat on IRC how to
proceed.
PS Dmitri is now porting WL#3726 to the latest 6.0 to prepare for
a push. Your patch simplifies his work quite a bit.
PPS I am not very worried about lack of reattach at
reopen_tables(), even though it can happen inside lock_tables(),
since we plan to remove reopen_tables() from 6.0 anyway.
--
-- Konstantin Osipov Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com The best DATABASE COMPANY in the GALAXY
