Hi Jason,
Here are my comments on the patch.
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-17 11:04:54 +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/log.h
> sp1f-log.h-20051222053446-ggv6hdi5fnxggnjemezvv7n2bcbkx45e
> 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
>
>
> ------------------------------------------------------------------------
>
>
+struct st_mysql_replication {
+ int interface_version;
+};
We have deprecated the use of st_* names in favor of names of the form
below (capital first letter, _ to separate words) for all types.
struct Mysql_replication {
int interface_version;
};
Don't bother about the other names right now: we have to take care of
them as well, but that is a separate problem.
Passing in the address of a variable is a good idea, but if it is common
that the variable is not used, it is common to allow passing NULL as
address, and the function will not fill in that variable. Unless I
misunderstand the code, you have several cases where the value of the
variable is not used, so it is sensible to allow NULL to be passed.
Could you please add a Doxygen comment for the flush_and_sync()
function? We need to get the function documentation in order, and I
think it is best to fill in a description when one has been going over
the function and have it fresh in mind how it works.
We have generally decided to use the @retval tag for Doxygen comments
and only use @return when the return value cannot be easily tabulated: so:
@retval 0 Success
@retval 1 Failure
*/
int (*after_commit)(Trans_param *param);
After some discussions with Kostja, the form "typedef struct X { ... }
X;" is discouraged, we should use the form "struct X { ... };" instead
for all structures and classes.
HAVE_REPLICATION is defined if the server can act as a replication
*slave*, it should not be used to remove binlogging code (i.e., prevent
the server from acting as a replication *master*). The constant is not
used anyway, and I doubt if it will work to compile the code without
that symbol defined.
+/*
+ NOTE2ME: I (hezx) am not sure if it's correct to add observer
+ plugins to the thd->lex list. Because afer each statement, all
+ plugins add to thd->lex will be automatically unlocked.
+ */
Unlocking the plug-ins just mean that it is possible to unload them
after the statement has finished. AFAICT, this cannot lead to a problem
since you are not keeping any references to loaded plug-ins around. The
only thing you have to make sure of is that there is a "statement-end"
call even in the slave thread, so that the plug-ins are not locked into
the slave server forever.
Could you please add a Doxygen comment explaining a little more about
the macro: especially the magic read_lock() and unlock() calls, which
look very strange unless you start looking at from where the macro is
called.
+#define FOREACH_OBSERVER(f, thd, args...) \
+ param.server_id= thd->server_id; \
The "args..." format only exist for GNU compilers, and we need to be
able to work with Intel, Sun, and HP compilers as well. The C99 form of
variadic parameters is not well-supported, so I suggest that you replace
the last macro parameter with a non-variadic parameter.
Can all the functions in the replication.h file be called as C
functions? There are plenty of C users that want to use these. Just to
make sure, can you please write a small C file that just includes the
header file and call one of the functions in it and test that it
compiles. I see that they are declared as extern "C" functions, so I
don't expect any problems, but could you just please check. Could you
also do an "nm" on the "rpl_handler.o" file and make sure that the names
of the C functions are not mangled?
It is possible to forget to initialize the delegates properly since
there is a default constructor that does not initialize the instances.
Could you instead use placement new to construct the instances in-memory
and assign the result of the placement new to distinct pointers to the
delegates and put the code that is now in init() inside the default
constructor instead? Like this:
header file
-----------
extern Trans_delegate *transaction_delegate;
Source file
-----------
static unsigned char memory[sizeof(Trans_delegate)];
...
transaction_delegate= new (memory) Trans_delegate;
...
Destruction is then handled with
transaction_delegate->~Trans_delegate();
That would make it easy to find a missing initialization, since the
pointer will be NULL.
- { C_STRING_WITH_LEN("AUDIT") }
+ { C_STRING_WITH_LEN("AUDIT") },
+ { C_STRING_WITH_LEN("REPLICATION") }
Keep a comma after the last item in an initializer list. This means you
don't have to change the last line the next time you add something.
int a[] = { 1, 2, 3, 4, };
is legal C and C++ code.
+
+ ulong ev_offset;
+
If somebody defines and registers a function that does reserve_header,
but forget to set ev_offset to something, of there is no call to that,
it seems as if ev_offset will have a random value. Please give it a
default value to avoid any corruption, and also to eliminate the
suspicion of incorrect code.
@@ -620,17 +642,25 @@
log's filename does not change while it's active
*/
if (coord)
- coord->pos= uint4korr(packet->ptr() + 1 + LOG_POS_OFFSET);
+ coord->pos= uint4korr(packet->ptr() + ev_offset + LOG_POS_OFFSET);
- if ((*packet)[EVENT_TYPE_OFFSET+1] == FORMAT_DESCRIPTION_EVENT)
+ event_type= (Log_event_type)((*packet)[LOG_EVENT_OFFSET+ev_offset]);
+ if (event_type == FORMAT_DESCRIPTION_EVENT)
{
- binlog_can_be_corrupted= test((*packet)[FLAGS_OFFSET+1] &
+ binlog_can_be_corrupted= test((*packet)[FLAGS_OFFSET+ev_offset] &
LOG_EVENT_BINLOG_IN_USE_F);
- (*packet)[FLAGS_OFFSET+1] &= ~LOG_EVENT_BINLOG_IN_USE_F;
+ (*packet)[FLAGS_OFFSET+ev_offset] &= ~LOG_EVENT_BINLOG_IN_USE_F;
}
- else if ((*packet)[EVENT_TYPE_OFFSET+1] == STOP_EVENT)
+ else if (event_type == STOP_EVENT)
binlog_can_be_corrupted= FALSE;
Normally, itt is not possible to reserve extra space in
Format_description_log_event, Start_v3_log_event, or Rotate_log_event,
so the code that seems to allow that looks highly suspicious. Is that
really safe?
@@ -649,7 +678,20 @@
goto err;
}
}
- packet->set("\0", 1, &my_charset_bin);
+
+ if (RUN_HOOK(binlog_transmit, after_send_event, thd, flags,
packet->ptr()))
+ {
+ errmsg= "Failed to run hook 'after_send_event'";
+ my_errno= ER_UNKNOWN_ERROR;
+ goto err;
+ }
+
+ 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;
+ }
}
Why do you call the reserve_header after the event has been sent? That
looks strange.
@@ -779,7 +821,15 @@
if (read_packet)
{
- thd_proc_info(thd, "Sending binlog event to slave");
+ thd_proc_info(thd, "Sending binlog event to slave");
+ pos = my_b_tell(&log);
+ if (RUN_HOOK(binlog_transmit, before_send_event,
+ thd, flags, packet, log_file_name, pos))
+ {
+ errmsg= "run 'before_send_event' hook failed";
+ goto err;
+ }
+
Check indentation.
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
