Hello Ingo,
On Tue, Nov 27, 2007 at 08:24:54PM +0100, Ingo Strüwing wrote:
> Ok to push by me.
thank you.
> Please see below for comments about a possible future improvement as
> discussed on IRC.
>
> Guilhem Bichot, 27.11.2007 15:04:
> ...
> > ChangeSet@stripped, 2007-11-27 15:04:43+01:00, guilhem@stripped +66 -0
> > WL#866 - Online backup driver for MyISAM.
> ...
> > include/keycache.h@stripped, 2007-11-27 15:04:37+01:00, guilhem@stripped +11
> -2
> > A block now has an optional callback_argument. A key cache has a global
> > post_write function which is called after any block goes to the file.
> > The callback's argument are set through key_cache_write(). The
> > callback is set when opening MyISAM tables.
> > This is used by MyISAM's physical logging: when a key page
> > is flushed to the file, the write is also recorded in the physical log,
> > thanks to such callback.
>
> This means that the callback function is called for every page flush,
> regardless if backup is going on or not and regardless if the table is
> in backup or not. The callback knows and returns if nothing is to do.
>
> IMHO it would save some overhead if we could limit calling of the
> callback to pages of tables that are in backup.
>
> Since we have a global pointer for the function, we could use
> callback_arg for the decision.
>
> You said rightfully: if you don't set callback_arg when
> key_cache_write() a block, later a backup may start and it needs the
> cached block.
>
> That's true as long as you want to avoid a cache flush per backup table
> at backup start. However, with such a flush we could supply 'share' only
> for tables in backup and NULL otherwise.
All true. It's a trade-off between the flush-at-backup-start and the
function call + atomic read all the time. Though we already have the
atomic read per every data file write (like in mi_dynrec.c).
I have added your idea as a comment in mi_log_start_physical():
/*
We don't need to flush key blocks, WRITE_CACHE or the state
because every time they are written to disk (at the latest in
mi_log_stop_physical()) they check for physical logging
(key cache always has log_key_page_flush_physical() as
post_write, WRITE_CACHE always has log_flushed_write_cache_physical()
has post_write, even when _not_ in backup), so any now cached info will
finally reach the log.
Conversely, if we wanted to register no callback in key cache and
WRITE_CACHE when no backup is running (to save function calls
and atomic reads when no backup is running), we would have to
flush key cache and WRITE_CACHE here.
*/
> This flush would also avoid that we log pages with changes that were
> done prior to the backup.
I don't think that this problem happens now. If a change is done prior
to the backup:
- if it is still in-memory-only when backup starts, we need to log it
- if it went to disk before backup started, we didn't log it
(physical_logging_state was 0), it is in the index file, so the backup
job will get this change when copying the index file.
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Mr. Guilhem Bichot <guilhem@stripped>
/ /|_/ / // /\ \/ /_/ / /__ MySQL AB, Lead Software Engineer
/_/ /_/\_, /___/\___\_\___/ Bordeaux, France
<___/ www.mysql.com
