From: Kristian Nielsen
Date: April 27 2010 2:38am
Subject: Unnecessary/wrong update_table_map_version() in binlog_end_trans() ?
List-Archive: http://lists.mysql.com/internals/37890
Message-Id: <87d3xl62n4.fsf@knielsen-hq.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii

Hi,

I am looking at the function binlog_end_trans() in sql/log.cc in MySQL 5.1.44,
and the similar functions binlog_flush_trx_cache() and
binlog_truncate_trx_cache() in latest mysql-trunk.

This has the following code:

    /*
      We need to step the table map version after writing the
      transaction cache to disk.
    */
    mysql_bin_log.update_table_map_version();

...

    /*
      We need to step the table map version on a rollback to ensure
      that a new table map event is generated instead of the one that
      was written to the thrown-away transaction cache.
    */
    mysql_bin_log.update_table_map_version();


My question is if this is really necessary? It seems that since the table map
version is also updated in MYSQL_BIN_LOG::flush_and_set_pending_rows_event(),
it should not be necessary to update it again here?

And if it _is_ necessary, it looks like a bug, as the update in
binlog_end_trans() is done without any locking:

    void update_table_map_version() { ++m_table_map_version; }

(the update in flush_and_set_pending_rows_event() is protected by LOCK_log in
MySQL 5.1.44).

So it seems to me that those update_table_map_version() calls are either
unnecessary or incorrect. Comments?

In mysql_trunk, all updates of table_map_version are now unprotected by any
lock. I do not see how this can be correct? Espcially since it is a 64-bit
number, so we could even imagine (on 32-bit machines) seeing the counter go
from 0xfffffffe to 0x0 in case of a race.

The locking around table_map_version update was removed in this commit:

     revid:alfranio.correia@stripped

This removes the locking and the following comment without any explanation
that I could find:

    /*
      If we are writing to the log file directly, we could avoid
      locking the log. This does not work since we need to step the
      m_table_map_version below, and that change has to be protected
      by the LOCK_log mutex.
    */

So can anyone help me with understanding:

1. If/why the table_map_version in binlog_end_trans is necessary?

2. If/why it is safe to update table_map_version without locks in 5.5?

 - Kristian.