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

[Georgi Kodinov]

Hi,

Thanks for your quick review.

On 23.06.2007, at 11:58, Konstantin Osipov wrote:

> * kgeorge@stripped <kgeorge@stripped> [07/06/22 20:49]:
>> 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.

<snip>

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

Good point : will make it a rule when adding threaded tests. Done.

>> +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?

I was told that the innodb_mysql.test is the only innodb related test  
where we can add innodb tests. The rest of the innodb*.test files are  
for the InnoDB guys to handle.

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

Right, I'll do that when I merge to 5.1, but for 5.0 there's no mix.inc.

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

It was a duplicate indeed. Removed. Thanks for spotting that out.

Best Regards,
Joro
-- 
Georgi Kodinov, Senior Software Engineer
MySQL AB, Plovdiv, Bulgaria, www.mysql.com
Office: [phone] Mobile: [phone] Skype: georgekodinov

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