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.
> +uint32 thd_server_id(MYSQL_THD thd)
> +{
> + return thd_to_mysql_thd(thd)->server_id;
> +}
> +
Should be mysql_thd_to_thd(thd)->server_id;
Very nice handling of keeping the NET structure within the server: now
the only communication involves the actual bytes sent and received. Very
good.
> +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.
> + 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.
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;
+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.
> +template <typename T>
> +class Observer_info_ {
> +public:
Avoid the trailing underscore, they are easy to miss.
Template classes are not allowed according to the coding standard, with
some exceptions for, e.g., the List template class (sorry).
- 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).
- /*
- 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.
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
