Hello,
thanks for your new reviews.
I signal here two bugs which I found:
On Tue, Nov 06, 2007 at 08:56:12PM +0100, Guilhem Bichot wrote:
> ChangeSet@stripped, 2007-11-06 20:56:05+01:00, guilhem@stripped +59 -0
> WL#866 - Online backup driver for MyISAM.
> When this driver (file myisam_backup_log.cc) is to do a backup,
> it starts physical logging of changes for target tables (file
> mi_log.c), dirtily copies tables, then the physical log.
> When this driver does a restore, it copies back tables and the log,
> applies the log (file mi_examine_log.c).
> The data file is always backed up; for the index file two methods are
> available: either only its 64KB header is copied and then restore will
> repair indexes, or the whole index file is copied.
> Backup starts and runs without disturbing any running or new update
> except at the very end: when creating the validity point it locks all
> backed-up MyISAM tables with a read lock (so, stalls new update
> statements and waits for running update statements to finish).
> I recommend to reviewers to read in order myisam_backup_driver.cc,
> mi_log.c then other files.
> GUILHEM_TODO tags identify the most urgent todos.
> @@ -436,6 +445,17 @@ int mi_reset(MI_INFO *info)
> */
> if (info->opt_flag & (READ_CACHE_USED | WRITE_CACHE_USED))
> {
> + /*
> + Logically there should not be a WRITE_CACHE at this stage. It is
> + important: mi_log_stop_physical() may be flushing the write cache at
> + this same moment, if we do it here concurrently it is not
> + safe. Basically, a client thread can flush the WRITE_CACHE if and only
> + if it is not concurrent with mi_log_stop_physical() i.e. holds a
> + write-thr_lock or intern_lock. mi_reset() for example does not satisfy
> + this, because the SQL layer unlocks tables before calling
> + ha_myisam::reset().
> + */
> + DBUG_ASSERT(!(info->opt_flag & WRITE_CACHE_USED));
> info->opt_flag&= ~(READ_CACHE_USED | WRITE_CACHE_USED);
> error= end_io_cache(&info->rec_cache);
> }
There is a case where this assertion wrongly fires.
Replication slave is executing a row-based binlog event
(Insert_rows_log_event) generated by a REPLACE:
- the slave calls ha_start_bulk_insert()
in Write_rows_log_event::do_before_row_operations(), that sets up
WRITE_CACHE_USED.
- Rows_log_event::write_row() tries a ha_myisam::write_row()
- which fails because of duplicate key (ok, this was REPLACE)
- so the duplicate record is retrieved (for an update soon to come, as
Rows_log_event::write_row() emulates REPLACE) and that calls
ha_rnd_init() which calls mi_reset(), while WRITE_CACHE_USED is still
true, asserts.
It is a case where the assertion is over-paranoid: the assertion is to
make sure that the end_io_cache() of mi_reset() never happens at the
same time as the end_io_cache() of mi_log_stop_physical(); but here
the table is locked (by some replication function) so
mi_log_stop_physical() cannot be running.
I will try to change it to
DBUG_ASSERT(!((info->opt_flag & WRITE_CACHE_USED) &&
(info->lock.type == TL_UNLOCK)));
which should trap only the dangerous case, that WRITE_CACHE_USED be
end-ed when the table is not locked.
> diff -Nrup a/storage/myisam/mi_log.c b/storage/myisam/mi_log.c
> --- a/storage/myisam/mi_log.c 2007-08-13 15:11:16 +02:00
> +void _myisam_log_command(IO_CACHE *log, enum myisam_log_commands command,
> + void *info_or_share,
> + const uchar *buffert, uint length, int result)
> {
> - uchar buff[9];
> - int error,old_errno;
> + uchar buff[14];
> + int error, old_errno, bufflen;
> ulong pid=(ulong) GETPID();
> -
> + my_bool logical= (log == &myisam_logical_log);
> + MYISAM_SHARE *share;
> + File file= logical ? ((MI_INFO *)info_or_share)->dfile :
> + (share= (MYISAM_SHARE *)info_or_share)->kfile;
> + LINT_INIT(share);
This LINT_INIT(share), in builds where it expands to share=0, will
cancel the effect of the assignement to "share" in the previous line.
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Mr. Guilhem Bichot <guilhem@stripped>
/ /|_/ / // /\ \/ /_/ / /__ MySQL AB, Lead Software Engineer
/_/ /_/\_, /___/\___\_\___/ Bordeaux, France
<___/ www.mysql.com
