Re: bk commit into 5.0 tree (gkodinov:1.2505) BUG#28983

[Georgi Kodinov]

Guilhem,

On 03.07.2007, at 17:15, Guilhem Bichot wrote:

> On Tue, Jul 03, 2007 at 04:42:02PM +0300, Georgi Kodinov wrote:
>> Guilhem,
>>
>> On 03.07.2007, at 16:37, Guilhem Bichot wrote:
>>
>>> Hello,
>>>
>>> On Tue, Jul 03, 2007 at 10:36:40AM +0300, kgeorge@stripped wrote:
>>>> ChangeSet@stripped, 2007-07-03 10:36:37+03:00, gkodinov@stripped
>>>> +1 -0
>>>>  Bug #28983: 'reset master' in multiple threads and innodb tables
>>>>  asserts debug binary
>>>>
>>>>  We can't reliably check if the binary log is opened without
>>>>  acquiring its mutex.
>>>>  Fixed by removing this check.
>>>>
>>>>
>>
>> <cut>
>>
>>> Are you sure that is_open()-without-mutex is really the cause of the
>>> assertions observed by the bug reports? They were assertions in
>>> binlog_close_connection() and binlog_commit(), which test also
>>> contents of trans_log and of thd->options. In "DBUG_ASSERT(A &&
> B)",
>>> is it A or B which is observed false?
>>
>> The problem is fully reproducible using the excellent test program
>> attached to the bug. I've been able to get it into my debugger. So
>> I'm sure this is the problem.
>
> Excellent. So you saw is_open() be false, even though --log-bin was
> given. That means RESET MASTER makes is_open() be false for an
> instant. And so during this instant some statement can miss going to
> the binlog (as it does a first check without mutex and then a second
> check with mutex, like in mysql_insert()). Which is not an issue as
> RESET MASTER deletes binlogs anyway...
> Still, wasn't there value in testing is_open() in assertions? It was
> to protect against wrong usage of binlog functions.
> If we wanted to keep the testing of is_open() in assertions, we should
> do it under mutex. How about introducing
> is_open_for_debug() which would be like is_open() but uses the mutex?
> And then replace, in DBUG_ASSERT(), is_open() by is_open_for_debug()?
> Then:
> - we have no crashes (bug is fixed) in debug builds as we use the
> mutex
> - we haven't slowed down release builds
> ... ?

Good idea in general, but still vulnerable : imagine:
1. acquire the mutex
2. test the flag
3. release the mutex
4. Some other thread closes the log
5. the operation continues with the problem the assert tries to  
prevent undetected.

So unless we do atomic check/action within the mutex I'd say its  
better without a check.

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