From: Michael Widenius
Date: February 4 2009 10:39pm
Subject: re: bitmap->changed=0 without mutex?
List-Archive: http://lists.mysql.com/maria/432
Message-Id: <18826.6436.281687.2824@narttu.mysql.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit


Hi!

>>>>> "Guilhem" == Guilhem Bichot <guilhem@stripped> writes:

Guilhem> Hello Monty,
Guilhem> In revision sp1r-monty@stripped/narttu.mysql.fi-20080529153333-17560
Guilhem> you did this change to _ma_flush_table_files():

Guilhem>       if (share->data_file_type == BLOCK_RECORD)
Guilhem>       {
Guilhem> -      if(_ma_bitmap_flush(share) ||
Guilhem> -         flush_pagecache_blocks(share->pagecache, &info->dfile,
Guilhem> -                                flush_type_for_data))
Guilhem> -        goto err;
Guilhem> +      if (flush_type_for_data != FLUSH_IGNORE_CHANGED)
Guilhem> +      {
Guilhem> +        if (_ma_bitmap_flush(share))
Guilhem> +          error= 1;
Guilhem> +      }
Guilhem> +      else
Guilhem> +        info->s->bitmap.changed= 0;
Guilhem> +      if (flush_pagecache_blocks(share->pagecache, &info->dfile,
Guilhem> +                                 flush_type_for_data))
Guilhem> +        error= 1;
Guilhem>       }

Guilhem> with comment "Optimization: - Don't flush io_cache or bitmap if we are 
Guilhem> using FLUSH_IGNORE_CHANGED".

Guilhem> I wonder, isn't it a problem to do
info-> s->bitmap.changed= 0;
Guilhem> without having bitmap->bitmap_lock?

Guilhem> The only caller of this function with FLUSH_IGNORE_CHANGED for the data 
Guilhem> file is maria_delete_all_rows() which takes an exclusive lock on the 
Guilhem> table (and will continue doing so in future Maria versions otherwise it 
Guilhem> cannot my_chsize() the files, as this removes any old rows so kills 
Guilhem> versioning).
Guilhem> But there could be a concurrent checkpoint doing _ma_bitmap_flush_all(), 
Guilhem> I wonder if there can be an issue there.

Guilhem> I propose to add a bitmap mutex lock/unlock around the setting, to feel 
Guilhem> safe.

Yes, this should be safer. This function is also called so seldom that
the new mutex will not have any notable slowdown effect.

Regards,
Monty