List:Internals« Previous MessageNext Message »
From:Mats Kindahl Date:April 27 2010 5:02am
Subject:Re: Unnecessary/wrong update_table_map_version() in binlog_end_trans()
?
View as plain text  
Hi Kristian,

The table map version was used in the early implementation to identify
when it was necessary to write out a table map, but I think that the
current logic does not require the table map at all.

I've been reluctant to remove it since I have not had time to sit down
and make sure that it is not needed any more.

Best wishes,
Mats Kindahl

Kristian Nielsen wrote:
> 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.
> 

-- 
Mats Kindahl
Senior Software Engineer
Database Technology Group
Sun Microsystems
Thread
Unnecessary/wrong update_table_map_version() in binlog_end_trans() ?Kristian Nielsen27 Apr
  • Re: Unnecessary/wrong update_table_map_version() in binlog_end_trans()?Mats Kindahl27 Apr
    • Re: Unnecessary/wrong update_table_map_version() in binlog_end_trans() ?Kristian Nielsen27 Apr
      • Re: Unnecessary/wrong update_table_map_version() in binlog_end_trans()?Mats Kindahl3 May