maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sachin!

On Mar 15, Sachin Setiya wrote:
> Hi Serg!
> 
> On Sun, Mar 12, 2017 at 11:50 PM, Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> > Hi, Sachin!
> >
> > On Feb 10, Sachin Setiya wrote:
> >> revision-id: b6f4d16a015fc778186ae4d2c1820a78cd9645d1 (mariadb-10.2.3-157-gb6f4d16)
> >> parent(s): 5285504857df6caf417c8a56601913a95c9f7abc
> >> author: Sachin Setiya
> >> committer: Sachin Setiya
> >> timestamp: 2017-02-10 18:24:12 +0530
> >> message:
> >>
> >> MDEV-10436 non-deterministic vcol does not force rbr
> >>
> >> Problem:- Since 10.2.1 one can create virtual generated columns with
> >> non-deterministic functions. Arguably, in cases like
> >> INSERT t1 SELECT * FROM t2
> >> if t2 contains such non-deterministic generated columns and replication
> >> mode is MIXED, the statement should be logged in the row format. But it is
> >> logged in statement format.
> >
> > Some explanation of the fix would be good here. Especially if it's not
> > trivial.
>
> Agreed, will change it.
>
> > Why did you need set_cs_bin_format_row_if_mixed_and_nd_vcol()
> > function and why are you doing something in end_send()? You've fixed
> > THD::decide_logging_format(), why was that not enough?
>
> No it wasnt , We require both Fix in thd_decide_logging_format and in end_send()
> For example consider this "create table t3 as select * from t1"
> Logging format of this can be evaluated by
> THD::decide_logging_format(), But in the case of
> insert into t3 select t1.a , t1.c , t2.b from t1 , t2 where t1.b = t2.a.
> We first have evaluate select condition then  table->read_set will be set.
> Then is why set_cs_bin_format_row_if_mixed_and_nd_vcol() after end_data().

Okay. A couple of minor comments about the test:

1. please add a test for "insert into t3 select a , c from t1;"
   it should be logged as a statement. it is, I've tried it, but let's
   have it in the test.

2. You can have --sync_slave_with_master instead of a pair of
    --connection slave + --sync_with_master.

Now the big thing. This THD::decide_logging_format() is designed to be
run only once, see the comment for the commit that added it into
lock_tables(), it's 41783de5496.

I see that you're changing the binlog format before the first write, so
that's ok. Two thoughts though:

1. it's still bad to decide on binlog format twice.
2. I don't know if it's safe to decide on binlog format that late as
   you're doing it.

So, I'd suggest the following: to address 1) we'll decide on a
binlogging format once, but late. Every table modification goes through
handler::check_table_binlog_row_based_internal(). So may be we can call
THD::decide_logging_format() from there? Of course, it should only be
called once per statement, for for every row write, so there should be
some kind of a guard. DDLs might need some special treatment, perhaps.

This shoud be done in a separate refactoring commit, before MDEV-10436
bug fix.

Now, 2). If the test suite will pass after moving
THD::decide_logging_format that late, then, I hope, we can assume that
it didn't break anything.

And if that won't work, because some code might need the corect binlog
format earlier - it's good that we've found it, without this test your
patch would've broken it and we'd never knew. In that case, let's see
how late we can do THD::decide_logging_format() without breaking
anything.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx