Hi Dmitri,
thank you very much for the answers. Please see below for comments.
Dmitri Lenev wrote:
...
> * Ingo Strüwing <ingo@stripped> [07/07/18 00:37]:
...
>> Dmitri Lenev wrote:
...
>>> * 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
...
> My initial idea was that this call can be used to break the association
> between a merge table and its underlying tables when we release the merge
> table into the table cache. Although, perhaps, we don't need this call as
> long as we call extra(HA_EXTRA_FINALIZE_MERGE_OPEN) each time we get a
> merge table from the table cache, I still think that it can be useful
> for debugging purposes. For example, in this call we can mark the
> handler for merged table as "unusable" until the next call to
> extra(HA_EXTRA_FINALIZE_MERGE_OPEN)).
Ok. I didn't get that idea before.
So when a MERGE table is released to the table cache, the children are
detached. This means that the storage engine instance of the MERGE table
must be closed. This would release the references to the MyISAM table
instances. Consequently we end up with a MERGE handler object without an
open storage engine table. This must of course not be used for table access.
When the MERGE table is re-used, new MyISAM table instances need to be
referenced. By chance this could be the same as before, but often it
would not.
This answers all my questions.
...
> assertions
> are meant to catch errors in the code. What will happen if someone in
> future makes a mistake and allows a situation in which some code tries
> to call handler methods after merge table was used by some statement (so
> MYRG_INFO is non-NULL), but when this statement is already finished, so
> this table was released to the table cache.
In this case I would close the storage engine instance and set
"MYRG_INFO" to NULL.
> Since I think that we should
> break the association between a merge table and its underlying table at
> this point, access to most of handler methods of merge tables should be
> illegal until the next call to extra(HA_EXTRA_FINALIZE_MERGE_OPEN).
Ok. But if we take that hypothetical chance into account, we should
check with "if", not with "DBUG_ASSERT". According to Murphy, the real
weird things happen in production systems, not in developer
environments. Our test suite proves every day that we cannot foresee
every possible situation.
...
>> 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.
>
> OK. But in this case please check that everything works correctly in
> the situation when we fail to open one of the child tables (i.e. that
> we remove the child tables from the list in this situation as well).
Either I misunderstand your sentence dramatically, or I am becoming
really scared. Does this mean that a failure to open a table takes that
table from the list and proceeds with the next one? Don't we just abort
the list and close what we have opened thus far?
...
> Hmm... This means that the table-cache should be aware of relationships
> between merge tables and their child tables. In my letter about your
> previous patch for this bug I tried to explain why I believe that this
> is a wrong approach (in short, it makes it harder to separate meta-data
> locking from the table cache implementation).
Yes. But I thought you missed the "refresh" problem. Since I do now know
that you want to disassociate children from parent at close, my concerns
are obsolete.
...
Then I'll continue with implementing close of MERGE tables. If you have
comments regarding my coding of the "open" part, please send them.
Regards
Ingo
--
Ingo Strüwing, Senior Software Developer
MySQL GmbH, Radlkoferstr. 2, D-81373 München
Geschäftsführer: Kaj Arnö - HRB München 162140
