Re: bzr commit into mysql-6.0-semi-sync-1.0 tree (hezx:2635) WL#4398

[He Zhenxing]

Hi Mats

Did you noticed that I included <my_global.h> in plugin.h? 

The functions I added in plugin.h used some typedefs defined 
in it, such as uint32, uchar. I am not quite sure other stuffs defined
in my_global.h should be exported or not.

See below.

On 2008-06-05 Thu 15:05 +0200, Mats Kindahl wrote:
> Hi Jason!
> 
> I don't have many big comments, just a bunch of minor ones. See below.
> 
> Just my few cents,
> Mats Kindahl
> 
> He Zhenxing wrote:
> > #At file:///media/sda3/work/mysql/bzrwork/semisync/mysql-6.0-semi-sync-1.0/
> > 
> > ------------------------------------------------------------
> > revno: 2635
> > revision-id: hezx@stripped
> > parent: sp1r-kostja@bodhi.(none)-20080420124250-11357
> > committer: He Zhenxing <hezx@stripped>
> > branch nick: mysql-6.0-semi-sync-1.0
> > timestamp: 四 2008-06-05 17:05:38 +0800
> > message:
> >   WL#4398 Replication Interface for semi-synchronous replication
> >   
> >   Add replication interface and plugin support
> > added:
> >   sql/replication.h             
> replication.h-20080520140908-iazlmzffc1xcnz71-2
> >   sql/rpl_handler.cc            
> rpl_handler.cc-20080520140908-iazlmzffc1xcnz71-1
> >   sql/rpl_handler.h             
> rpl_handler.h-20080520140902-36338uea7oevomai-1
> > modified:
> >   include/mysql/plugin.h        
> sp1f-plugin.h-20051105112032-xacmvx22ghtcgtqhu6v56b4bneqtx7l5
> >   libmysqld/Makefile.am         
> sp1f-makefile.am-20010411110351-26htpk3ynkyh7pkfvnshztqrxx3few4g
> >   sql/Makefile.am               
> sp1f-makefile.am-19700101030959-xsjdiakci3nqcdd4xl4yomwdl5eo2f3q
> >   sql/handler.cc                
> sp1f-handler.cc-19700101030959-ta6zfrlbxzucylciyro3musjsdpocrdh
> >   sql/log.cc                    
> sp1f-log.cc-19700101030959-r3hdfovek4kl6nd64ovoaknmirota6bq
> >   sql/mysqld.cc                 
> sp1f-mysqld.cc-19700101030959-zpswdvekpvixxzxf7gdtofzel7nywtfj
> >   sql/slave.cc                  
> sp1f-slave.cc-19700101030959-a636aj3mjxgu7fnznrg5kt77p3u2bvhh
> >   sql/sql_parse.cc              
> sp1f-sql_parse.cc-19700101030959-ehcre3rwhv5l3mlxqhaxg36ujenxnrcd
> >   sql/sql_plugin.cc             
> sp1f-sql_plugin.cc-20051105112032-hrm64p6xfjq33ud6zy3uivpo7azm75a2
> >   sql/sql_repl.cc               
> sp1f-sql_repl.cc-20001002032713-xqbns5ofqsaebhgi2ypcfn7nhz7nh5rp
> > per-file comments:
> >   include/mysql/plugin.h
> >     Add replication plugin support and some API functions
> >   sql/Makefile.am
> >     Add replication interface support
> >   sql/handler.cc
> >     Add replication interface support
> >   sql/log.cc
> >     Add replication interface support
> >   sql/mysqld.cc
> >     Add replication interface support
> >   sql/replication.h
> >     Add replication interface support
> >   sql/rpl_handler.cc
> >     Add replication interface support
> >   sql/rpl_handler.h
> >     Add replication interface support
> >   sql/slave.cc
> >     Add replication interface support
> >   sql/sql_parse.cc
> >     Add replication interface support
> >   sql/sql_plugin.cc
> >     Add replication plugin support
> >   sql/sql_repl.cc
> >     Add replication interface support
> > 
> > 
> > ------------------------------------------------------------------------
> > 
> > 
> 
> > +Trans_delegate transaction_delegate;
> > +Binlog_storage_delegate binlog_storage_delegate;
> > +#ifdef HAVE_REPLICATION
> > +Binlog_transmit_delegate binlog_transmit_delegate;
> > +Binlog_relay_IO_delegate binlog_relay_io_delegate;
> > +#endif /* HAVE_REPLICATION */
> 
> These are declared extern (i.e., have external linkage), but are not
> used anywhere outside the file (as far as I can tell). I suggest you
> assign them internal linkage by declaring them static and remove the
> extern declarations from the header file. It is easy to make a variable
> with internal linkage have external linkage, but the opposite can be
> problematic.
> 

They are used externally, I hided it in the following macro :)

#define RUN_HOOK(group, hook, args...)		\
  group ##_delegate.hook(args)


> 
> > +uint32 thd_server_id(MYSQL_THD thd)
> > +{
> > +  return thd_to_mysql_thd(thd)->server_id;
> > +}
> > +
> 
> Should be mysql_thd_to_thd(thd)->server_id;
> 

Right, thanks

> Very nice handling of keeping the NET structure within the server: now
> the only communication involves the actual bytes sent and received. Very
> good.
> 

Thanks!

> > +int get_user_var_str(MYSQL_THD thd, const char *name,
> > +		     char *value, size_t len, uint precision, my_bool *null_value)
> > +{
> > +  String str;
> > +  my_bool null_value_;
> > +  if (!null_value)
> > +    null_value= &null_value_;
> 
> Avoid using a trailing _ to make variables as local: it is easy to miss
> that underscore when reading the code. The convention is to use "_arg"
> to mark parameters to functions, if you want to "reuse" the name locally.
> 

OK, will be changed.

> > +  user_var_entry *entry=
> > +    (user_var_entry*) hash_search(&mysql_thd_to_thd(thd)->user_vars,
> (uchar*) name,
> > +                                  strlen(name));
> > +  if (entry)
> > +  {
> > +    entry->val_str(null_value, &str, precision);
> > +    strncpy(value, str.c_ptr(), len);
> > +  }
> > +  else
> > +  {
> > +    *value= 0;
> > +  }
> > +  return 0;
> > +}
> 
> I would suggest to return an error code from the function if the
> variable is not found. Otherwise, it is impossible to distinguish
> between a variable that exists but is the empty string, and a variable
> that does not exist.

Actually the out parameter `null_value' is used for this purpose, maybe
I should change this to use the return value as an indicator of if the
variable is defined (return 0 on success, 1 if variable not found). and
remove `null_value' parameter.

int get_user_var_int(MYSQL_THD thd, const char *name, longlong *value);
int get_user_var_real(MYSQL_THD thd, const char *name, double *value);
int get_user_var_str(MYSQL_THD thd, const char *name,
		     char *value, size_t len, uint precision);

> 
> Also, I usually prefer to not touch the variable if there is an error
> (it's pointless to introduce a memory lock when the result will not be
> used), but that is me.
> 
> In other words, I think something like this would be better.
> 
>   if (!entry)
>     return 1;             /* No variable by that name found */
> 
>   entry->val_str(null_value, &str, precision);
>   strncpy(value, str.c_ptr(), len);
>   return 0;
> 

OK

> +int delegates_init()
> +{
> 
> Maybe you should call this function "rpl_delegates_init()" (and the
> "delegates_destroy()" similarly changed) since there conceivably can be
> other delegates in the future that are not related to replication.
> 

You're right, and event now not all the delegates are related to
replication, the Transaction_delegate, for example, can be used for
other purpose, so I omitted the `rpl' prefix intentionally. If we want
to add new delegates in the future, they should be intialized here too.

> > +template <typename T>
> > +class Observer_info_ {
> > +public:
> 
> Avoid the trailing underscore, they are easy to miss.

OK

> 
> Template classes are not allowed according to the coding standard, with
> some exceptions for, e.g., the List template class (sorry).
> 

I don't see this on the coding standard:
http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines

Is there an other coding standard?

> -  ha_autocommit_or_rollback(thd, thd->is_error());
> +  if (thd->transaction.stmt.ha_list)
> +  {
> +    ha_autocommit_or_rollback(thd, thd->is_error());
> +  }
> +  else
> +  {
> +    if (!thd->is_error())
> +    {
> +      RUN_HOOK(transaction, after_commit, thd, 0);
> +    }
> +    else
> +    {
> +      RUN_HOOK(transaction, after_rollback, thd, 0);
> +    }
> +  }
> 
> You can skip the braces for single-line bodies: they are not mandatory
> (and actually the recommendation is to not use them unless necessary for
> readability reasons).
> 

OK

> -  /*
> -    We need to start a packet with something other than 255
> -    to distinguish it from error
> -  */
> -  packet->set("\0", 1, &my_charset_bin); /* This is the start of a new
> packet */
> +  if (RUN_HOOK(binlog_transmit, reserve_header, thd, flags, packet,
> &ev_offset))
> +  {
> +    errmsg= "Failed to run hook 'reserve_header'";
> +    my_errno= ER_UNKNOWN_ERROR;
> +    goto err;
> +  }
> 
> Where did the "packet->set("\0", .." go? I can't find it anywhere.
> 
> Ah, found it as an append() elsewhere. I suggest to use "set()" instead,
> it is more efficient.
> 

OK

> 
> -- 
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=1