List:Replication« Previous MessageNext Message »
From:周振兴 Date:September 7 2011 4:16pm
Subject:Re: Semi-sync patch from orczhou
View as plain text  
Hi Libing Song

Thanks for your explanation. Now i know what "is_real_trans" is for. And
after took a deeper dives into the source code, i think i almost understand
why
"all || thd->transaction.all.ha_list == 0" can works.

Here is the my understand:
I check the stack of binlog_commit, it something like this
>binlog_commit (handlerton *hton, THD *thd, bool all)
>ha_commit_one_phase (THD *thd, bool all)
>ha_commit_trans (THD *thd, bool all)
>trans_commit_stmt || trans_begin || trans_commit_implicit ||
trans_xa_commit
and I notice only when trans_commit_stmt, the “all” is false.

So, the parameter "*all*" is cool and enough to check it's in a "real
transaction" or a “statement transactions”.

A new commit has been done on launchpad (use the parameter "*all*"):
http://bazaar.launchpad.net/~orczhou/mysql-server/ESR/revision/3490?start_revid=3490

Still i am working on the test-case.

On Wed, Sep 7, 2011 at 6:27 PM, anders <anders.song@stripped>wrote:

> Hi 周振兴,
> On Wed, 2011-09-07 at 15:40 +0800, 周振兴 wrote:
> > Hi Libing Song
> >
> >
> > Some modification has been commit on Launchpad.net.
> > https://code.launchpad.net/~orczhou/mysql-server/ESR. And i am working
> > on the test-case now and it will be finished in this week.
> >
> >
> > I am going to forward this mail to "replication@stripped"
> > to discuss with more people, Is that proper or necessary ?
> >
> It is ok.
> >
> > Changes of this commit are noted below in red font:
> >
> >
> > On Thu, Sep 1, 2011 at 10:58 AM, anders
> > <anders.song@stripped> wrote:
> >         Hi orczhou,
> >
> >         It is a really great idea to implement the feature and nice
> >         work!
> >
> >         After a careful review, I got some comments.
> >
> >         First of all, I'd like to advice you to commit the patch to
> >         commits@stripped. and then link it to
> >         http://bugs.mysql.com/bug.php?id=62174. It will be easy for
> >         others
> >         to follow it.
> >
> >         It will be better, if you can provide a test case to verify
> >         your patch.
> >         mysql-test/suite/rpl/t/rpl_semi_sync.test is an example that
> >         you can
> >         refer to.
> >
> >         Detail comments are below, please check them.
> >
> >         On Wed, 2011-08-31 at 10:29 +0800,
> >         songlibing@stripped
> >         wrote:
> >         > === modified file 'plugin/semisync/semisync_master.cc'
> >         > --- plugin/semisync/semisync_master.cc        2011-06-30
> >         15:46:53 +0000
> >         > +++ plugin/semisync/semisync_master.cc        2011-08-31
> >         01:55:31 +0000
> >         > @@ -24,6 +24,7 @@
> >         >
> >         >  /* This indicates whether semi-synchronous replication is
> >         enabled. */
> >         >  char rpl_semi_sync_master_enabled;
> >         > +char rpl_semi_sync_master_wait_before_commit;
> >         >  unsigned long rpl_semi_sync_master_timeout;
> >         >  unsigned long rpl_semi_sync_master_trace_level;
> >         >  char rpl_semi_sync_master_status                    = 0;
> >         >
> >         > === modified file 'plugin/semisync/semisync_master.h'
> >         > --- plugin/semisync/semisync_master.h 2011-06-30 15:46:53
> >         +0000
> >         > +++ plugin/semisync/semisync_master.h 2011-08-31 01:55:31
> >         +0000
> >         > @@ -592,6 +592,7 @@
> >         >
> >         >  /* System and status variables for the master component */
> >         >  extern char rpl_semi_sync_master_enabled;
> >         > +extern char rpl_semi_sync_master_wait_before_commit;
> >         >  extern char rpl_semi_sync_master_status;
> >         >  extern unsigned long rpl_semi_sync_master_clients;
> >         >  extern unsigned long rpl_semi_sync_master_timeout;
> >         >
> >         > === modified file
> >         'plugin/semisync/semisync_master_plugin.cc'
> >         > --- plugin/semisync/semisync_master_plugin.cc 2011-06-30
> >         15:46:53 +0000
> >         > +++ plugin/semisync/semisync_master_plugin.cc 2011-08-31
> >         01:55:47 +0000
> >         > @@ -43,6 +43,24 @@
> >         >    return error;
> >         >  }
> >         >
> >         > +int repl_semi_after_binlog_commit(Binlog_storage_param
> >         *param,
> >         > +                                const char *log_file,
> >         > +                                my_off_t log_pos, uint32
> >         flags)
> >         I think it is better to keep the interface same to
> >         repl_semi_report_commit.
> > I have change the name "after_binlog_commit" to report_binlog_commit".
> > Yep, it's better and it's good to keep consistent with original code.
> >
> >         +int repl_semi_report_binlog_commit(Trans_param *param)
> Not only the name, but the arguments should be changed as well.
>
> >
> >         > +{
> >         > +  if(!rpl_semi_sync_master_wait_before_commit)
> >         > +    return 0;
> >         > +  THD *thd= current_thd;
> >         It is not a good idea to use thd object here. As
> >         it is a plugin and we want it know MySQL as less as possible.
> > I'm not sure about this. Actually i copy the code (THD *thd=
> > current_thd;) just from function "repl_semi_after_send_event"  in the
> > same file.
> > If this really matters, I can move the flag-checking to function: int
> > Binlog_storage_delegate::after_binlog_commit(THD *thd). I have not
> > change this in this commit.
> It is not really matter, but I prefer not to use thd here.
> So I'd like to use a Trans_param as argument(See above), then we don't
> need to check thd->transaction.all.ha_list in the function.
> >
> >         > +  bool is_real_trans = thd->transaction.all.ha_list == 0;
> >         Only checking ha_list is not enough to know whether it is
> >         committing/rolling back a real transaction.
> >         Because the function is called in binlog_commit. At the
> >         moment,
> >         other engines don't commit yet and all.ha_list is not cleared
> >         either.
> >         For a real transaction, all.ha_list is never null when calling
> >         the
> >         function.
> >         > +  //bool is_real_trans= param->flags &
> TRANS_IS_REAL_TRANS;
> >         So, we need this flag. That is why I want to change the
> >         function's
> >         interface.
> > In my understand,  "thd->transaction.all.ha_list == 0" is enough to
> > decide whether this is a real transaction. I'm not that sure about
> > this,but i notice that
> > "thd->transaction.all.ha_list == 0" is almost identical to "bool
> > is_real_trans= param->flags & TRANS_IS_REAL_TRANS" (check this :int
> > Trans_delegate::after_commit(THD *thd, bool all))
> > So, i don't not change anything here. Is this ok? or we need ask
> > someone else to reviews this?
> You might missed something, so let me to explain it again.
>
> after_commit hook is called not only when a real trx committed, but at
> the end of each statement(only DDL and DML), no matter it is in a
> transaction or not.
> So the behaviors are like:
> BEGIN;
> INSERT ...; call after_commit hook at the end of INSERT statement
> INSERT ...; call after_commit hook at the end of INSERT statement
> COMMIT; call after_commit hook at the end of COMMIT statement
>
> // autocommit mode
> INSERT a trx table; call after_commit hook
> INSERT a non-trx table; call after_commit hook
>
> CREATE TABLE ...(DDL statements); call after_commit hook;
>
> According to the behaviors, repl_semi_report_commit is called at the
> end of each DDL and DML. It is possible that it is called by a statement
> which is in a transaction, but not a COMMIT statement. If it is true,
> semisync should not wait.
> Because of this, repl_semi_report_commit has to know if it is still in
> an active trx. That is why we have the is_real_trans. But its name is
> not accurate to express its meaning. The exact meaning should be 'is a
> real trans ending' or 'it is not in an active trx'.
>
> Now, let me explain why 'thd->transaction.all.ha_list == 0' can be used
> in repl_semi_report_commit. In a real transaction, all.ha_list is used
> like:
> BEGIN;
> INSERT a trx table trx1; Put trx1's engine handler in all.ha_list;
> INSERT a trx table trx2; Put trx2's engine handler in all.ha_list;
> COMMIT; foreach 'engine in all.ha_list' do prepare all engines, commit
> all engines and then clear all.ha_list;
> So all.ha_list is not NULL if after_commit hook is called by a INSERT
> statement. And all.ha_list is alway NULL if after_commit hook is called
> by a COMMIT statement. In autocommit mode, all.ha_list is always NULL.
> That is why 'thd->transaction.all.ha_list == 0' can be used in
> repl_semi_report_commit. But it cannot be used in after_binlog_commit
> hook, because all.ha_list is not set to NULL when after_binlog_commit
> hook is called. so it will never wait if it checks only all.ha_list.
>
> It is tricky, isn't it :-)? The code is in ha_commit_one_phase, you can
> check it.
>
> It can be solved by using
> bool is_real_trans= (all || thd->transaction.all.ha_list == 0);
>
>
> >         > +
> >         > +  if (is_real_trans && log_pos)
> >         > +  {
> >         > +    const char *binlog_name= log_file;
> >         > +    return repl_semisync.commitTrx(binlog_name, log_pos);
> >         > +  }
> >         > +  return 0;
> >         > +}
> >
> >         [snip]
> >         > @@ -256,6 +288,7 @@
> >         >    sizeof(Binlog_storage_observer), // len
> >         >
> >         >    repl_semi_report_binlog_update, // report_update
> >         > +  repl_semi_after_binlog_commit, // Just after
> >         binlog_commit
> >         >  };
> >         >
> >         I prefer to add repl_semi_report_binlog_commit trans_observer.
> >         But it is not a strong opinions.
> > I don't think it'a good idea to add repl_semi_report_binlog_commit to
> > trans_observer. The target of this HOOK is trying to trace the binary
> > log' activity, and it's not related to transaction object. Actually in
> In my opinion, binlog_commit is not a binlog activity. Binlog activities
> are some operations on the binlog file, for example read, write, flash
> etc. binlog_commit is a transaction operation. We can observe whole
> transaction's activities, why we cannot observe each engine's
> transaction activities?
> However, it not a big deal. You can do what you want, it is fine for me.
> >  MySQL, "binlog" is a "transaction engine", so i don't think it'a good
> > idea to call the HOOK in binlog_commit something like
> > "rpl_semi_before_commit".
> >
> >
> > That's why i put the HOOK in "Binlog_storage_observer" but not
> > "Trans_observer".
> >
> >
>
>
>
>
>
>


-- 
此致
    敬礼
-----------------------------------------------------
周振兴  159-9004-0105 Taobao.com
http://orczhou.com

Thread
Re: Semi-sync patch from orczhou周振兴7 Sep
Re: Semi-sync patch from orczhouanders8 Sep