List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:June 24 2008 5:24am
Subject:Re: bzr commit into mysql-6.0-semi-sync-1.0 tree (hezx:2635)
WL#4398
View as plain text  
On 2008-06-23 Mon 21:36 +0200, Mats Kindahl wrote:
> 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.
> 

OK

> 
> 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.
> 

I don't know which ones you mean, could you please point them out?

> 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.
> 

OK

> 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);
> 

Good

> 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.
> 

OK

> 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.
> 

I think that's what I did, Trans_* and Binlog_storage interfaces are
always available. I only use HAVE_REPLICATION to surround
Binlog_transmit_* and Binlog_relay_IO_* interfaces.  Maybe I missed
something here, could you please point them out for me?

> +/*
> +  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.
> 

OK,then do you think I should keep the comment or not.

> 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;				\
> 

This macros is use internally for implementation, I can add comment to
explain more about it. But this could be changed and other part of the
code is discouraged to use this macro. Do you really think I need add a
Doxygen comment about it? 

> 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.
> 

Hmm... If I cannot use variadic parameters in the macro, then it seems I
have to remove the macro and copy the codes in the macro to all of the
functions. Do you have any good idea to prevent this copy and past work?

> 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?
> 

I have checked that the functions are not mangled in rpl_handler.o. But
inorder to make the small C file to compile, I have to declare the
following data types:
  ulong, uint, uint32, my_off_t, MYSQL, String

These is not problem for other types, but 'String' might be a problem.
We did not discussing 'String' type when reviewing the architect of the
interface. I think we should not export this internal type. This is used
by reserve_header, and before_send_event. It can be easily replaced with
'char *' for before_send_event, but requires some work to remove it from
reserve_header.

> 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.
> 

Good

> -  { 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.
> 

OK

> +
> +  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.
> 

ev_offset is set a default value '1' in
Binlog_transmit_delegate::reserve_header

> @@ -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?
> 

This will not reserve extra space in the event, the extra space is
reserved as the packet header, so there should be no problem.

> @@ -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.
> 

This is the end of the while loop, and the header is reserved for the
next event read from log file.

> @@ -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.

OK

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

Thread
bzr commit into mysql-6.0-semi-sync-1.0 tree (hezx:2635) WL#4398He Zhenxing17 Jun
  • Re: bzr commit into mysql-6.0-semi-sync-1.0 tree (hezx:2635) WL#4398Mats Kindahl23 Jun
    • Re: bzr commit into mysql-6.0-semi-sync-1.0 tree (hezx:2635)WL#4398He Zhenxing24 Jun
      • Re: bzr commit into mysql-6.0-semi-sync-1.0 tree (hezx:2635) WL#4398Mats Kindahl24 Jun
        • Re: bzr commit into mysql-6.0-semi-sync-1.0 tree(hezx:2635) WL#4398He Zhenxing24 Jun
          • Re: bzr commit into mysql-6.0-semi-sync-1.0 tree (hezx:2635) WL#4398Mats Kindahl24 Jun