Re: bk commit into maria tree (bell:1.2613) BUG#34712

[Sanja Byelkin]

On Fri, Mar 07, 2008 at 10:13:26AM +0100, Guilhem Bichot wrote:
[skip]
> > +/*static inline*/ void set_lsn(LSN *lsn, LSN value)
> 
> Can we uncomment "static inline" ?

oops it was just for debugging purposes, sorry.

> > +{
> > +  translog_lock_assert_owner();
> > +  *lsn= value;
> > +  /* we generate LSN so something is not flushed in log */
> > +  log_descriptor.is_everything_flushed= 0;
> > +}
> > +
> > +
> >  /**
> >     @brief Write variable record in 1 group.
> >  
> > @@ -4631,7 +4645,7 @@ translog_write_variable_record_1group(LS
> >    if (buffer_to_flush)
> >      translog_buffer_lock_assert_owner(buffer_to_flush);
> >  
> > -  *lsn= horizon= log_descriptor.horizon;
> > +  set_lsn(lsn, horizon= log_descriptor.horizon);
> >    if (translog_set_lsn_for_files(LSN_FILE_NO(*lsn), LSN_FILE_NO(*lsn),
> >                                   *lsn, TRUE) ||
> >        (log_record_type_descriptor[type].inwrite_hook &&
> > @@ -4780,8 +4794,7 @@ translog_write_variable_record_1chunk(LS
> >  
> >    translog_write_variable_record_1group_header(parts, type, short_trid,
> >                                                 header_length, chunk0_header);
> > -
> > -  *lsn= log_descriptor.horizon;
> > +  set_lsn(lsn, log_descriptor.horizon);
> >    if (translog_set_lsn_for_files(LSN_FILE_NO(*lsn), LSN_FILE_NO(*lsn),
> >                                   *lsn, TRUE) ||
> >        (log_record_type_descriptor[type].inwrite_hook &&
> > @@ -5468,7 +5481,18 @@ translog_write_variable_record_mgroup(LS
> >      if (first_chunk0)
> >      {
> >        first_chunk0= 0;
> > -      *lsn= horizon;
> > +
> > +      /*
> > +        We can drop "log_descriptor.is_everything_flushed" earlier when have
> > +        lock on loghandler and assign initial value of "horizon" variable or
> > +        before unlocking loghandler (because we will increase writers
> > +        counter on the buffer and every thread which wanted flush the buffer
> > +        will wait till we finish with it). But IMHO better here take short
> > +        lock and do not bother other threads with waiting.
> > +      */
> > +      translog_lock();
> > +      set_lsn(lsn, horizon);
> > +      translog_unlock();
> >        if (log_record_type_descriptor[type].inwrite_hook &&
> >            (*log_record_type_descriptor[type].inwrite_hook) (type, trn,
> >                                                              tbl_info,
> 
> We have a problem here.
> *lsn must be set and inwrite_hook must be called, both under
> translog_lock() (for *lsn, the reason is because Checkpoint relies on
> it, and for inwrite_hook, the reason is the definition of
> inwrite_hook, and Checkpoint relies on this definition).
> It looks like in the existing code, this is not true (if it was true,
> a safemutex assertion would fire, saying "you already have the lock",
> when it executes the translog_lock() of your patch above).
> Could you please fix it?

No. it is true. if I remove the lock assertion will fire.

We unlocked the handler at this moment. LSN in this case not the very
begining of the group but inside it. so we do fallowing:

Store horizon
advance horizon pointer
unlock handler
write data and move stored value
store this "moving pointer" as LSN at right moment

> A big question:
> if two threads do
> translog_lock()
> some code;
> translog_unlock();
> 
> is it absolutely sure that they are serialized?

yes for sure.

> 
> > @@ -5742,7 +5766,7 @@ static my_bool translog_write_fixed_reco
> >        translog_buffer_lock_assert_owner(buffer_to_flush);
> >    }
> >  
> > -  *lsn= log_descriptor.horizon;
> > +  set_lsn(lsn, log_descriptor.horizon);
> >    if (translog_set_lsn_for_files(LSN_FILE_NO(*lsn), LSN_FILE_NO(*lsn),
> >                               *lsn, TRUE) ||
> >        (log_record_type_descriptor[type].inwrite_hook &&
> > @@ -7293,6 +7317,12 @@ my_bool translog_flush(TRANSLOG_ADDRESS 
> >  
> >    pthread_mutex_lock(&log_descriptor.log_flush_lock);
> >    translog_lock();
> > +  if (log_descriptor.is_everything_flushed)
> > +  {
> > +    DBUG_PRINT("info", ("everything is flushed"));
> > +    translog_unlock();
> > +    goto out;
> > +  }
> >    flush_horizon= LSN_IMPOSSIBLE;
> >    old_flushed= log_descriptor.flushed;
> >    for (;;)
> > @@ -7338,7 +7368,14 @@ my_bool translog_flush(TRANSLOG_ADDRESS 
> >              then was at the moment of start flushing);
> >            */
> >            if (buffer_start == log_descriptor.bc.buffer_no)
> > +          {
> > +            /*
> > +              We are going to flush last buffer, and will not release
> > +              log_flush_lock until it happened, so we can set the flag here
> > +            */
> 
> I don't understand. Above, we set is_everything_flushed under
> translog_lock(), and here you refer to log_flush_lock.

the handler is locked also here (for
translog_force_current_buffer_to_finish()) but coment is about other: no
other thread can use the value to skip flush untill this fluesh really
be finished.

I'll try clarify it in the comment.

> 
> > +            log_descriptor.is_everything_flushed= 1;
> >              translog_force_current_buffer_to_finish();
> > +          }
> >          }
> >          break;
> >        }
> 

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /    Mr. Oleksandr Byelkin <sanja@stripped>
 / /|_/ / // /\ \/ /_/ / /__   MySQL AB, Full-Time Developer
/_/  /_/\_, /___/\___\_\___/   Lugansk, Ukraine
       <___/   www.mysql.com