← Back to team overview

maria-developers team mailing list archive

Re: [Commits] b60fb6d: MDEV-11636 Extra persistent columns on slave always gets NULL in RBR

 

sachin.setiya@xxxxxxxxxxx writes:

> revision-id: b60fb6daee2fcc2f433a50b5a5639065f5a46fe8 (mariadb-galera-10.0.28-8-gb60fb6d)
> parent(s): be430b80df0cdd4eba32df1570195721dbfd1b39
> author: Sachin Setiya
> committer: Sachin Setiya
> timestamp: 2016-12-22 19:22:36 +0530
> message:
>
> MDEV-11636 Extra persistent columns on slave always gets NULL in RBR
>
> Problem:- In replication if slave has extra persistent column then these
> column are not computed while applying write-set from master.

Generally, when slave and master differs there will be situations where
things will not fully work. However, in this case, the result is really
corrupt data (a persistent column with wrong values), and I agree this
should be fixed (and the patch looks simple).

I am not familiar with the code for virtual columns. I assume you looked at
the normal code path for virtual columns and made similar functionality for
row-based replication applier. Your code looks reasonable to me.

You add code in Rows_log_event::write_row(). What about update_row? It is
not tested in the testcase, you should add test of the UPDATE case (and fix
the code, if needed).

The testcase needs more work, see comments inline.

Getting replication tests to work reliably (not generate occasional spurious
errors on buildbot etc.) is quite tricky (spoken from painful experience :-).
Please push this to a feature tree and make sure the test passes on all
builders before pushing to main.

(A useful trick also is to run on your own machine something like:

  ./mysql-test-run.pl rpl.rpl_alter_extra_persistent --repeat=1000

to catch at least some timing-dependent errors).

> diff --git a/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test b/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test

> +--sync_slave_with_master
> +
> +connection slave;
> +select * from t1;

Use ORDER BY a (here and below), to avoid any risk of getting different
output order.

> +--connection master
> +insert into t1 values(5);
> +insert into t1 values(6);
> +
> +--connection slave
> +sleep 2;

Never use sleep in test cases. Use an appropriate mechanism to sync the
slave to the master.

> +select * from t1;
> +
> +--echo #Break Unique Constraint
> +alter table t1 add column z4 int as (a % 6) persistent unique;
> +
> +--connection master
> +
> +--echo #entering duplicate value for slave persistent column
> +insert into t1 values(7);
> +select * from t1;
> +
> +--connection slave
> +select * from t1;
> +--query_vertical show slave status;

This will not work reliably. You cannot put SHOW SLAVE STATUS output in
.result, it will not be stable between test runs. And there is a race on
whether the slave has time to hit the duplicate error.

For this, use the existing include/wait_for_slave_sql_error.inc file, which
can reliably detect a slave failure. See comments in the file and example
usage in other test files.

> +
> +--connection master
> +drop table t1;
> +
> +--connection slave
> +alter table t1 drop column z4   ;
> +start slave;
> +drop table t1;

This drop table should not be there (the drop table from the master will be
replicated).

> diff --git a/sql/log_event.cc b/sql/log_event.cc
> index 1cc58cd..a8f035b 100644
> --- a/sql/log_event.cc
> +++ b/sql/log_event.cc
> @@ -11541,6 +11541,9 @@ Rows_log_event::write_row(rpl_group_info *rgi,
>      DBUG_RETURN(error);
>    }
>  
> +  if ((error= fill_extra_persistent_columns(table, &m_cols))
> +    DBUG_RETURN(error);
> +

Is similar code needed for update_row? At least, this should be tested in
the test case.

 - Kristian.