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@knielsen-hq.org> wrote:

Jonas Oreland <jonaso@google.com> 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.