Bug #12410 | mysqldump/InnoDB too permissive with LOCK TABLE foo READ LOCAL | ||
---|---|---|---|
Submitted: | 6 Aug 2005 6:20 | Modified: | 1 Sep 2005 14:46 |
Reporter: | Timothy Smith | Email Updates: | |
Status: | Closed | Impact on me: | |
Category: | MySQL Server: InnoDB storage engine | Severity: | S2 (Serious) |
Version: | 4.1 | OS: | Any (any) |
Assigned to: | Heikki Tuuri | CPU Architecture: | Any |
[6 Aug 2005 6:20]
Timothy Smith
[7 Aug 2005 6:30]
Heikki Tuuri
Tim, in READ LOCAL, InnoDB does use an S-lock on the rows it reads. ha_innodb.cc, ::store_lock(): if ((lock_type == TL_READ && thd->in_lock_tables) || (lock_type == TL_READ_HIGH_PRIORITY && thd->in_lock_tables) || lock_type == TL_READ_WITH_SHARED_LOCKS || lock_type == TL_READ_NO_INSERT || (thd->lex->sql_command != SQLCOM_SELECT && lock_type != TL_IGNORE)) { /* The OR cases above are in this order: 1) MySQL is doing LOCK TABLES ... READ LOCAL, or 2) (we do not know when TL_READ_HIGH_PRIORITY is used), or 3) this is a SELECT ... IN SHARE MODE, or 4) we are doing a complex SQL statement like INSERT INTO ... SELECT ... and the logical logging (MySQL binlog) requires the use of a locking read, or MySQL is doing LOCK TABLES ... READ. 5) we let InnoDB do locking reads for all SQL statements that are not simple SELECTs; note that select_lock_type in this case may get strengthened in ::external_lock() to LOCK_X. */ ... prebuilt->select_lock_type = LOCK_S; prebuilt->stored_select_lock_type = LOCK_S; Thus, a single table is dumped in a consistent state, which is the state at the end of the transaction. The bug is that mysqldump does not use BEGIN ... COMMIT to wrap the whole dump operation. Then after dumping each individual table, S-locks on rows are released. Then each individual table can correspond to a different timepoint! Possible fixes: 1) we could convert the MySQL lock TL_READ to TL_READ_NO_INSERT in ::store_lock(); then READ LOCAL would be equivalent to READ for InnoDB tables; or 2) mysqldump could use BEGIN ... COMMIT to wrap the whole dump operation. Note that InnoDB tables should normally be dumped with the --single-transaction option. Then there are no locks whatsoever, and the dump is consistent across different InnoDB tables. The READ LOCAL option should be used if we want a dump of mixed MyISAM and InnoDB tables. Hmm... we have to think about this. Regards, Heikki
[8 Aug 2005 22:07]
Timothy Smith
Heikki, As I see it, the problem is that the meaning of READ LOCAL for MyISAM is quite different from the meaning it currently has for InnoDB. I think either READ LOCAL should be disallowed for InnoDB tables, or it should somehow behave in the same way. Perhaps it is OK to be more restrictive with InnoDB (have READ LOCAL behave as READ), and give a warning. With MyISAM, the LOCK TABLES foo READ LOCAL works across multiple transactions, allows other sessions to INSERT only (no updates or deletes), and the session that holds the lock does not see those changes. With InnoDB, it does not work across multiple transactions. Also, the share locks aren't needed; inside a single transaction, the a normal SELECT is fine. The goal is not to prevent others from updating the tables, but only to prevent the current session from seeing those updates. Well, that's what InnoDB always does with a simple SELECT. So, if mysqldump were to use BEGIN ... END around the dumps of all tables in the database, then no LOCK TABLES statement at all is needed (for InnoDB tables, that is). I still don't like the idea of LOCK TABLES ... READ LOCAL having very different semantics between InnoDB and MyISAM. I'd much prefer that it simply not work on InnoDB, or perhaps have stronger locking semantics and issue a warning (and of course document it clearly in the LOCK TABLES section of the manual). So, to move forward with this (assuming I understood things correctly), we should certainly change mysqldump to dump all tables in a database within a single transaction. That will solve the immediate problem for the customer. And, we should decide how to resolve the semantic difference between MyISAM and InnoDB for READ LOCAL locks. The choices I see are: 1) throw an error when trying to lock an InnoDB table, 2) convert the READ LOCAL lock to a READ lock, and issue a warning, or 3) somehow make the READ LOCAL S-locks persist across multiple transactions Maybe there are some other options. My preference is #1 - it's not needed for InnoDB tables, it's probably very hard to implement the same semantics, and disallowing it will be the least confusing to the user. My problem with #2 is that it makes InnoDB do more locking than MyISAM, when there is no need for it. It may damage InnoDB's credibility ("If InnoDB can't allow inserts here, where else does it have locking problems?"), it's a hack that may come back to bite us later on, etc. READ LOCAL makes sense for non-transactional storage engines. It makes no sense for InnoDB, in my opinion. My problem with #3 is that it's likely to be a lot of work, inefficient, and it's useless anyways. Regards, Timothy
[17 Aug 2005 14:29]
Heikki Tuuri
Tim, if someone has time to fix this in mysqldump, that would be very good. I do not think that converting LOCK TABLES ... READ LOCAL to LOCK TABLES READ is a big sin. Normally, a database engine is allowed to use stricter locking if it does not have the exact same level of locking. Hmm... I am changing this to a mysqldump bug report, and unassigning myself from this. Anyone is free to assign this bug back to me if he thinks I should change the InnoDB internal behavior. Regards, Heikki
[18 Aug 2005 1:24]
Timothy Smith
Heikki, I'm not sure that mysqldump can be fixed without changing the InnoDB behavior. If mysqldump tries to "start transaction with consistent snapshot" and then "lock tables", there is a gap there where the MyISAM tables might get updated and be out of sync with the consistent snapshot of the InnoDB tables. Also, LOCK TABLES will perform an implicit commit, so the transaction disappears anyways. If mysqldump tries to lock tables first, there is a probem because START TRANSACTION will unlock tables implicitly. Also, I still feel that the current READ LOCAL behavior of InnoDB is wrong. Changing it to behave just like READ would be acceptable to me, but leaving it as it is now does not seem OK. I will let the MySQL architecture people decide that, though. Regards, Timothy
[19 Aug 2005 9:11]
Heikki Tuuri
Tim, hmm... actually now I see that READ LOCAL causes MyISAM tables to be dumped in the state corresponding to the START of the dump, while InnoDB tables are dumped at the state corresponding to the END of the dump! This is clearly a bug, and I think the only way to fix this is to convert READ LOCAL inside InnoDB to a strict READ lock. I am assigning this bug back to myself. Regards, Heikki
[31 Aug 2005 11:10]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/internals/29095
[31 Aug 2005 11:12]
Heikki Tuuri
Fixed in 4.1.15 and 5.0.13. Dear docs team: also update the manual to say that for InnoDB tables, LOCK TABLE ... READ LOCAL is converted to LOCK TABLE ... READ starting from the above versions.
[1 Sep 2005 14:46]
Paul DuBois
Noted in 4.1.15, 5.0.13 changelogs and in LOCK TABLES section.