Re: bk commit into 5.0 tree (gkodinov:1.2500) BUG#29154

[Konstantin Osipov]

* kgeorge@stripped <kgeorge@stripped> [07/06/22 20:49]:
> Below is the list of changes that have just been committed into a local
> 5.0 repository of kgeorge. When kgeorge does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> 
> ChangeSet@stripped, 2007-06-22 19:20:55+03:00, gkodinov@stripped +4 -0
>   Bug #29154: LOCK TABLES is not atomic when >1 InnoDB tables are locked
>     LOCK TABLES takes a list of tables to lock. It may lock several 
>     tables successfully and then encounter a tables that it can't lock, 
>     e.g. because it's locked. In such case it needs to undo the locks on
>     the already locked tables. And it does that. But it has also notified
>     the relevant table storage engine handlers that they should lock.
>     The only reliable way to ensure that the table handlers will give up
>     their locks is to end the transaction. This is what UNLOCK TABLE 
>     does : it ends the transaction if there were locked tables by LOCK 
>     tables.
>     It is possible to end the transaction when the lock fails in 
>     LOCK TABLES because LOCK TABLES ends the transaction at its start 
>     already. 
>     Fixed by ending (again) the transaction when LOCK TABLES fails to
>     lock a table.
> --- 1.626/sql/sql_parse.cc	2007-06-14 23:58:22 +03:00
> +++ 1.627/sql/sql_parse.cc	2007-06-22 19:20:54 +03:00
> @@ -3837,7 +3837,10 @@ end_with_restore_list:
>      break;
>    case SQLCOM_LOCK_TABLES:
>      unlock_locked_tables(thd);
> -    if (check_db_used(thd, all_tables) || end_active_trans(thd))
> +    /* we must end the trasaction first, regardless of anything */
> +    if (end_active_trans(thd))
> +      goto error;
> +    if (check_db_used(thd, all_tables))
>        goto error;
>      if (check_table_access(thd, LOCK_TABLES_ACL | SELECT_ACL, all_tables, 0))
>        goto error;
> @@ -3855,7 +3858,15 @@ end_with_restore_list:
>        send_ok(thd);
>      }
>      else
> +    {
> +      /* 
> +        Need to end the current transaction, so the storage engine (InnoDB)
> +        can free its locks if LOCK TABLES locked some tables before finding
> +        that it can't lock a table in its list
> +      */
> +      end_active_trans(thd);
>        thd->options&= ~(ulong) (OPTION_TABLE_LOCK);
> +    }
>      thd->in_lock_tables=0;
>      break;
>    case SQLCOM_CREATE_DB:
> 
> --- 1.23/mysql-test/r/innodb_mysql.result	2007-06-14 14:39:42 +03:00
> +++ 1.24/mysql-test/r/innodb_mysql.result	2007-06-22 19:20:54 +03:00
> @@ -661,4 +661,15 @@ UPDATE t3 SET a = 'us' WHERE a = 'uk';
>  SELECT * FROM t3 WHERE a = 'uk';
>  a
>  DROP TABLE t1,t2,t3;
> +CREATE TABLE t1 (a INT) ENGINE=InnoDB;
> +CREATE TABLE t2 (a INT) ENGINE=InnoDB;
> +SET AUTOCOMMIT=0;
> +INSERT INTO t2 VALUES (1);
> +SET AUTOCOMMIT=0;
> +LOCK TABLES t1 READ, t2 READ;
> +ERROR HY000: Lock wait timeout exceeded; try restarting transaction
> +COMMIT;
> +INSERT INTO t1 VALUES (1);
> +SET AUTOCOMMIT=default;
> +DROP TABLE t1,t2;
>  End of 5.0 tests
> 
> --- 1.23/mysql-test/t/innodb_mysql.test	2007-06-14 14:40:23 +03:00
> +++ 1.24/mysql-test/t/innodb_mysql.test	2007-06-22 19:20:54 +03:00
> @@ -636,4 +636,35 @@ SELECT * FROM t3 WHERE a = 'uk';
>  
>  DROP TABLE t1,t2,t3;
>  
> +
> +#
> +# Bug #29154: LOCK TABLES is not atomic when >1 InnoDB tables are locked
> +#
> +
> +CREATE TABLE t1 (a INT) ENGINE=InnoDB; 
> +CREATE TABLE t2 (a INT) ENGINE=InnoDB; 
> +
> +CONNECT (c1,localhost,root,,);
> +CONNECT (c2,localhost,root,,);
> +
> +CONNECTION c1;

Wow, I didn't know this syntax is allowed.
I've always been using --connection.

Could you precede each switch of the current connection with
--echo switch to connection XXX
?

That way it's easier to parse the result log.

> +SET AUTOCOMMIT=0;
> +INSERT INTO t2 VALUES (1);
> +
> +CONNECTION c2;
> +SET AUTOCOMMIT=0;
> +--error ER_LOCK_WAIT_TIMEOUT
> +LOCK TABLES t1 READ, t2 READ;
> +
> +connection c1;
> +COMMIT;
> +INSERT INTO t1 VALUES (1);
> +
> +CONNECTION default;
> +SET AUTOCOMMIT=default;
> +DISCONNECT c1;
> +DISCONNECT c2;
> +DROP TABLE t1,t2;

I don't understand the layout of InnoDB tests.
We have innodb-lock.test file. Perhaps your test should go there
instead?

innondb_mysql.test seems to source mix1.inc and that's all - AFAIU 
this has been done to factor out some tests to use with Falcon in
6.0.
If so, then maybe your test should go to mix1.inc file instead?

Finally, there is already innodb_mysql-master.opt. Why was it
necessary to add yet another .opt file?

Please investigate, fix, and push.

Thank you for looking into this,

-- 
-- Konstantin Osipov              Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com   The best DATABASE COMPANY in the GALAXY