Hi Konstantin,
this is the second and final part of my reply to your review.
If you agree (kind of) with my comments, I can make an incremental
and/or collapsed patch for your final review.
Konstantin Osipov, 29.02.2008 23:10:
...
> 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)
Done.
> - how they work for a sub-statement that is run inside a function
Done.
> - the two above, but under LOCK TABLES
Done.
> - attach/detach for LOCK TABLES statement itself
Done.
> - 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.
Done.
> - what is the difference in all above cases between behavior
> of temporary and base tables.
Done.
>
> 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.
...
>
> 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?
No. Whenever a locked table is selected from the list, it is assigned
the current query_id. Then it is skipped the next time.
>
> 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.
If I get the semantics of this pointer correctly, it is used to chop off
the list of tables added for SF and the like. Tables added for
prelocking, that is.
Chopping off happens in close_tables_for_reopen() only. Even if a MERGE
parent would be the last own table, and hence its children would be
chopped off too, close_tables_for_reopen() immediately calls
close_thread_tables(), where they are removed anyway.
However, since removing the child list is done by copying next_global
from the last child into the parent, we could perhaps "resurrect" the
chopped off tables.
I was indeed able to verify this by debugging. The resulting list is fed
to the next call to open_tables(). But
sp_cache_routines_and_add_tables() fixed it.
Anyway, to do it correctly, I fixed it as so:
@@ -751,8 +751,19 @@ int ha_myisammrg::detach_children(void)
/*
Remove children from the table list. This won't fail if called
twice. The list is terminated after removal.
+
+ If the parent is LEX::query_tables_own_last and pre-locked tables
+ follow (tables used by stored functions or triggers), the children
+ are inserted behind the parent and before the pre-locked tables. But
+ we do not adjust LEX::query_tables_own_last. The pre-locked tables
+ could have chopped off the list by clearing
+ *LEX::query_tables_own_last. This did also chop off the children. If
+ we would copy the reference from *this->children_last_l in this
+ case, we would put the chopped off pre-locked tables back to the
+ list. So we refrain from copying it back, if the destination has
+ been set to NULL meanwhile.
*/
- if (this->children_l->prev_global)
+ if (this->children_l->prev_global &&
*this->children_l->prev_global)
*this->children_l->prev_global= *this->children_last_l;
>
> 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 ;(.
Neither do I.
>
> The change of mysql_unlock_read_tables() should go into a
> separate changeset -- I will be happy to review it.
Ok. But probably I will not be able to find a repeatable test case for
it. And it is too small for a new worklog task.
>
> 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.
Does it? It isn't obvious from code to me either. And I don't see an
evidence of it by tracing. What made you believe it does?
Note that I have explicit table->file->extra(HA_EXTRA_DETACH_CHILDREN)
near table->file->ha_reset() in mark_temp_tables_as_free_for_reuse() and
close_thread_table(), but not in mark_used_tables_as_free_for_reuse().
The latter is called from close_thread_tables() only, where I call
table->file->extra(HA_EXTRA_DETACH_CHILDREN) also.
>
> - perhaps detach_children() should somehow reflect in its name
> that it also removes TABLE_LIST elements from the global list
IMHO removing TABLE_LIST elements is kind of a detach too. So the name
could be thought as detach_children_from_table_and_query_list(), which
is unnecessary long for my taste.
But I admit that at least the function comment should mention the fact.
>
> - 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.
This bug is a feature. :)
At first 'tables' is an automatic variable, local to open_tables(). At
leave of the function its value doesn't matter.
Second, at the err: label I use 'tables' exclusively for the purpose to
test if both loops (the big open loop and the small attach loop)
completed or if one of them was interrupted. So I don't use its value
for referencing any object.
Third, the feature is to check (result && tables) after err: and to
clear out all tables->table references if there was any error and a loop
did not end. mysql_admin_table() does not check the 'result' of
open_tables(), but only tables->table.
Fourth, I guess, moving the attach loop to a separate function breaks
merge.test exactly when it uses mysql_admin_table() in some way, because
the above mentioned cleanup is skipped.
>
> - err_tables variable in reopen_tables() is not used
Agree.
...
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
