Guilhem,
On 04.07.2007, at 16:46, Guilhem Bichot wrote:
> Hello,
>
> On Tue, Jul 03, 2007 at 05:18:36PM +0300, Georgi Kodinov wrote:
>> On 03.07.2007, at 17:15, Guilhem Bichot wrote:
>>> On Tue, Jul 03, 2007 at 04:42:02PM +0300, Georgi Kodinov wrote:
>>>> On 03.07.2007, at 16:37, Guilhem Bichot wrote:
>>>>> 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.
>
> It is vulnerable, yes it would not catch all wrong coding, but it
> would catch the obvious mistakes. It would miss detecting a wrong code
> (I call a wrong code some code calling a binlog function if binlog is
> closed) only if the wrong code is called while a RESET MASTER is
> happening, which is not all the times.
> But if I follow your idea of atomic check/action: the assertion in
> binlog_commit and binlog_rollback indeed is not atomic, but those
> functions end up calling
> bool MYSQL_BIN_LOG::write(THD *thd, IO_CACHE *cache, Log_event
> *commit_event)
> where the binlog mutex is taken:
>
> VOID(pthread_mutex_lock(&LOCK_log));
> ...
> if (likely(is_open())) // Should always be
> true
> {
>
> So now I propose to leave your patch unchanged except by adding a
> DBUG_ASSERT(is_open()) just above the if(is_open()).
> With this in place, we will protect against wrong code, and do it
> atomically.
I've committed a change like this (http://lists.mysql.com/commits/
31814).
Can you please take a look and let me know if it's ok to push.
Best Regards,
Joro
--
Georgi Kodinov, Senior Software Engineer
MySQL AB, Plovdiv, Bulgaria, www.mysql.com
Office: +359 32 634 397 Mobile: +359 887 700 566 Skype: georgekodinov
Are you MySQL certified? www.mysql.com/certification
