Hi Chuck!
The code actually looks pretty good. I have a few comments, but apart
from that this is as close to "minimal" patch that you can come.
Just my few cents,
Mats Kindahl
cbell@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 5.1 repository of cbell. When cbell does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-08-21 15:56:03-04:00, cbell@mysql_cab_desk. +4 -0
> BUG#29020 : Test: Event results not replicated to slave in RBR
>
> Patch adds new methods to initiate locks on the binlog. These locks are
> used to ensure a table map / row event pair are written as a pair and
> prevent interleaving of the events.
>
> sql/log.cc@stripped, 2007-08-21 15:55:53-04:00, cbell@mysql_cab_desk. +28 -0
> BUG#29020 : Test: Event results not replicated to slave in RBR
>
> Patch adds new methods to initiate locks on the binlog. These locks are
> used to ensure a table map / row event pair are written as a pair and
> prevent interleaving of the events.
>
> sql/log.h@stripped, 2007-08-21 15:55:53-04:00, cbell@mysql_cab_desk. +2 -0
> BUG#29020 : Test: Event results not replicated to slave in RBR
>
> Patch adds new method declarations to initiate locks on the binlog.
>
> sql/sql_class.cc@stripped, 2007-08-21 15:55:54-04:00, cbell@mysql_cab_desk. +5 -1
> BUG#29020 : Test: Event results not replicated to slave in RBR
>
> Patch initializes thread variables for detecting when the new binlog lock
> methods are called.
>
> Adds a call to unlock the new binlog lock.
>
> sql/sql_class.h@stripped, 2007-08-21 15:55:55-04:00, cbell@mysql_cab_desk. +2 -0
> BUG#29020 : Test: Event results not replicated to slave in RBR
>
> Patch initializes thread variables for detecting when the new binlog lock
> methods are called.
>
> diff -Nrup a/sql/log.cc b/sql/log.cc
> --- a/sql/log.cc 2007-08-01 20:39:57 -04:00
> +++ b/sql/log.cc 2007-08-21 15:55:53 -04:00
> @@ -3399,10 +3399,13 @@ int THD::binlog_write_table_map(TABLE *t
>
> Table_map_log_event
> the_event(this, table, table->s->table_map_id, is_trans, flags);
> + binlog_in_trans= is_trans;
>
> if (is_trans && binlog_table_maps == 0)
> binlog_start_trans_and_stmt();
>
> + mysql_bin_log.binlog_begin_stmt();
>
Can this be put this in binlog_start_trans_and_stmt(), maybe changing
the logic here so that the function always is called, and the test is
inside the function instead?
Basically, you should not lock the binary log if you are writing to
transactional engines (those events will be put in the transaction
cache), or if there are already events in the transaction cache, only if
the table is non-transactional and the transaction cache is empty will
it be necessary.
> +
> if ((error= mysql_bin_log.write(&the_event)))
> DBUG_RETURN(error);
>
> @@ -3526,6 +3529,31 @@ MYSQL_BIN_LOG::flush_and_set_pending_row
>
> DBUG_RETURN(error);
> }
> +
> +bool MYSQL_BIN_LOG::binlog_begin_stmt()
> +{
> + bool error= 0;
> + DBUG_ENTER("MYSQL_BIN_LOG::binlog_begin_stmt()");
> + if (!current_thd->binlog_lock_taken &&
> !current_thd->binlog_in_trans)
> + {
> + current_thd->binlog_lock_taken= TRUE;
> + pthread_mutex_lock(&LOCK_log);
> + }
> + DBUG_RETURN(error);
> +}
>
current_thd is actually a macro that calls a function. It is more
efficient to do:
THD *thd= current_thd;
and then use 'thd' in the function instead of 'current_thd'.
> +
> +bool MYSQL_BIN_LOG::binlog_end_stmt()
> +{
> + bool error= 0;
> + DBUG_ENTER("MYSQL_BIN_LOG::binlog_end_stmt()");
> + if (current_thd->binlog_lock_taken)
> + {
> + pthread_mutex_unlock(&LOCK_log);
> + current_thd->binlog_lock_taken= FALSE;
> + }
> + DBUG_RETURN(error);
> +}
> +
>
> /*
> Write an event to the binary log
> diff -Nrup a/sql/log.h b/sql/log.h
> --- a/sql/log.h 2007-07-27 14:19:33 -04:00
> +++ b/sql/log.h 2007-08-21 15:55:53 -04:00
> @@ -337,6 +337,8 @@ public:
> /* Use this to start writing a new log file */
> void new_file();
>
> + bool binlog_begin_stmt();
> + bool binlog_end_stmt();
>
Some (Doxygen) comments on what the functions do and how they work?
> bool write(Log_event* event_info); // binary log write
> bool write(THD *thd, IO_CACHE *cache, Log_event *commit_event);
>
> diff -Nrup a/sql/sql_class.cc b/sql/sql_class.cc
> --- a/sql/sql_class.cc 2007-08-02 04:22:29 -04:00
> +++ b/sql/sql_class.cc 2007-08-21 15:55:54 -04:00
> @@ -375,7 +375,9 @@ THD::THD()
> bootstrap(0),
> derived_tables_processing(FALSE),
> spcont(NULL),
> - m_lip(NULL)
> + m_lip(NULL),
> + binlog_lock_taken(FALSE),
> + binlog_in_trans(FALSE)
> {
> ulong tmp;
>
> @@ -3241,6 +3243,8 @@ int THD::binlog_flush_pending_rows_event
> }
>
> error= mysql_bin_log.flush_and_set_pending_rows_event(this, 0);
> + if (stmt_end)
> + mysql_bin_log.binlog_end_stmt();
> }
>
> DBUG_RETURN(error);
> diff -Nrup a/sql/sql_class.h b/sql/sql_class.h
> --- a/sql/sql_class.h 2007-08-01 20:59:39 -04:00
> +++ b/sql/sql_class.h 2007-08-21 15:55:55 -04:00
> @@ -1119,6 +1119,8 @@ public:
> Rows_log_event* binlog_get_pending_rows_event() const;
> void binlog_set_pending_rows_event(Rows_log_event* ev);
> int binlog_flush_pending_rows_event(bool stmt_end);
> + my_bool binlog_lock_taken;
> + my_bool binlog_in_trans;
>
Comments for these variables as well
>
> private:
> uint binlog_table_maps; // Number of table maps currently in the binlog
>
>
>
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
