maria-developers team mailing list archive

[Jonas Oreland <jonaso@xxxxxxxxxx>]

Hi there,

I think it might be easier you just took and applied it to your favorite
branch (hence i'll won't send
a new version of the patch).

however there is one think that I discovered that needs to be fixed with
the interaction with
the Dump Thread Enhancement...I'll add that patch to this or the other JIRA
entry.

other comments inline below.

thanks for review.

/Jonas

On Fri, Dec 12, 2014 at 12:08 PM, Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx
> wrote:

> Jonas Oreland <jonaso@xxxxxxxxxx> writes:
>
> Thanks for the MDEV-162 patch! Here is my review.
>
> I think the patch looks fine, and we should take it. Just a few questions
> below, and one minor comment:
>
>
> > === modified file 'mysql-test/suite/rpl/t/rpl_semi_sync.test'
> > --- mysql-test/suite/rpl/t/rpl_semi_sync.test 2014-08-07 16:06:56 +0000
> > +++ mysql-test/suite/rpl/t/rpl_semi_sync.test 2014-10-20 10:52:49 +0000
> > @@ -59,7 +59,6 @@
> >  echo [ status of semi-sync on master should be ON even without any
> semi-sync slaves ];
> >  show status like 'Rpl_semi_sync_master_clients';
> >  show status like 'Rpl_semi_sync_master_status';
> > ---replace_result 305 304
> >  show status like 'Rpl_semi_sync_master_yes_tx';
>
> I'm curious why you decided to remove this --replace_result?
> I see in bzr history that Monty added this --replace_result without any
> explanation why... :-/
>

i removed it cause the result didn't contain 304 or 305
my guess is that it hasn't for several years, and hence it was pure
obfuscation.

i also don't know/understand why it was added in the first place, maybe
because committer was too lazy to investigate?



>
> > === added file 'mysql-test/suite/rpl/t/rpl_semi_sync_wait_point.opt'
> > --- mysql-test/suite/rpl/t/rpl_semi_sync_wait_point.opt       1970-01-01
> 00:00:00 +0000
> > +++ mysql-test/suite/rpl/t/rpl_semi_sync_wait_point.opt       2014-10-20
> 09:25:13 +0000
> > @@ -0,0 +1,1 @@
> > +--log_bin
>
> Any reason not to use --source include/have_log_bin.inc instead?
>

hmm...damn it, don't remember...
i guess it would work equally well (or even better from mtr.pl point of
view)

> === added file 'mysql-test/suite/rpl/t/rpl_semi_sync_wait_point.test'
> > --- mysql-test/suite/rpl/t/rpl_semi_sync_wait_point.test      1970-01-01
> 00:00:00 +0000
> > +++ mysql-test/suite/rpl/t/rpl_semi_sync_wait_point.test      2014-10-20
> 09:25:13 +0000
>
> > +--echo # Kill the waiting thread; it should die immediately.
> > +KILL @other_connection_id;
> > +
> > +--echo # Collect the error from the INSERT thread; it should be
> disconnected.
> > +connection other;
> > +--error 2013
>
> Please use here:
>
>   --error 2013,ER_CONNECTION_KILLED
>

ack


>
> to avoid a rare test failure (apparently the ER_CONNECTION_KILLED can
> occasionally reach the client before the socket is closed, I've seen it in
> our
> Buildbot).
>
> > +--echo # Collect the error from the INSERT thread; it should be
> disconnected.
> > +connection other;
> > +--error 2013
>
> Aso here use --error 2013,ER_CONNECTION_KILLED
>

ack


>
>
> > === modified file 'sql/replication.h'
> > --- sql/replication.h 2014-08-07 16:06:56 +0000
> > +++ sql/replication.h 2014-10-20 09:25:13 +0000
>
> > @@ -114,7 +117,13 @@
> >  */
> >  enum Binlog_storage_flags {
> >    /** Binary log was sync:ed */
> > -  BINLOG_STORAGE_IS_SYNCED = 1
> > +  BINLOG_STORAGE_IS_SYNCED = 1,
> > +
> > +  /** First(or alone) in a group commit */
> > +  BINLOG_GROUP_COMMIT_LEADER = 2,
> > +
> > +  /** Last(or alone) in a group commit */
> > +  BINLOG_GROUP_COMMIT_TRAILER = 4
> >  };
>
> What is the reason for introducing these flags?
> As far as I can see from the patch, they are set, but never read?
>

this is for a (yet) unpublished optimization, that is
to not sem-sync all individual transactions in a group-commit
but only the last one (trailer) (and i added leader for completeness).

performance impact is significant, i'll publish the patch+result later.
i haven't ported
http://my-replication-life.blogspot.se/2014/03/faster-semisync-replication.html
(since I found the implementation hackish by violating the vio-abstraction).
but hope that optimization above will give about the same performance
wise...

/Jonas




>
> Thanks,
>
>  - Kristian.
>