Re: bk commit - 4.1 tree (istruewing:1.2630) BUG#26379

Hello Ingo!

• Ingo Strüwing <ingo@mysql.com> [07/07/18 00:37]:
  > Hi Dmitri, Konstantin,
  >
  > Dmitri Lenev wrote:
  > > Hello Ingo!
  > >
  > > * ingo@mysql.com <ingo@mysql.com> [07/05/21 19:55]:
  > >> ChangeSet@1.2630, 2007-05-21 17:48:22+02:00, istruewing@chilla.local +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():
  

...

> > 2) ha_myisammrg::extra():

...

> > 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.

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)).

> > 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.

Well, of course this cannot happen if the code is correct. But 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. 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).

Do you need help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

See more on this below.

> > 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.

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).

> > 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.

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).

...

>
> 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.

I think that we should follow this approach. Indeed, you have a point when you say that repeating compatibility checks each time for each statement is too expensive. But we can try to alleviate this problem. Note, that long-term we will have to support some form of dependency tracking between various objects and perform invalidation anyway. When this mechanism is in place, we can use it to avoid extra compatibility checks. Short-term we can use a more crude method, for example, we can have a global counter of DDL operations in the server and store its value in both merge and child tables' TABLE objects. If these values don't match when we call extra(HA_EXTRA_FINALIZE_MERGE_OPEN), that means we need to repeat the compatibility check.

Do you need more help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

> ...

> > 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.

Hmm... OK.

> > 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.

Yes.

> 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
> above.

See my comments above.

-- 
Dmitri Lenev, Software Developer
MySQL AB, www.mysql.com

Are you MySQL certified?  
http://www.mysql.com/certification

-- 
MySQL Code Commits Mailing List
For list archives: 
http://lists.mysql.com/commits
To unsubscribe:    
http://lists.mysql.com/commits?unsub=lists@pantek.com