Hi!

...

...

...

...

"sanja" == sanja <sanja@montyprogram.com> writes:

sanja> At file:///home/bell/maria/bzr/work-maria-5.2-lpb956079/ sanja> ------------------------------------------------------------ sanja> revno: 3126 sanja> revision-id: sanja@montyprogram.com-20120319122824-7tnmdh8bq7cbnvni sanja> parent: sanja@montyprogram.com-20120314100903-a6txqk4l73smh189 sanja> committer: sanja@montyprogram.com sanja> branch nick: work-maria-5.2-lpb956079 sanja> timestamp: Mon 2012-03-19 14:28:24 +0200 sanja> message: sanja> FIX for LP bug#956079 sanja> Always set all callbacks for page cache. <cut>

--- a/storage/maria/ma_pagecache.c 2011-08-10 19:44:39 +0000 +++ b/storage/maria/ma_pagecache.c 2012-03-19 12:28:24 +0000 @@ -633,7 +633,6 @@ static my_bool pagecache_fwrite(PAGECACH { DBUG_ENTER("pagecache_fwrite"); DBUG_ASSERT(type != PAGECACHE_READ_UNKNOWN_PAGE); - #ifdef EXTRA_DEBUG_BITMAP /* This code is very good when debugging changes in bitmaps or dirty lists @@ -654,6 +653,12 @@ static my_bool pagecache_fwrite(PAGECACH } #endif

+ DBUG_ASSERT(filedesc->read_callback); + DBUG_ASSERT(filedesc->write_callback); + DBUG_ASSERT(filedesc->write_fail); + DBUG_ASSERT(filedesc->flush_log_callback);

Aren't the above callbacks guranteed to be set by the init code? If thats the case, there is no reason to test for these. There is no reason to test for read_callback or write_callback as we will always get a crash if these are not set (a few lines later). As we already test this in ma_pagecache.h, I think we can remove the DBUG_ASSERT's from ma_pagecache.c

=== modified file 'storage/maria/ma_pagecache.h' --- a/storage/maria/ma_pagecache.h 2011-02-28 17:39:30 +0000 +++ b/storage/maria/ma_pagecache.h 2012-03-19 12:28:24 +0000 @@ -271,6 +271,10 @@ extern void pagecache_set_write_on_delet (F).read_callback= (RC); (F).write_callback= (WC); \ (F).write_fail= (WF); \ (F).flush_log_callback= (GLC); (F).callback_data= (uchar*)(D); \ + DBUG_ASSERT((F).read_callback); \ + DBUG_ASSERT((F).write_callback); \ + DBUG_ASSERT((F).write_fail); \ + DBUG_ASSERT((F).flush_log_callback); \ } while(0)

The above is good! It would of course be good to always use the function pagecache_file_init() to set all callbacks instead of setting these directly. To do this, you could change ma_open.c:: _ma_set_data_pagecache_callbacks() to use the above function instead of setting these directly. This way we would know for sure that either none or all are set and we don't need to do any DBUG_ASSERT() of these elements in the code. Regards, Monty