Hi Mats,
Thanks for looking at this for me. I have one comment about one of your
suggestions. I will have a patch ready for review today.
Chuck
> 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?
I could not get this to work. The binlog_start_trans_and_stmt() is also
called from MYSQL_BINLOG::write(). Several tests failed and the server
crashed when I moved the binlog_begin_stmt() into the method with the guard.
I think for now I will leave it as is.
>
> 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
>
>
