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

[Guilhem Bichot]

Hello Sanja,

I have questions:

On Thu, Mar 06, 2008 at 08:31:40AM +0200, sanja@stripped wrote:
> ChangeSet@stripped, 2008-03-06 08:31:35+02:00, bell@stripped +1 -0
>   Avoiding changing log on flush call
>   if nothing was added to the log (BUG#34712)

If possible, add the bug's title too.

>   storage/maria/ma_loghandler.c@stripped, 2008-03-06 08:31:33+02:00,
> bell@stripped +19 -1
>     Flag that signaling about "everythig-flushed" state of
>     the log added, to avoid changing log on flush request
>     if nothing was written but horizon placed on the page boarder.
> 
> diff -Nrup a/storage/maria/ma_loghandler.c b/storage/maria/ma_loghandler.c
> --- a/storage/maria/ma_loghandler.c	2008-02-29 13:11:24 +02:00
> +++ b/storage/maria/ma_loghandler.c	2008-03-06 08:31:33 +02:00
> @@ -297,6 +297,7 @@ struct st_translog_descriptor
>    pthread_mutex_t purger_lock;
>    /* last low water mark checked */
>    LSN last_lsn_checked;
> +  my_bool is_everything_flushed;
>  };
>  
>  static struct st_translog_descriptor log_descriptor;
> @@ -3771,6 +3772,7 @@ my_bool translog_init_with_table(const c
>              It is beginning of the log => there is no LSNs in the log =>
>              There is no harm in leaving it "as-is".
>            */
> +          log_descriptor.is_everything_flushed= 1;
>            DBUG_RETURN(0);
>          }
>          file_no--;
> @@ -3846,7 +3848,7 @@ my_bool translog_init_with_table(const c
>        translog_free_record_header(&rec);
>      }
>    }
> -
> +  log_descriptor.is_everything_flushed= 1;
>    DBUG_RETURN(0);
>  }

Is it possible to just put
log_descriptor.is_everything_flushed= 1;
at the start of translog_init_with_table() (log manager starts so it's
clear it has nothing to flush) ?

>  
> @@ -4632,6 +4634,7 @@ translog_write_variable_record_1group(LS
>      translog_buffer_lock_assert_owner(buffer_to_flush);
>  
>    *lsn= horizon= log_descriptor.horizon;
> +  log_descriptor.is_everything_flushed= 0;
>    if (translog_set_lsn_for_files(LSN_FILE_NO(*lsn), LSN_FILE_NO(*lsn),
>                                   *lsn, TRUE) ||
>        (log_record_type_descriptor[type].inwrite_hook &&
> @@ -4782,6 +4785,7 @@ translog_write_variable_record_1chunk(LS
>                                                 header_length, chunk0_header);
>  
>    *lsn= log_descriptor.horizon;
> +  log_descriptor.is_everything_flushed= 0;
>    if (translog_set_lsn_for_files(LSN_FILE_NO(*lsn), LSN_FILE_NO(*lsn),
>                                   *lsn, TRUE) ||
>        (log_record_type_descriptor[type].inwrite_hook &&
> @@ -5469,6 +5473,7 @@ translog_write_variable_record_mgroup(LS
>      {
>        first_chunk0= 0;
>        *lsn= horizon;
> +      log_descriptor.is_everything_flushed= 0;
>        if (log_record_type_descriptor[type].inwrite_hook &&
>            (*log_record_type_descriptor[type].inwrite_hook) (type, trn,
>                                                              tbl_info,
> @@ -5743,6 +5748,7 @@ static my_bool translog_write_fixed_reco
>    }
>  
>    *lsn= log_descriptor.horizon;
> +  log_descriptor.is_everything_flushed= 0;
>    if (translog_set_lsn_for_files(LSN_FILE_NO(*lsn), LSN_FILE_NO(*lsn),
>                               *lsn, TRUE) ||
>        (log_record_type_descriptor[type].inwrite_hook &&

Instead of the three places above, why not put
log_descriptor.is_everything_flushed= 0;
only at the start of translog_write_record() ?
If the three functions above are always called via
translog_write_record(), and *only* via it (is it true?), it sounds
safe to do.

> @@ -7292,6 +7298,11 @@ my_bool translog_flush(TRANSLOG_ADDRESS 
>    LINT_INIT(sent_to_disk);
>  
>    pthread_mutex_lock(&log_descriptor.log_flush_lock);
> +  if (log_descriptor.is_everything_flushed)
> +  {
> +    DBUG_PRINT("info", ("everything is flushed"));
> +    goto out;
> +  }
>    translog_lock();
>    flush_horizon= LSN_IMPOSSIBLE;
>    old_flushed= log_descriptor.flushed;
> @@ -7338,7 +7349,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
> +            */
> +            log_descriptor.is_everything_flushed= 1;
>              translog_force_current_buffer_to_finish();
> +          }
>          }
>          break;
>        }

Thanks for enlightening me.
And by the way, the patch above does fix the bug, I tested.

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /    Mr. Guilhem Bichot <guilhem@stripped>
 / /|_/ / // /\ \/ /_/ / /__   MySQL AB, Lead Software Engineer
/_/  /_/\_, /___/\___\_\___/   Bordeaux, France
       <___/   www.mysql.com