MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:July 13 2007 4:51pm
Subject:Re: bk commit - 4.1 tree (istruewing:1.2630) BUG#26379
View as plain text  
Hi Dmitri, Konstantin,

Dmitri Lenev wrote:
> Hello Ingo!
> * ingo@stripped <ingo@stripped> [07/05/21 19:55]:
>> ChangeSet@stripped, 2007-05-21 17:48:22+02:00, istruewing@stripped +23 -0
>>   Bug#26379 - Combination of FLUSH TABLE and REPAIR TABLE corrupts
>>               a MERGE table

I committed an intermediate study for the new open approach:

bk commit into 5.1 tree (istruewing:1.2530) BUG#26379
date: 2007-07-13 17:58:57+02:00

This is able to open a merge table successfully.
No attempt is made yet to close it, or treat it otherwise.

I have a couple of questions. Please see below.

> 1) myrg_open():
>    Change this function in such way that it will only read list of
>    child tables from the .MRG and associate this list with handler
>    or even TABLE object (this still can be done with the help of
>    callback function). The rest of the code from this function,
>    which assumes that child tables are already open, should be
>    moved to separate function which should be called once SQL-layer
>    will open them.


> 2) ha_myisammrg::extra():
>    Introduce a new value of parameter for handler::extra() which will
>    inform handler that SQL-layer opened child tables so handler can
>    use them. Upon this call handler also may perform those checks
>    that we have moved to separate function during the first step.


>    Also we might need a separate parameter value that will tell
>    handler that it couldn't use child tables as they are about to be
>    closed.

I fail to see why this could be necessary. The closing thread won't use
the children during close. Other threads have their own instances open.

> It is probably a good idea to add asserts which will
>    ensure that most of ha_myisammrg methods are not called outside
>    of these bracketing calls of handler::extra(). 

The way I understood your suggestions, and the way I implemented them,
this seems not necessary. The initial open does not allocate any MERGE
object. So there is a NULL MYRG_INFO pointer in ha_myisammrg. Attempts
to use it will fail anyway. But again I don't see how this could happen
at all. Same reason as with close.

> 3) open_tables()/close_thread_tables() (tentatively):
>    Introduce code at SQL-layer that for each merge table add its
>    child tables to statement's global table list for further opening,
>    much in the same way it is done now in mysql_make_view() for views.
>    But unlike for views we also have to remove child tables from the
>    statement table list to make the process prepared-statement safe.
>    Two obvious (but not necessarily the best) places for this are 
>    open_tables() and close_thread_tables(). In the first function we
>    can add all child tables for each merge table being opened to the
>    statement's table list, when doing this we might want to keep
>    pointers to the first and last of these tables associated with
>    merge table to simplify their removal afterwards. When we open
>    last child table for a merge table we should call handler::extra()
>    for this merge table to inform handler that all underlying tables
>    are open. Alternatively we can do this for all merge tables at
>    the end of open_tables() call.

As you can see from my patch I understood the above so that I should add
the children to the table list (next_global) when a merge table is
opened, and remove them after the last child has been opened.

>                                    During the close_thread_tables()
>    we should pass through all merge tables and use pointers stored
>    earlier to exclude child tables from the statement's table list.

If we want to finally close the MERGE table, we could take a similar
approach as with open (add children to open_tables and mark them "old").

But when we want to release the table to the cache for later reuse by
the same or another thread, then we must not do the same with the
children. The MERGE storage engine holds references to the MyISAM table
objects. So we must assure that the same TABLE objects with their MyISAM
objects become children of the MERGE table when it is reused.

The alternative would be to re-reference the MyISAM tables from the
MERGE table every time the MERGE table is reused. We must also repeat
the compatibility checks, which are costly.

Please advise.

> 4) ha_myisammrg::lock_count()/store_lock() or lock_tables():
>    Change ha_myisammrg::lock_count() and store_lock() to do nothing
>    and rely on SQL-layer locking child tables. Another possible
>    alternative is to change lock_tables() to ignore child tables.

IMHO we do not need to change anything here. We took the children off
the table list. So lock_tables() won't handle them directly.

> 6) reopen_table(), reopen_tables():
>    Remove those usages of reopen_table()/reopen_tables() which are not
>    associated with reopening table under LOCK TABLES (i.e. get rid of
>    its usage within wait_for_tables()). This will simplify changing of
>    reopen_table() in such way that it will safely handle merge tables.
>    In particular I think we should disallow situations when we will
>    reopen merge tables in place of non-merge tables or merge tables
>    with different child tables (this can lead to deadlock).

This may be the appropriate place to raise the question: How to handle
MERGE tables when a child is refreshed? In this case we must at least
re-reference this child and check its compatibility. If we don't want to
over-stress extra(), then we should do this by re-opening the whole
MERGE table.

Anyway we need to know about the parent-child relationship in the table
cache. Unless we re-open the MERGE table on every reuse as I mentioned


Ingo Strüwing, Senior Software Developer
MySQL GmbH, Radlkoferstr. 2, D-81373 München
Geschäftsführer: Kaj Arnö - HRB München 162140
Re: bk commit - 4.1 tree (istruewing:1.2630) BUG#26379Dmitri Lenev20 Jun
  • Re: bk commit - 4.1 tree (istruewing:1.2630) BUG#26379Ingo Strüwing21 Jun
  • Re: bk commit - 4.1 tree (istruewing:1.2630) BUG#26379Ingo Strüwing13 Jul
  • reopen_tables() [Re: bk commit - 4.1 tree (istruewing:1.2630) BUG#26379]Ingo Strüwing29 Aug
    • Re: reopen_tables() [Re: bk commit - 4.1 tree (istruewing:1.2630) BUG#26379]Dmitri Lenev7 Sep
      • Re: reopen_tables() [Re: bk commit - 4.1 tree (istruewing:1.2630)BUG#26379]Ingo Strüwing7 Sep
        • Re: reopen_tables() [Re: bk commit - 4.1 tree (istruewing:1.2630) BUG#26379]Dmitri Lenev11 Sep
Re: bk commit - 4.1 tree (istruewing:1.2630) BUG#26379Dmitri Lenev25 Jul
  • Re: bk commit - 4.1 tree (istruewing:1.2630) BUG#26379Ingo Strüwing26 Jul