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.
How does it sound?
