List:Commits« Previous MessageNext Message »
From:Alfranio Correia Date:March 10 2011 3:04pm
Subject:Re: bzr commit into mysql-trunk branch (alfranio.correia:3564) WL#4832
View as plain text  
Hi Mats,

Thank you for the review.
See some comments in-line.

On 03/10/2011 01:12 PM, Mats Kindahl wrote:
> Hi Alfranio,
> 
> Here are my comments on the code.
> 
> Some general comments:
> 
>     * Please remove any added or removed empty lines: they affect
>       merging if anybody makes changes in adjacent lines.

Ok. I will do that.

>     * Please put a space after C/C++ keywords, even for "return".

Ok. I will do that.

>     * Please do not add parentheses for the expression following
>       "return" unless the expression contain a binary operator.
> 

Any particular reason? I will do that anyway.

> 
> On 02/04/2011 03:17 PM, Alfranio Correia wrote:
>> === modified file 'sql/binlog.cc'
>> --- a/sql/binlog.cc	2011-01-05 05:21:07 +0000
>> +++ b/sql/binlog.cc	2011-02-04 14:17:40 +0000
>> @@ -35,6 +35,7 @@ const char *log_bin_basename= 0;
>>  MYSQL_BIN_LOG mysql_bin_log(&sync_binlog_period);
>>  
>>  static int binlog_init(void *p);
>> +static void binlog_start_trans_and_stmt(THD *thd, Log_event *start_event);
>>  static int binlog_close_connection(handlerton *hton, THD *thd);
>>  static int binlog_savepoint_set(handlerton *hton, THD *thd, void *sv);
>>  static int binlog_savepoint_rollback(handlerton *hton, THD *thd, void *sv);
>> @@ -85,7 +86,7 @@ class binlog_cache_data
>>  {
>>  public:
>>  
>> -  binlog_cache_data(): m_pending(0),
>> +  binlog_cache_data(): is_transactional(FALSE), m_pending(0),
>>    incident(FALSE), saved_max_binlog_cache_size(0), ptr_binlog_cache_use(0),
>>    ptr_binlog_cache_disk_use(0)
>>    { }
>> @@ -121,6 +122,11 @@ public:
>>      return(incident);
>>    }
>>  
>> +  bool has_trans_updates()
>> +  {
>> +    return(is_transactional);
>> +  }
>> +
> 
> Should probably be a constant member function since it does not change
> any state. Also, the naming suggests that the function returns true if
> the entity contain transactional updates, while is_transactional means
> that *all* updates in the cache is transactional.
> 
> Maybe name it "all_trans_updates" or "is_all_trans_updates" (I usually
> prefer to use the prefix "is_", "any_", or "all_", where I reserve
> "any_" and "all_" for entities that represents set-like objects).


Ok. I will do that.

> 
>>    virtual void reset()
>>    {
>>      compute_statistics();
>> @@ -189,6 +195,12 @@ protected:
>>      cache_log.end_of_file= saved_max_binlog_cache_size;
>>    }
>>  
>> +  /*
>> +    Defines if the cache stores transactional or non-transactional
>> +    updates.
>> +  */
>> +  bool is_transactional;
>> +
>>  private:
>>    /*
>>      Pending binrows event. This event is the event where the rows are currently
>> @@ -245,7 +257,9 @@ class binlog_trx_cache_data : public bin
>>  public:
>>    binlog_trx_cache_data() : m_cannot_rollback(FALSE),
>>    before_stmt_pos(MY_OFF_T_UNDEF)
>> -  {}
>> +  { 
>> +    is_transactional= TRUE; 
>> +  }
> 
> Is there any reason this cannot be put in the initializer list?

I think it is possible. If this is not possible, I will write a comment.

> 
>>    void reset()
>>    {
>> @@ -303,6 +317,9 @@ private:
>>      Binlog position before the start of the current statement.
>>    */
>>    my_off_t before_stmt_pos;
>> +
>> +  binlog_trx_cache_data& operator=(const binlog_trx_cache_data& info);
>> +  binlog_trx_cache_data(const binlog_trx_cache_data& info);
> 
> Thank you.
> 
>> @@ -565,10 +586,15 @@ static inline int
>>  binlog_commit_flush_stmt_cache(THD *thd,
>>                                 binlog_cache_mngr *cache_mngr)
>>  {
>> +  binlog_cache_data* cache_data= &cache_mngr->stmt_cache;
>>    Query_log_event end_evt(thd, STRING_WITH_LEN("COMMIT"),
>> -                          FALSE, TRUE, TRUE, 0);
>> -  return (binlog_flush_cache(thd, &cache_mngr->stmt_cache, &end_evt,
>> -                             FALSE));
>> +                          cache_data->has_trans_updates(), FALSE, TRUE, 0);
>> +  /*
>> +    Force the use of the stmt-cache because the current statement may
>> +    be something that makes Query_log_event to choose another cache.
>> +  */
>> +  end_evt.set_cache(cache_data->has_trans_updates());
>> +  return (binlog_flush_cache(thd, cache_data, &end_evt));
> 
> I don't understand the comment, could you please elaborate?

When we create an Query_log_event the cache is chosen based on the parse's
state, i.e. the statement that is being currently processed. In this case,
we are creating a Query_log_event("COMMIT/ROLLBACK") and want to write it
into a specific cache, regardless of the parse's state. For that reason,
we set/force the cache.

> 
> You are flushing the statement cache, but somehow you are "forcing the
> use of stmt-cache", which I don't really understand what it means. Are
> you forcing the event to end up in the statement cache?
> 
> Is there any reason you're not creating the event with the correct
> event-logging-type and event-cache-type in the constructor?


My bad. I will improve the comments.

> 
>>  }
>>  
>>  /**
>> @@ -583,10 +609,15 @@ binlog_commit_flush_stmt_cache(THD *thd,
>>  static inline int
>>  binlog_commit_flush_trx_cache(THD *thd, binlog_cache_mngr *cache_mngr)
>>  {
>> +  binlog_cache_data* cache_data= &cache_mngr->trx_cache;
>>    Query_log_event end_evt(thd, STRING_WITH_LEN("COMMIT"),
>> -                          TRUE, TRUE, TRUE, 0);
>> -  return (binlog_flush_cache(thd, &cache_mngr->trx_cache, &end_evt,
>> -                             TRUE));
>> +                          cache_data->has_trans_updates(), FALSE, TRUE, 0);
>> +  /*
>> +    Force the use of the trx-cache because the current statement may
>> +    be something that makes Query_log_event to choose another cache.
>> +  */
>> +  end_evt.set_cache(cache_data->has_trans_updates());
>> +  return (binlog_flush_cache(thd, cache_data, &end_evt));
> 
> See above.
> 
>>  }
>>  
>>  /**
>> @@ -601,10 +632,15 @@ binlog_commit_flush_trx_cache(THD *thd,
>>  static inline int
>>  binlog_rollback_flush_trx_cache(THD *thd, binlog_cache_mngr *cache_mngr)
>>  {
>> +  binlog_cache_data* cache_data= &cache_mngr->trx_cache;
>>    Query_log_event end_evt(thd, STRING_WITH_LEN("ROLLBACK"),
>> -                          TRUE, TRUE, TRUE, 0);
>> -  return (binlog_flush_cache(thd, &cache_mngr->trx_cache, &end_evt,
>> -                             TRUE));
>> +                          cache_data->has_trans_updates(), FALSE, TRUE, 0);
>> +  /*
>> +    Force the use of the trx-cache because the current statement may
>> +    be something that makes Query_log_event to choose another cache.
>> +  */
>> +  end_evt.set_cache(cache_data->has_trans_updates());
>> +  return (binlog_flush_cache(thd, cache_data, &end_evt));
> 
> See above.
> 
>> -void THD::binlog_set_stmt_begin() {
>> -  binlog_cache_mngr *cache_mngr=
>> -    (binlog_cache_mngr*) thd_get_ha_data(this, binlog_hton);
>> +    /*
>> +      Set callbacks in order to be able to call commmit or rollback.
>> +    */
>> +    if (thd->in_multi_stmt_transaction_mode())
>> +      trans_register_ha(thd, TRUE, binlog_hton);
>> +    trans_register_ha(thd, FALSE, binlog_hton);
>> +
>> +    /*
>> +      Set the binary log as read/write otherwise callbacks are not called.
>> +    */
>> +    thd->ha_data[binlog_hton->slot].ha_info[0].set_trx_read_write();
> 
> Introduce a new function in Ha_data to get the right instance? There
> is already a thd_get_ha_data, so something similar to get the
> transaction info is natural to add.
> 
>> === modified file 'sql/log_event.cc'
>> --- a/sql/log_event.cc	2011-01-28 14:49:41 +0000
>> +++ b/sql/log_event.cc	2011-02-04 14:17:40 +0000
>> @@ -684,16 +684,18 @@ const char* Log_event::get_type_str()
>>  #ifndef MYSQL_CLIENT
>>  Log_event::Log_event(THD* thd_arg, uint16 flags_arg, bool using_trans)
>>    :log_pos(0), temp_buf(0), exec_time(0), flags(flags_arg),
>> -   cache_type(Log_event::EVENT_INVALID_CACHE), crc(0), thd(thd_arg),
>> -   checksum_alg(BINLOG_CHECKSUM_ALG_UNDEF)
>> +  event_cache_type(EVENT_INVALID_CACHE),
>> +  event_logging_type(EVENT_INVALID_LOGGING),
>> +  crc(0), thd(thd_arg), checksum_alg(BINLOG_CHECKSUM_ALG_UNDEF)
>>  {
>>    server_id=	thd->server_id;
>>    when=		thd->start_time;
>>  
>>    if (using_trans)
>> -    cache_type= Log_event::EVENT_TRANSACTIONAL_CACHE;
>> +    event_cache_type= Log_event::EVENT_TRANSACTIONAL_CACHE;
>>    else
>> -    cache_type= Log_event::EVENT_STMT_CACHE;
>> +    event_cache_type= Log_event::EVENT_STMT_CACHE;
>> +  event_logging_type= Log_event::EVENT_NORMAL_LOGGING;
>>  }
>>  
>>  /**
>> @@ -724,8 +726,10 @@ Log_event::Log_event()
>>  
>>  Log_event::Log_event(const char* buf,
>>                       const Format_description_log_event* description_event)
>> -  :temp_buf(0), exec_time(0), cache_type(Log_event::EVENT_INVALID_CACHE),
>> -    crc(0), checksum_alg(BINLOG_CHECKSUM_ALG_UNDEF)
>> +  :temp_buf(0), exec_time(0),
>> +  event_cache_type(EVENT_INVALID_CACHE),
>> +  event_logging_type(EVENT_INVALID_LOGGING),
>> +  crc(0), checksum_alg(BINLOG_CHECKSUM_ALG_UNDEF)
>>  {
>>  #ifndef MYSQL_CLIENT
>>    thd = 0;
>> @@ -918,8 +922,8 @@ my_bool Log_event::need_checksum()
>>    ret= (checksum_alg != BINLOG_CHECKSUM_ALG_UNDEF) ?
>>      (checksum_alg != BINLOG_CHECKSUM_ALG_OFF) :
>>      ((binlog_checksum_options != BINLOG_CHECKSUM_ALG_OFF) &&
>> -     (cache_type == Log_event::EVENT_NO_CACHE))? binlog_checksum_options :
>> -    FALSE;
>> +     (event_cache_type == Log_event::EVENT_NO_CACHE))?
>> +    binlog_checksum_options : FALSE;
> 
> Ugh... I would prefer if this could be written using if-statements as
> you did in the hunk before this one. It will not affect the code, and
> possibly even make it more efficient since it gives more freedom to
> the compiler to optimize.

ok. I will do that.

> 
>> @@ -2688,23 +2692,20 @@ Query_log_event::Query_log_event()
>>  }
>>  
>>  
>> -/*
>> -  SYNOPSIS
>> -    Query_log_event::Query_log_event()
>> -      thd_arg           - thread handle
>> -      query_arg         - array of char representing the query
>> -      query_length      - size of the  `query_arg' array
>> -      using_trans       - there is a modified transactional table
>> -      suppress_use      - suppress the generation of 'USE' statements
>> -      errcode           - the error code of the query
>> -      
>> -  DESCRIPTION
>> -  Creates an event for binlogging
>> -  The value for `errcode' should be supplied by caller.
>> +/**
>> +  Creates an event for binlogging.
>> +
>> +  @param thd_arg      Thread handle
>> +  @param query_arg    Array of char representing the query
>> +  @param query_length Size of the  `query_arg' array
>> +  @param using_trans  There is a modified transactional table
>> +  @param immediate    Should be flushed immediately
>> +  @param suppress_use Suppress the generation of 'USE' statements
>> +  @param errcode      The error code of the query
>>  */
>>  Query_log_event::Query_log_event(THD* thd_arg, const char* query_arg,
>>  				 ulong query_length, bool using_trans,
>> -				 bool direct, bool suppress_use, int errcode)
>> +				 bool immediate, bool suppress_use, int errcode)
>>  
>>    :Log_event(thd_arg,
>>               (thd_arg->thread_specific_used ? LOG_EVENT_THREAD_SPECIFIC_F :
>> @@ -2804,7 +2805,8 @@ Query_log_event::Query_log_event(THD* th
>>      Note that a cache will not be used if the parameter direct is TRUE.
>>    */
>>    bool trx_cache= FALSE;
>> -  cache_type= Log_event::EVENT_INVALID_CACHE;
>> +  event_cache_type= Log_event::EVENT_INVALID_CACHE;
>> +  event_logging_type= Log_event::EVENT_INVALID_LOGGING;
> 
> Can't this be put in the initializer list?

Sure.

> 
> [snip]
> 
>> @@ -5489,7 +5501,8 @@ Rotate_log_event::Rotate_log_event(const
>>    DBUG_PRINT("enter",("new_log_ident: %s  pos: %s  flags: %lu",
> new_log_ident_arg,
>>                        llstr(pos_arg, buff), (ulong) flags));
>>  #endif
>> -  cache_type= EVENT_NO_CACHE;
>> +  event_cache_type= EVENT_NO_CACHE;
>> +  event_logging_type= EVENT_IMMEDIATE_LOGGING;
> 
> Initializer list?

Sure.

> 
> [snip]
> 
>> === modified file 'sql/log_event.h'
>> --- a/sql/log_event.h	2011-01-11 05:13:23 +0000
>> +++ b/sql/log_event.h	2011-02-04 14:17:40 +0000
>> @@ -920,9 +920,10 @@ public:
>>      EVENT_SKIP_COUNT
>>    };
>>  
>> +protected:
>>    enum enum_event_cache_type 
>>    {
>> -    EVENT_INVALID_CACHE,
>> +    EVENT_INVALID_CACHE= 0,
>>      /* 
>>        If possible the event should use a non-transactional cache before
>>        being flushed to the binary log. This means that it must be flushed
>> @@ -937,15 +938,35 @@ public:
>>      EVENT_TRANSACTIONAL_CACHE,
>>      /* 
>>        The event must be written directly to the binary log without going
>> -      through a cache.
>> +      through any cache.
>>      */
>>      EVENT_NO_CACHE,
>> -    /**
>> +    /*
>>         If there is a need for different types, introduce them before this.
>>      */
>>      EVENT_CACHE_COUNT
>>    };
>>  
>> +  enum enum_event_logging_type
>> +  {
>> +    EVENT_INVALID_LOGGING= 0,
>> +    /*
>> +      The event must be written to a cache and upon commit or rollback
>> +      written to the binary log.
>> +    */
>> +    EVENT_NORMAL_LOGGING,
>> +    /*
>> +      The event must be written to an empty cache and immediatly written
>> +      to the binary log without waiting for any other event.
>> +    */
>> +    EVENT_IMMEDIATE_LOGGING,
>> +    /*
>> +       If there is a need for different types, introduce them before this.
>> +    */
>> +    EVENT_CACHE_LOGGING_COUNT
>> +  };
>> +
>> +public:
>>    /*
>>      The following type definition is to be used whenever data is placed 
>>      and manipulated in a common buffer. Use this typedef for buffers
>> @@ -996,22 +1017,28 @@ public:
>>    */
>>    uint16 flags;
>>    
>> -  /*
>> -    Defines the type of the cache, if any, where the event will be
>> -    stored before being flushed to disk.
>> -  */
>> -  uint16 cache_type;
>> -
>>    /**
>>      A storage to cache the global system variable's value.
>>      Handling of a separate event will be governed its member.
>>    */
>>    ulong slave_exec_mode;
>>  
>> +  /*
>> +    Defines the type of the cache, if any, where the event will be
>> +    stored before being flushed to disk.
>> +  */
>> +  uint16 event_cache_type;
>> +
>> +  /*
>> +    Defines when information, i.e. event or cache will be stored into
>> +    disk.
>> +  */
>> +  uint16 event_logging_type;
> 
> Maybe use Doxygen comments?
> 
> Why are you using uint16 instead of enum_event_logging_type?
> 
> Why are you using uint16 instead of enum_event_cache_type?

I kept the old definition but I will change it following your suggestions/requests.

Cheers.

Thread
bzr commit into mysql-trunk branch (alfranio.correia:3564) WL#4832Alfranio Correia4 Feb
  • Re: bzr commit into mysql-trunk branch (alfranio.correia:3564) WL#4832Mats Kindahl10 Mar
    • Re: bzr commit into mysql-trunk branch (alfranio.correia:3564) WL#4832Alfranio Correia10 Mar
      • Re: bzr commit into mysql-trunk branch (alfranio.correia:3564) WL#4832Mats Kindahl10 Mar
  • Re: bzr commit into mysql-trunk branch (alfranio.correia:3564) WL#4832Luís Soares15 Mar
    • Re: bzr commit into mysql-trunk branch (alfranio.correia:3564) WL#4832Alfranio Correia17 Mar